[IMP][16.0] project_sequence: set analytic's code as per project's sequence_code#1658
[IMP][16.0] project_sequence: set analytic's code as per project's sequence_code#1658arnaudlayec wants to merge 9 commits intoOCA:16.0from
code as per project's sequence_code#1658Conversation
|
Hi @anddago78, @yajo, |
yajo
left a comment
yajo
left a comment
There was a problem hiding this comment.
Please could you share screenshots or screencast to see how it was before and how it is now? I think that will make it easier to review.
Code looks OK. Just one doubt.
Thank you for your review! Screencast.from.2026-01-20.14-08-38.mp4Se how Analytic Account's field "code" is filled in, as per project's sequence code: |
Hello @yajo , would you like to review this PR? |
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for this improvement, @arnaudlayec. Syncing the project's name and sequence_code separately into the analytic account's name and code fields is the right approach -- it lets the AAC's own display_name handle formatting and makes code-based lookups possible (e.g. for analytic distribution in imports).
Code is clean and CI is green. One small gap I noticed:
In test_sync_analytic_account, you now assert proj.analytic_account_id.name == proj.name, but since the method now also writes code, it would be good to assert proj.analytic_account_id.code == proj.sequence_code as well -- for consistency with the other tests that already do this.
Also a note on the create condition change (not vals.get("sequence_code", False) -> "sequence_code" not in vals): this means passing sequence_code="" will no longer auto-assign a sequence. I see this aligns with v18.0, just flagging it as a behavioral change for other reviewers.
Otherwise LGTM.
| proj.analytic_account_id = analytic_account | ||
| proj._sync_analytic_account_name() | ||
| self.assertEqual(proj.analytic_account_id.name, proj.display_name) | ||
| proj._sync_analytic_account() |
There was a problem hiding this comment.
The method now syncs both name and code, but this test only asserts name. Consider adding:
self.assertEqual(proj.analytic_account_id.code, proj.sequence_code)The other test methods (test_analytic_account_after_creation_named, test_sequence_copied_to_name_if_emptied) already verify both fields.
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Good improvement. Syncing sequence_code into the analytic account's code field separately from name is the right call -- it leverages the AAC's own display_name formatting and enables code-based lookups (useful for import scenarios with analytic_distribution).
CI is green, tests are updated and cover the new behavior well. The create condition change (not vals.get(...) to "sequence_code" not in vals) is a deliberate v18.0 alignment and has been discussed in the review thread.
One minor suggestion below (non-blocking).
Review posted via CorporateHub OCA review campaign
| proj.analytic_account_id = analytic_account | ||
| proj._sync_analytic_account_name() | ||
| self.assertEqual(proj.analytic_account_id.name, proj.display_name) | ||
| proj._sync_analytic_account() |
There was a problem hiding this comment.
suggestion (non-blocking): Since _sync_analytic_account() now writes both name and code, consider adding:
self.assertEqual(proj.analytic_account_id.code, proj.sequence_code)The other test methods (test_analytic_account_after_creation_named, test_sequence_copied_to_name_if_emptied) already verify both fields -- this would keep coverage consistent.
There was a problem hiding this comment.
Thanks for review, indeed the analytic's code should be tested here too, it's now added
|
This PR has the |
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: project-16.0/project-16.0-project_task_pull_request Translate-URL: https://translation.odoo-community.org/projects/project-16-0/project-16-0-project_task_pull_request/
Currently translated at 100.0% (7 of 7 strings) Translation: project-16.0/project-16.0-project_task_pull_request Translate-URL: https://translation.odoo-community.org/projects/project-16-0/project-16-0-project_task_pull_request/it/
Currently translated at 100.0% (48 of 48 strings) Translation: project-16.0/project-16.0-project_timesheet_time_control Translate-URL: https://translation.odoo-community.org/projects/project-16-0/project-16-0-project_timesheet_time_control/pt_BR/
[IMP]16.0] define analytic's code
| limit=1, | ||
| ) | ||
| ) | ||
| default_plan_id = cls.env.company.analytic_plan_id |
There was a problem hiding this comment.
question: regarding this change, I see a behavioral change. Before, an AAP without company would be considered as a default plan. Now, only the one from the company will be considered.
Could you please explain why this change?
Thanks!
9 participants
Before, the project's
display_namewas synched in the project's analytic account (aac) name. This way, the "code" field of aac was empty, and the name had both the project's code and the name in the default format "[code] - [name]" Proposal : synch the 2 fields ("sequence_code" and "name") of the project respectively in the 2 fields ("code" and "name") of the aacThis allow:
display_namemethod)analytic_distributionusing the aac's codeThis PR also update tests.
I needed to fix some:
default_plan_idmust be computed once to existtest_project_without_sequenceandtest_project_with_empty_sequence. Doing that required to change project'screatemethodNote: if OK, I can port to upper version.