Fixed #12952 -- Made admin's model history change messages use form labels instead of field names.

[CuriousLearner]

cc @carltongibson

PS: Special Thanks to @andrewgodwin helping in resolving the issue around serializing the gettext versioned proxy objects during DjangoCon US sprints :)

[CuriousLearner]

Hi @claudep

I've addressed your reviews. The failure of sqlite3 DB seems unrelated to the changes.

Can you please take another pass here?

[claudep]

The following diff should explain better than explanations two more issues with this patch:

index 77cd87b1f4..c692d5dc3f 100644
--- a/tests/admin_views/admin.py
+++ b/tests/admin_views/admin.py
@@ -882,11 +882,21 @@ class CityInlineAdmin(admin.TabularInline):
 
 
 class StateAdminForm(forms.ModelForm):
+    nolabel_form_field = forms.BooleanField(required=False)
+
     class Meta:
         model = State
         fields = '__all__'
         labels = {'name': 'The State Name'}
 
+    @property
+    def changed_data(self):
+        data = super().changed_data
+        if data:
+            # Add arbitrary name to changed_data to test change message construction.
+            return data + ['not_a_form_field']
+        return data
+
 
 class StateAdmin(admin.ModelAdmin):
     inlines = [CityInlineAdmin]

[CuriousLearner]

@claudep I see. For the first diff, what do you think should be a good way to handle that? Should we give out an error that such field does not even exist, or just suppress it silently, and just giving out the label name for fields that actually exist on the model?

[claudep]

That was my initial remark on the KeyError. Instead of the 'password' special case, I would write something like (field_name instead of f):

            try:
                verbose = form.fields[field_name].label or field_name
            except KeyError:
                verbose = field_name
            change_field_labels.append(verbose)

[CuriousLearner]

Hey @claudep

Does this looks okay now?

[claudep]

I have still some more comments, thanks for you patience!

[CuriousLearner]

@claudep Can you please take another pass here?

Thank you for your patience.

[carltongibson]

This is looking good I think. Thanks for the work @CuriousLearner and @claudep!

There was just a small hole in the test coverage for the fieldsets case. (I don't know what you think about that?) (FIXED)

[claudep]

So apart from the small remaining question about the docstring, this looks good to me, thanks for that work!

[timgraham]

I think this merits a mention under "backwards incompatible changes" in docs/releases/2.2.txt so that users aren't caught off guard by this change.

[CuriousLearner]

Hi @timgraham

I've addressed most of your review. There is just one small bit left out.

Here: #10531 (comment)

The issue is if I use the form.changed_data directly in the block where translations are disabled, then I also get Created field which ultimately gets appended to changed_field_labels

It seems like when I do take an alias but try to use form.changed_data directly in loop everything is working perfectly.

I'm not sure how evaluating form.changed_data at different times (outside and inside of block that disables translation) is causing this behavior. Do you have any idea about this?

cc @carltongibson @claudep

[CuriousLearner]

Hey @timgraham @carltongibson @claudep

Do you have any idea of what is going on with form.changed_data in this context?

[claudep]

I found the reason about the changed_data stuff. It's about date interpretation, which varies depending on the current language. So 12/3/2018 is 12th of March for a French person, but 3rd of December for a US person (default when translations are deactivated). That's why the changed_data detection can change depending on the active language. Also note it's a cached property. I guess you'll have to let the changed_data computing outside of the translation-deactivated block, with a comment explaining why.

[CuriousLearner]

hmm... this is indeed interesting!

@claudep I've added a comment explaining the reasoning of evaluation of form.changed_data before deactivating translations.

[CuriousLearner]

@pope1ni I wasn't sure about your release note entry suggestion, but I've still done it.

Do you mind having a second look here?

@timgraham Can you please have a look at this?

[ngnpope]

Spotted a couple of minor niggles and have clarified what I meant regarding the release note.

[CuriousLearner]

@pope1ni What do you think about this patch now?

[CuriousLearner]

@timgraham Is this good to go now?

[carltongibson]

Hi @CuriousLearner.

Wowser right. There's a lot of history here. I've tried to resolve the conversations that look out of date to clear it up a bit.

I've left a few small comments. Can you resolve those, rebase and update to 3.0 etc. I'll then ask everyone to give it another look to make sure everyone's points were addressed. (But looks good! Thanks you for the effort.)

[CuriousLearner]

Hey @carltongibson

Apologies for getting this late, but I've fixed this now.

Please let me know if there are any more changes needed.

Thanks!

[CuriousLearner]

Hey @carltongibson

This PR was approved but is pending for a while to be checked in.

Can you please take a look?

[carltongibson]

Hi @CuriousLearner. Yes, super. Let's get this in 🙂 Can you rebase (rather than merge in master)?

I'll have a look this week.

[carltongibson]

Hi @CuriousLearner. Thank you for the extended effort here. (And everyone for review.)

I've been through the history, resolved the conversations, and rebased. This looks ready to go now.
👍

[CuriousLearner]

Sorry for not being able to come to rebase this earlier and thank you for merging this in @carltongibson :) 🎉