Opened 2 years ago

Closed 13 months ago

#20133 closed New feature (fixed)

Admin: Add deletion summary

Reported by: jonash Owned by: areski
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: areski 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 jonash 2 years ago.

Download all attachments as: .zip

Change History (17)

Changed 2 years ago by jonash

comment:1 Changed 2 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Great idea!

comment:2 Changed 2 years ago by jonash

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

comment:3 Changed 2 years ago by jonash

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 2 years ago by jonash

bump

comment:5 Changed 21 months ago by jonash

bump

comment:6 Changed 13 months ago by areski

+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 13 months ago by areski

  • Cc areski added

comment:8 Changed 13 months ago by timgraham

  • 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 13 months ago by areski

  • Owner changed from nobody to areski
  • Status changed from new to assigned

comment:10 Changed 13 months ago by areski

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 13 months ago by areski

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

comment:12 Changed 13 months 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 13 months ago by areski

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 13 months ago by timgraham

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

Looks simple enough, but needs tests and release notes.

comment:15 Changed 13 months ago by areski

  • Needs documentation unset
  • Needs tests unset

Tests + release notes added

comment:16 Changed 13 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to 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