Opened 15 years ago
Closed 5 years ago
#12952 closed Cleanup/optimization (fixed)
Models history doesn't use verbose names
Reported by: | Antonio Cangiano | Owned by: | Sanyam Khurana |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | vvangelovski@…, Sanyam Khurana | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
The history for a model object (within the admin section) should show human-readable messages, favoring verbose names over field names. However, this is not currently the case. For example, consider a model with the following class variable:
pub_date = models.DateTimeField("date published")
Changing the publication date for an object of that model, will display "Changed pub_date." in its admin history, rather than "Change date published." as one would expect (as older versions of Django did).
Attachments (2)
Change History (39)
by , 15 years ago
Attachment: | django-admin-history-verbose-name.diff added |
---|
comment:1 by , 15 years ago
Has patch: | set |
---|---|
milestone: | 1.2 |
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 years ago
Type: | → Bug |
---|
comment:3 by , 14 years ago
Severity: | → Normal |
---|
comment:4 by , 13 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
UI/UX: | unset |
comment:5 by , 13 years ago
Patch needs improvement: | unset |
---|---|
UI/UX: | set |
comment:6 by , 13 years ago
Cc: | added |
---|
by , 13 years ago
Attachment: | admin_history.diff added |
---|
comment:7 by , 13 years ago
Needs tests: | unset |
---|
Updated patch to apply to current trunk and added a test for the history view.
comment:8 by , 13 years ago
Triage Stage: | Accepted → Design decision needed |
---|
Marking as DDN until an answer to ticket:14358#comment:3 is provided.
comment:9 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
The objection was "for debug purposes, it would be more useful to have the field names, as they are necessarily unique, untranslated, etc."
In my opinion:
- uniqueness is a weak argument: if you have two fields with the same name, you're just asking for trouble;
- translation isn't an issue at all: in doubt, just switch the website to the default language of your codebase.
comment:11 by , 10 years ago
Patch needs improvement: | set |
---|
Patch will need to be updated to apply cleanly.
comment:12 by , 10 years ago
Patch needs improvement: | unset |
---|
A Github pull request (#4496) has been opened for this ticket.
comment:13 by , 10 years ago
Type: | Bug → Cleanup/optimization |
---|---|
Version: | 1.2-beta → master |
I am thinking it might be best to try to address #21113 first (allow messages to be translated to the current language rather than displaying the language that was active for editing) since this ticket would require a new implementation for that one.
comment:15 by , 9 years ago
I think it's better to work with the form labels than fields' verbose name,
I think It's actually a more natural flow and more consistent if the label is different then the verbose name. (That's the idea behind the field label option, right?!)
My Approach would be to gather the changed fields' labels, then send it to get_text_list
https://github.com/django/django/blob/master/django/contrib/admin/options.py#L925-L936
translated_changed_fields = [form.fields[f].label for f in form.changed_data] change_message.append(_('Changed %s.') % get_text_list(translated_changed_fields, _('and'))) #again for formset for changed_object, changed_fields in formset.changed_objects: translated_changed_fields = [formset.forms[0].fields[f].label for f in changed_fields] #using formset.forms[0] looks ugly i agree , couldn't find better ways But hey , it's a changed formset , index [0] is there ! change_message.append(_('Changed %(list)s for %(name)s "%(object)s".') % {'list': get_text_list(translated_changed_fields, _('and')), #...
Created a duplicate 24990
Regards;
comment:16 by , 9 years ago
It seems to me that the verbose name is a safer choice to account for the fact that you might have several different editing forms each with a different label.
comment:17 by , 9 years ago
IMHO, that depend on the intended audience of the changed message in the history.
If it's the developer then verbose_name are the way to go.
If it's the site 'content' administrator, this is what i think is the case, then labels are more of expected behavior; and in that case the "maybe different" verbose name can be confused as a bug.
"I edited the field "More info", why does it show me that i edited 'description' which is not even present on the form ?"
:)
Kind regards;
comment:18 by , 9 years ago
I see that this pull request didn't get merged.
I can go for it with the form label resolution.
comment:21 by , 9 years ago
I left some comments for improvement on the pull request. Don't forget to uncheck "Patch needs improvement" on this ticket after you update it so it appears in the review queue, thanks!
comment:22 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:23 by , 9 years ago
Patch needs improvement: | set |
---|
Some discussion on the mailing list confirmed my feeling that we should fix #21113 first.
comment:24 by , 6 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
I'm trying to modify the current patch and apply it cleanly to the current master branch.
comment:27 by , 6 years ago
Patch needs improvement: | unset |
---|
I've addressed the reviews. Can you please take another pass?
comment:28 by , 6 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Review comments have been addressed. Patch looks good to go. (Will just leave time for Claude to follow up if he wants to.)
comment:29 by , 6 years ago
Addressed Claude's review on the patch and it is ready for a review again and possibly check-in.
comment:30 by , 6 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
There were just a couple more comments on the ticket. Sanyam, please uncheck Patch needs improvement when you've had a look at those. (Thanks for the great effort!)
comment:31 by , 6 years ago
Patch needs improvement: | unset |
---|
Hey Carlton, I've addressed the last nit-picks in the patch. Thank you for your patience!
comment:32 by , 6 years ago
Hello,
Can you please have a look at the recent updates on this patch?
Please let me know if any more changes are needed.
Thanks!
comment:33 by , 6 years ago
(The patch is in the review queue, no need to add a comment asking for a review. I was on vacation last week and am catching up on things.)
comment:34 by , 6 years ago
Patch needs improvement: | set |
---|
PR looks not far off after several reviews. Once comments are addressed it would be good if previous reviewers could give one more look to make sure we don't miss anything.
comment:35 by , 5 years ago
Patch needs improvement: | unset |
---|
Hi Carlton,
The PR was approved a long ago.
Can you please take a look and let me know if you think there is anything more to be done here?
Thanks!
comment:36 by , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Initial patch, probably needs tests.