Opened 11 years ago

Closed 10 years ago

#20133 closed New feature (fixed)

Admin: Add deletion summary

Reported by: Jonas H. Owned by: Areski Belaid
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Areski Belaid Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

Here's a first version of a patch that adds a short modelname => number-of-deleted-objects-of-that-model map to the admin object deletion confirmation page.

http://i.imgur.com/g5Ltqzc.png

The patch is by no means RFC, it's only meant to be a first sketch in order to get feedback about the idea in general. I've also done some refactoring as preparation for implementing this feature. To be specific, there was some code that mixed logic and presentation that is now a bit more separated; although there's still potential to reduce complexity by factoring get_deleted_objects *into* the views (i.e. removing the helper function entirely).

Attachments (1)

patch1.patch​ (15.0 KB ) - added by Jonas H. 11 years ago.

Download all attachments as: .zip

Change History (17)

by Jonas H., 11 years ago

Attachment: patch1.patch​ added

comment:1 by Julien Phalip, 11 years ago

Triage Stage: Unreviewed → Accepted

Great idea!

comment:2 by Jonas H., 11 years ago

Any comments on the patch or the idea in general? Feature requests? Site design/structure?

comment:3 by Jonas H., 11 years ago

Btw there's some serious DRY violation in that particular area of the admin, both in the views and templates. If anyone with a bit more knowledge of the admin code base has any tips on how to refactor this, please let me know.

comment:4 by Jonas H., 11 years ago

bump

comment:5 by Jonas H., 10 years ago

bump

comment:6 by Areski Belaid, 10 years ago

+1
I quite like the idea, pity we didn't get much traction on this ticket.
Maybe you should send a Pull Request on github, it might be a easier way for core dev to rebase the patch on master and test it.

comment:7 by Areski Belaid, 10 years ago

Cc: Areski Belaid added

comment:8 by Tim Graham, 10 years ago

Patch needs improvement: set

Anyone is welcome to rebase the patch and test it. I would just say that I'd prefer that commits separated refactoring of logic from the adding of new features if possible as this makes the diffs smaller and easier to review.

comment:9 by Areski Belaid, 10 years ago

Owner: changed from nobody to Areski Belaid
Status: new → assigned

comment:10 by Areski Belaid, 10 years ago

here the new PR ​https://github.com/django/django/pull/3052
I followed timgraham advice and so removed the refactoring part, I will send an other PR for it

comment:11 by Areski Belaid, 10 years ago

follow-up PR for the refactoring part with a small correction

comment:12 by Tim Graham <timograham@…>, 10 years ago

In 3687aa00932c0e951643790fcb0f863049aa0446:

Simplified admin delete confirmation templates using {% elif %}.

Thanks jonash for the initial patch; refs #20133.

comment:13 by Areski Belaid, 10 years ago

Ignore the previous PR, it breaks many things on Django.
This new one is more straightforward and should pass all Django tests.
​https://github.com/django/django/pull/3063

comment:14 by Tim Graham, 10 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: unset

Looks simple enough, but needs tests and release notes.

comment:15 by Areski Belaid, 10 years ago

Needs documentation: unset
Needs tests: unset

Tests + release notes added

comment:16 by Tim Graham <timograham@…>, 10 years ago

Resolution: → fixed
Status: assigned → closed

In 302145328560ded44bcfded8a67a1e7df08b411b:

Fixed #20133 -- Added summary to admin deletion confirmation pages.

Thanks jonash for the suggestion and initial patch.

Note: See TracTickets for help on using tickets.
Back to Top