fix(spp_programs): locale-aware total_amount_in_words (#236) - #300
fix(spp_programs): locale-aware total_amount_in_words (#236)#300Tarekchehahde wants to merge 1 commit into
Conversation
…P#236) Derive num2words language from env context or user lang instead of hardcoding English; fall back to en for unsupported locales. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request introduces localization support for converting total amounts into words using the num2words library, falling back to English if the requested language is not supported, and adds a corresponding test case for French. The reviewer suggested improving the language resolution logic to try the full language code (preserving regional dialects like pt_BR) before falling back to the base language code and finally English.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| lang_code = (self.env.context.get("lang") or self.env.user.lang or "en_US").split("_")[0] | ||
| try: | ||
| amount_in_words = num2words(record.total_amount, lang=lang_code).title() | ||
| except NotImplementedError: | ||
| amount_in_words = num2words(record.total_amount, lang="en").title() | ||
| record.total_amount_in_words = f"{amount_in_words} {record.currency_id.name}" |
There was a problem hiding this comment.
By immediately splitting the language code on _ (e.g., converting pt_BR to pt), regional dialects supported by num2words (such as Brazilian Portuguese pt_BR or British English en_GB) are lost, falling back to the generic base language.
A more robust approach is to attempt translation using the full language code first, then fall back to the base language code (split by _), and finally fall back to English (en) if neither is supported.
| lang_code = (self.env.context.get("lang") or self.env.user.lang or "en_US").split("_")[0] | |
| try: | |
| amount_in_words = num2words(record.total_amount, lang=lang_code).title() | |
| except NotImplementedError: | |
| amount_in_words = num2words(record.total_amount, lang="en").title() | |
| record.total_amount_in_words = f"{amount_in_words} {record.currency_id.name}" | |
| lang_code = self.env.context.get("lang") or self.env.user.lang or "en_US" | |
| for lang in (lang_code, lang_code.split("_")[0], "en"): | |
| try: | |
| amount_in_words = num2words(record.total_amount, lang=lang).title() | |
| break | |
| except NotImplementedError: | |
| continue | |
| record.total_amount_in_words = f"{amount_in_words} {record.currency_id.name}" |
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thanks for this — the root-cause analysis in #236 is spot-on, and you patched the right file despite the issue linking to cycle_base.py (which turns out to be dead code — it's never imported in models/__init__.py; the live model is in cycle.py). The fallback chain and the NotImplementedError guard are both reasonable. Two must-fixes before this can merge, plus a few suggestions.
Must-fix 1: add @api.depends_context("lang")
total_amount_in_words is a non-stored computed field. Odoo caches non-stored computes per record per transaction, and the cache only varies by context keys declared via @api.depends_context. Without it, the first language computed in a transaction wins: read the field in English, then read cycle.with_context(lang="fr_FR").total_amount_in_words in the same transaction, and you get the cached English string back with no recompute. That hits exactly the flows #236 cares about — e.g. the QWeb summary report (report/summary_report.xml renders doc.total_amount_in_words) when rendered under a partner's language after the record was already read in another language.
@api.depends("total_amount", "currency_id")
@api.depends_context("lang")
def _compute_total_amount_in_words(self):The current test doesn't catch this because it calls the private _compute_total_amount_in_words() directly, which force-overwrites the cache — it validates the compute body, not the field's behavior. Please restructure the test to read the field naturally: read cycle.total_amount_in_words (English) first, then read cycle.with_context(lang="fr_FR").total_amount_in_words on the same record in the same transaction. That ordering fails without the decorator and passes with it, so it pins the actual bug.
Must-fix 2: version bump + HISTORY entry
spp_programs is a released module (currently 19.0.2.2.1 on 19.0). Please bump the micro version in __manifest__.py (to whatever is next when this merges — 19.0.2.2.2 as of today) and add an entry to spp_programs/readme/HISTORY.md.
Suggestions (non-blocking)
- Drop the
.split("_")[0]—num2wordsalready tries the full lang code first and then falls back to the first 2 letters before raisingNotImplementedError. Passing the full locale is simpler and preserves the region-specific converters num2words ships (fr_CH,fr_BE,es_CO, …). Your existingtry/except NotImplementedErroralready covers truly unsupported codes. - Hoist the lang lookup out of the
for record in selfloop — it's the same value every iteration. - Add a fallback test with an unsupported locale (e.g.
with_context(lang="xx_XX")) asserting the English output, so theexceptbranch is covered.
CI note
No workflows ever ran on this PR (fork PR awaiting first-contributor approval at open time, and no pushes since). Your next push should trigger CI automatically.
Summary
env.context['lang']orenv.user.lang(withenfallback) fornum2wordsin_compute_total_amount_in_words.test_total_amount_in_wordsto assert French output underfr_FRcontext.Test plan
./spp test spp_programs—test_total_amount_in_wordsfr_FR, confirm amount-in-words renders in FrenchFixes #236
Made with Cursor