Fix notifications in CWS being translated after being formatted #1046
Conversation
We would mark for translation the message with placeholders (which is correct) but we would look the message up in the catalog after having formatted it, and then of course we wouldn't find any translation. This also fixes an issue with message extraction.
Codecov Report
@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
+ Coverage 61.98% 61.98% +<.01%
==========================================
Files 227 227
Lines 16405 16421 +16
==========================================
+ Hits 10169 10179 +10
- Misses 6236 6242 +6
Continue to review full report at Codecov.
|
stefano-maggiolo
left a comment
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lerks)
cms/server/contest/communication.py, line 50 at r1 (raw file):
class UnacceptableQuestion(Exception):
Maybe a cleanup would be to have an intermediate exception type that handles text_params in a standard way?
cms/server/contest/handlers/taskusertest.py, line 141 at r1 (raw file):
except UnacceptableUserTest as e: logger.info("Sent error: `%s' - `%s'", e.subject, e.formatted_text) self.notify_error(e.subject, e.text, e.text_params)
Can you avoid changing notify_error and use formatted_text here too?
lw
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lerks)
cms/server/contest/communication.py, line 50 at r1 (raw file):
Previously, stefano-maggiolo (Stefano Maggiolo) wrote…
Maybe a cleanup would be to have an intermediate exception type that handles text_params in a standard way?
Sure, I can create a common superclass of all these exceptions that defines the __init__ method and the formatted_text property. Where should I put it? It would be nice to have it in the same file as add_notification, notify_error, etc... but that would create a circular dependency (I think). cms/server/contest/__init__.py could work but we don't like putting code in __init__ files, do we? Should I create a new file, say cms/server/contest/util.py, just for this?
cms/server/contest/handlers/taskusertest.py, line 141 at r1 (raw file):
Previously, stefano-maggiolo (Stefano Maggiolo) wrote…
Can you avoid changing notify_error and use formatted_text here too?
I don't see what you mean. formatted_text is untranslated: if we pass it to notify_error then it would need to be de-formatted in order to find its translation. We need to pass to notify_error the two parts (text and params) separate, and then add_notification (called by notify_error) will translate the first and only then put them together. Am I missing something?
stefano-maggiolo
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
cms/server/contest/communication.py, line 50 at r1 (raw file):
Previously, lerks (Luca Wehrstedt) wrote…
Sure, I can create a common superclass of all these exceptions that defines the
__init__method and theformatted_textproperty. Where should I put it? It would be nice to have it in the same file asadd_notification,notify_error, etc... but that would create a circular dependency (I think).cms/server/contest/__init__.pycould work but we don't like putting code in__init__files, do we? Should I create a new file, saycms/server/contest/util.py, just for this?
The new file sounds good, but thinking about it, it's not so important, feel free to ignore.
cms/server/contest/handlers/taskusertest.py, line 141 at r1 (raw file):
Previously, lerks (Luca Wehrstedt) wrote…
I don't see what you mean. formatted_text is untranslated: if we pass it to notify_error then it would need to be de-formatted in order to find its translation. We need to pass to notify_error the two parts (text and params) separate, and then add_notification (called by notify_error) will translate the first and only then put them together. Am I missing something?
Oh, right. I guess you could cal a e.formatted_translated_text(self._), but the advantage becomes smaller or nonexisting.
We would mark for translation the message with placeholders (which is correct) but we would look the message up in the catalog after having formatted it, and then of course we wouldn't find any translation.
This also fixes an issue with message extraction.
This fixes #1040 and supersedes #1041.
This change is