Opened 4 years ago

Closed 2 years ago

#20133 closed New feature (fixed)

Admin: Add deletion summary

Reported by: Jonas H. Owned by: Areski Belaid
Component: contrib.admin Version: master
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. 4 years ago.

Download all attachments as: .zip

Change History (17)

Changed 4 years ago by Jonas H.

Attachment: patch1.patch added

comment:1 Changed 3 years ago by Julien Phalip

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Great idea!

comment:2 Changed 3 years ago by Jonas H.

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

comment:3 Changed 3 years ago by Jonas H.

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 Changed 3 years ago by Jonas H.

bump

comment:5 Changed 3 years ago by Jonas H.

bump

comment:6 Changed 2 years ago by Areski Belaid

+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 Changed 2 years ago by Areski Belaid

Cc: Areski Belaid added

comment:8 Changed 2 years ago by Tim Graham

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 Changed 2 years ago by Areski Belaid

Owner: changed from nobody to Areski Belaid
Status: newassigned

comment:10 Changed 2 years ago by Areski Belaid

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 Changed 2 years ago by Areski Belaid

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

comment:12 Changed 2 years ago by Tim Graham <timograham@…>

In 3687aa00932c0e951643790fcb0f863049aa0446:

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

Thanks jonash for the initial patch; refs #20133.

comment:13 Changed 2 years ago by Areski Belaid

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 Changed 2 years ago by Tim Graham

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

Looks simple enough, but needs tests and release notes.

comment:15 Changed 2 years ago by Areski Belaid

Needs documentation: unset
Needs tests: unset

Tests + release notes added

comment:16 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

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