Opened 11 years ago

Closed 9 years ago

Last modified 9 years ago

#21113 closed New feature (fixed)

django_admin_log table stores messages in different languages depending on which language was active while editing.

Reported by: dimyur27@… Owned by: Claude Paroz <claude@…>
Component: contrib.admin Version: dev
Severity: Normal Keywords: admin logs i18n
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In my project (in development) there are 3 different languages supported. Admin site supposed to have 3 main superusers, each is a native speaker of one of the languages. If one superuser edited an object, another one is not able to read what his fellow superuser previously did, because "Change history" page doesn't respect the current user language and displays the messages in the language that was current when the changes took place.
Maybe change_message field should be removed and the messages generated each time they are displayed.
So is object_repr, because object's representation may be dependant on the current language too.
Thank you.

Change History (24)

comment:1 by wim@…, 11 years ago

Triage Stage: UnreviewedAccepted
Version: 1.6-beta-1master

Accepted. I'd like messages to be language-agnostic and translatable.

comment:2 by Tim Graham, 11 years ago

Type: UncategorizedNew feature

comment:3 by jooyous, 10 years ago

Owner: changed from nobody to jooyous
Status: newassigned

comment:4 by jooyous, 10 years ago

So at this point I understand how to change this so all subsequent log entries behave correctly. But currently the translated strings are being saved away as part of the object, and I think (?) this means we've lost the information of the starting language, so the only way I can think of fixing *existing* histories is by writing a script and having the user run it, specifying their relevant languages?

comment:5 by Claude Paroz, 10 years ago

Fixing existing history might be impossible to achieve. I wouldn't set that as a goal for this ticket.

comment:6 by anonymous, 10 years ago

I can't get ugettext to translate "Changed" for some reason, even though there are bits of code where it does it successfully and I'm not sure what I'm doing wrong.

in reply to:  6 comment:7 by jooyous, 10 years ago

Replying to anonymous:

I can't get ugettext to translate "Changed" for some reason, even though there are bits of code where it does it successfully and I'm not sure what I'm doing wrong.

Whoops, that was me. Didn't realize I was logged out.

comment:8 by jooyous, 10 years ago

Hellooo I am still working on this.

comment:9 by jooyous, 10 years ago

Has patch: set
Needs tests: set
Owner: jooyous removed
Patch needs improvement: set
Status: assignednew

https://github.com/django/django/pull/3345

Here's a partial fix for this bug. It changes the log messages so that they're saved in the database in English and then parsed and translated when they need to be displayed. However, due to the way the change messages were written upon being entered into the database, I wasn't able to properly distinguish between different types of save messages. For example, if an object (for example, a Poll or Choice) contained a period or quotation marks in the body, I could not tell the difference between the end of the message and the punctuation within the message body. So I'm only submitting a partial fix. From what I could test, it works pretty well for Users. I was also having some trouble getting certain words to translate.

If having history messages displayed in different languages is a priority, I would recommend restructuring the logging functionality entirely, and have the messages be saved in an unambiguously delimited format (yaml? json? something!) so that they can be formed into translatable sentences after they are extracted from the database. I would be willing to work on this if an issue gets made!

in reply to:  9 comment:10 by Claude Paroz, 10 years ago

Replying to jooyous:
(...)

If having history messages displayed in different languages is a priority, I would recommend restructuring the logging functionality entirely, and have the messages be saved in an unambiguously delimited format (yaml? json? something!) so that they can be formed into translatable sentences after they are extracted from the database. I would be willing to work on this if an issue gets made!

Yes, I think that this would be the right approach.

comment:11 by Claude Paroz, 9 years ago

Needs tests: unset
Patch needs improvement: unset

comment:12 by Tim Graham, 9 years ago

Patch needs improvement: set

Left comments for improvement on the pull request.

comment:13 by Claude Paroz <claude@…>, 9 years ago

In 35c4198:

Moved LogEntry-related tests to their own test case

Thanks Tim Graham for reviewing and contributing to the patch.
Refs #21113.

comment:14 by Claude Paroz, 9 years ago

Patch needs improvement: unset

comment:15 by Chris Cogdon, 9 years ago

I know this is very late to the party, and it's not that critical of an issue, but generally it is considered good form [citation-needed] to ensure the top level of a JSON structure is an object/dictionary, rather than a list, string, int, etc. django.http.response.JsonResponse for example considers anything except a dict to be 'unsafe'.

The change is simple... just use {'changes':[ ...]} rather than simply [...], but this does involve changing the entire patch. 😦

Version 0, edited 9 years ago by Chris Cogdon (next)

comment:16 by Claude Paroz, 9 years ago

Changing the entire patch is not an issue at all.

I would appreciate though if you could find some reference about this design issue.

comment:17 by Tim Graham, 9 years ago

From a bit of searching, I guess the "good form" is to mitigate JSON hijacking? Those concerns probably aren't relevant here.

comment:18 by Tim Graham, 9 years ago

Patch needs improvement: set

Left a few more comments on the pull request.

comment:19 by Claude Paroz, 9 years ago

Patch needs improvement: unset

comment:20 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:21 by Claude Paroz <claude@…>, 9 years ago

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In cf7894be:

Fixed #21113 -- Made LogEntry.change_message language independent

Thanks Tim Graham for the review.

comment:22 by Tim Graham, 9 years ago

Possible flaky test seen on Jenkins:

admin_utils.test_logentry.LogEntryTests.test_logentry_change_message

Stacktrace

Traceback (most recent call last):
  File "/mnt/jenkinsdata/workspace/django-master/database/mysql/python/python3.5/tests/admin_utils/test_logentry.py", line 65, in test_logentry_change_message
    self.assertEqual(logentry.get_change_message(), 'Changed title and hist.')
AssertionError: 'Changed something' != 'Changed title and hist.'
- Changed something
+ Changed title and hist.

comment:23 by Claude Paroz, 9 years ago

Probably an issue with .latest(), on MySQL, not by chance! We may find another way to get the target logentry based on another criterion...

comment:24 by Claude Paroz <claude@…>, 9 years ago

In 4a03e6f2:

Refs #21113 -- Updated test to allow for bad MySQL time resolution

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