Conversation
- bump version to 18.0.1.0.0 - replace legacy JS with Odoo 18 assets system - add web.assets_backend entry for translation_letter_counting_js.js
- Migrating the code for the ‘correspondence’ model and translation workflows so that they work with Odoo 18 (formerly Odoo 14) - Adjusting methods, fields, and dependencies for compatibility with Odoo 18's new APIs and conventions
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the sbc_translation module, which provides a backend management system for SBC letter translations. It includes an XMLRPC-API for an external OWL-based translation platform, management of translators and their competencies, and automated workflows for letter translation queues. Several technical issues were identified, including potential crashes in supervisor assignment, missing module dependencies, and inefficient or incorrect computed field dependencies. Improvements were also suggested for multi-user data isolation and Odoo 18 coding standards.
| for letter in self.filtered(lambda l: not l.translation_supervisor_id): | ||
| letter.translation_supervisor_id = supervisors[ | ||
| randint(0, len(supervisors) - 1) | ||
| ] | ||
| return True |
There was a problem hiding this comment.
The random selection of a supervisor will crash if no users are found in the manager group. randint(0, len(supervisors) - 1) will raise a ValueError if supervisors is empty because the range will be (0, -1). You should check if supervisors is not empty before attempting to access an element.
| for letter in self.filtered(lambda l: not l.translation_supervisor_id): | |
| letter.translation_supervisor_id = supervisors[ | |
| randint(0, len(supervisors) - 1) | |
| ] | |
| return True | |
| if supervisors: | |
| for letter in self.filtered(lambda l: not l.translation_supervisor_id): | |
| letter.translation_supervisor_id = supervisors[ | |
| randint(0, len(supervisors) - 1) | |
| ] |
| if child_langs: | ||
| dst_lang = child_langs[-1] | ||
| else: | ||
| dst_lang = self.env.ref("advanced_translation.lang_compassion_english") |
There was a problem hiding this comment.
The module advanced_translation is referenced here but it is not listed as a dependency in the __manifest__.py file. This will cause a ValueError at runtime if the module is not installed or if the reference is not found during module loading.
|
|
||
| @api.depends("all_letter_ids") | ||
| def _compute_current_letters(self): | ||
| for competence in self: |
There was a problem hiding this comment.
The number_current_letters field is stored and depends on all_letter_ids. However, its value also depends on the state of the letters. If a letter's state changes (e.g., from 'to do' to 'done'), this field will not be recomputed because all_letter_ids (the relation itself) hasn't changed. You should add .state to the dependency.
@api.depends("all_letter_ids.state")| translator.nb_translated_letters = len(translator.translated_letter_ids) | ||
|
|
||
| @api.depends("translated_letter_ids") | ||
| def _compute_nb_translated_letters_this_year(self): |
There was a problem hiding this comment.
The nb_translated_letters_this_year field is stored but its dependency doesn't include the field that determines the count (translate_date). If a letter is translated (date is set), the count won't update. You should depend on translated_letter_ids.translate_date.
@api.depends("translated_letter_ids.translate_date")| with file_open("sbc_translation/static/tp/index.html") as app: | ||
| return app.read() |
There was a problem hiding this comment.
The controller assumes that index.html always exists in the static folder. If the file is missing (e.g., because the build step was skipped), this will raise a FileNotFoundError. It's safer to check for file existence or handle the exception.
| # T2007 : If a TranslationLetterCounting wizard is active, the letter should be added to it | ||
| translation_letter_counting_wizards = self.env[ | ||
| "translation.letter.counting.wizard" | ||
| ].search([]) |
There was a problem hiding this comment.
Searching for all translation.letter.counting.wizard records without filtering by user might lead to unexpected behavior in a multi-user environment. A letter finished by one user would be added to the counting wizards of all other users who happen to have one active. You should filter by create_uid.
translation_letter_counting_wizards = self.env[
"translation.letter.counting.wizard"
].search([("create_uid", "=", self.env.uid)])| current_letters = self.env["correspondence"].search( | ||
| [ | ||
| ("state", "=", "Global Partner translation queue"), | ||
| ("translation_competence_id", "=", competence.id), | ||
| ("translation_competence_id", "!=", False), | ||
| ] | ||
| ) | ||
| competence.current_letter_ids = current_letters | ||
| competence.number_current_letters = len(current_letters) | ||
|
|
There was a problem hiding this comment.
Performing a search inside a loop over self is inefficient (O(N) queries). Since all_letter_ids is a One2many field, Odoo already prefetches these records. You can use filtered on the recordset to get the records for each competence in a single query's worth of data.
for competence in self:
current_letters = competence.all_letter_ids.filtered(
lambda l: l.state == "Global Partner translation queue"
)
competence.current_letter_ids = current_letters
competence.number_current_letters = len(current_letters)| nb_translated_letters_this_year = fields.Integer( | ||
| "Translations this year", | ||
| compute="_compute_nb_translated_letters_this_year", | ||
| store=True, | ||
| ) | ||
| nb_translated_letters_last_year = fields.Integer( | ||
| "Translations last year", | ||
| compute="_compute_nb_translated_letters_last_year", | ||
| store=True, | ||
| ) |
There was a problem hiding this comment.
These fields are stored and depend on the current year (fields.Datetime.now().year). Stored computed fields are only recomputed when their dependencies change. When the year changes (e.g., on January 1st), these fields will remain incorrect until a letter is added or removed from the translator. It is generally better not to store fields that depend on the current time, or to have a scheduled job to recompute them periodically.
| correspondence_ids = self.env["correspondence"].search( | ||
| [("translate_done", ">=", self.start_of_counting)] | ||
| ) | ||
| self.correspondence_ids = [(6, 0, correspondence_ids)] |
There was a problem hiding this comment.
In Odoo 18, you should assign the recordset directly to the Many2many field instead of using the command tuple (6, 0, ...). This is cleaner and more idiomatic.
| self.correspondence_ids = [(6, 0, correspondence_ids)] | |
| self.correspondence_ids = correspondence_ids |
1 participant
No description provided.