Opened 2 years ago

Closed 5 weeks ago

Last modified 5 weeks 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: master
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 Changed 2 years ago by wim@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.6-beta-1 to master

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

comment:2 Changed 2 years ago by timo

  • Type changed from Uncategorized to New feature

comment:3 Changed 20 months ago by jooyous

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

comment:4 Changed 20 months ago by jooyous

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 Changed 20 months ago by claudep

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

comment:6 follow-up: Changed 20 months ago by 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.

comment:7 in reply to: ↑ 6 Changed 20 months ago by jooyous

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 Changed 19 months ago by jooyous

Hellooo I am still working on this.

comment:9 follow-up: Changed 16 months ago by jooyous

  • Has patch set
  • Needs tests set
  • Owner jooyous deleted
  • Patch needs improvement set
  • Status changed from assigned to new

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!

comment:10 in reply to: ↑ 9 Changed 16 months ago by claudep

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 Changed 7 weeks ago by claudep

  • Needs tests unset
  • Patch needs improvement unset

comment:12 Changed 7 weeks ago by timgraham

  • Patch needs improvement set

Left comments for improvement on the pull request.

comment:13 Changed 7 weeks ago by Claude Paroz <claude@…>

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 Changed 6 weeks ago by claudep

  • Patch needs improvement unset

comment:15 Changed 6 weeks ago by chmarr

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 {'entries':[ ...]}, or {'additions':[...], 'deletions':[...], ...} rather than simply [...], but this does involve changing the entire patch. 😦

Last edited 6 weeks ago by chmarr (previous) (diff)

comment:16 Changed 6 weeks ago by claudep

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 Changed 5 weeks ago by timgraham

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

comment:18 Changed 5 weeks ago by timgraham

  • Patch needs improvement set

Left a few more comments on the pull request.

comment:19 Changed 5 weeks ago by claudep

  • Patch needs improvement unset

comment:20 Changed 5 weeks ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:21 Changed 5 weeks ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from new to closed

In cf7894be:

Fixed #21113 -- Made LogEntry.change_message language independent

Thanks Tim Graham for the review.

comment:22 Changed 5 weeks ago by timgraham

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 Changed 5 weeks ago by claudep

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 Changed 5 weeks ago by Claude Paroz <claude@…>

In 4a03e6f2:

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

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