[18.0][MIG] base_tier_validation: Migration to 18.0 #992

xaviedoanhduy · 2025-01-16T02:42:55Z

supersedes: #966

changes are follow up from old PR:

fix the icon on systray
fix the dymanic color alert on list reviewers
fix of not recalculating reviewCount when updating review requests (TierReviewService). don't use useState to calculate tierReviewCounter because the service will update services["mail.store"]
replaced old-style string interpolation with f-string and placeholder formatting
remove old jquery and replace with javascript available objects
optimize multi-company rule
optimize using new API ORM _read_group for better performance
replace deprecated output directives (t-esc)

bosd · 2025-01-16T04:34:48Z

Can you place your commits in a separate commit?
As there is also #990. Maybe we can cherry pick to merge these changes into there. If there is any difference.

xaviedoanhduy · 2025-01-16T04:51:09Z

Can you place your commits in a separate commit? As there is also #990. Maybe we can cherry pick to merge these changes into there. If there is any difference.

hi @bosd, that commit fixed the icon bug, but i consider that fix as part of PR Migration (because old PR is not yet merged). and author of this PR (#990) also mentioned we have duplicated work, you can read the conversation here: xaviedoanhduy#1 (comment)

bosd · 2025-01-16T07:11:07Z

Thanks for the clarification. I was getting a bit lost between the migration pr of this module.
Guess it was better to review this one.
Will do so when I have time.

Side note, on this one codecov is red.

ivantodorovich

Hey, thanks for taking care of this!

There are a few things that need to be corrected, IMO.
Could you check my remarks? 🙏🏻

Thanks!

ivantodorovich · 2025-01-16T14:09:58Z

base_tier_validation/models/res_users.py

+        review_groups = self.env["tier.review"].read_group(domain, ["model"], ["model"])
+        for review_group in review_groups:
+            model = review_group["model"]
+            Model = self.env[model]
+            reviews = self.env["tier.review"].search(review_group.get("__domain"))


💡 tip: This could probably be improved by leveraging the private _read_group method, and aggregating ['id:recordset'], and thus not needing to do a search afterwards

@ivantodorovich Good spot. that was on my mind as well.
There are some more places in the code of this module where the orm is flooded by searches from within a for loop.

The _read_group method was introduced in the orm in V17.
IMO that improvement could go into V17 and be forward ported here. Should not be blocking for this pr.

(But I've been told, there is even a more improved method in the V18 orm. But I don't know the details or have looked into that one.)

I think :recordset was added in 18.0.
In 17.0 a similar thing can be accomplished with :array_agg, and then browsing the list of ids.

FYI, _read_group and ir.recordset already exist in version 17.0, see: odoo/odoo#110737

and by the way I also applied the above code improvement from your suggestion, thank @ivantodorovich

@xaviedoanhduy Thanks!
Actually there are still some places in the code where a search is performed inside a for loop.
(Bad practice for performance.)
https://github.com/OCA/server-ux/pull/992/files#diff-7cca2e2b43a392c79d13759687633b9d88cfe56e6c2238b762e2c8d461690607R123-R124

Maybe you can attend to this, or put it on the roadmap.

FYI, _read_group and ir.recordset already exist in version 17.0, see: odoo/odoo#110737

Oh, indeed. Thanks!

hello, I've improved some search functions that are called in several loops into a separate commit

base_tier_validation/models/tier_review.py

base_tier_validation/models/tier_validation.py

base_tier_validation/static/src/components/tier_review_widget/tier_review_widget.esm.js

miikanissi

Functional test looks good to me - no issues found

bosd

Thanks for your efforts and work on this: 👍

Functional test:

Colors are not showing in review bar as mentioned by @celm1990 in #990 (review)

xaviedoanhduy · 2025-01-21T01:14:06Z

Thanks for your efforts and work on this: 👍

Functional test:

Colors are not showing in review bar as mentioned by @celm1990 in #990 (review)

hi, color issue on reviewers list has been fixed. thanks for the tests

bosd

Funcitonal test. LGTM 👍

Non-blocking / nitpicking.
JS error/warning

UI can be improved a bit.

core:

sebalix · 2025-01-22T16:00:38Z

/ocabot migration base_tier_validation

xaviedoanhduy · 2025-01-23T09:46:13Z

Funcitonal test. LGTM 👍

Non-blocking / nitpicking. JS error/warning ![image](https://private-user-images.githubusercontent.com/11499387/405347355-

hi @bosd, I'm not sure the error from your dev tool comes from this module.

I also took this opportunity to improve the interface a bit.

bosd · 2025-01-23T10:19:24Z

Thanks, If you want, you can include #991

celm1990

LGTM, just a minor change and let's go.
Additionally, please squash the administrative commits a bit. Thanks for the effort!

celm1990 · 2025-01-25T14:15:57Z

base_tier_validation/static/src/components/tier_review_widget/tier_review_widget.esm.js

+    onToggleCollapse(ev) {
+        const panelHeading = ev.currentTarget.closest(".panel-heading");
+        const collapseDiv = panelHeading.nextElementSibling.matches("div#collapse1")
+            ? panelHeading.nextElementSibling
+            : null;
+        if (!collapseDiv) return;
+        this.state.collapse = !this.state.collapse;
+        if (this.state.collapse) {
+            collapseDiv.style.display = "none";
+        } else {
+            collapseDiv.style.display = "block";
+        }
+    }


The first time you click the widget, it does not open; a second click is needed to collapse it. Please take a look. On the second attempt, it works fine, but if you refresh the view again, it doesn't work. I think the initial state might be related.

@xaviedoanhduy could you please check this

hi @celm1990, in my country it is Lunar New Year. I will start working again on 02/03/2025. Thank you for raising the issue and btw happy Lunar New Year 2025

Happy lunar year! The year of intelligence and love

hi @bosd and @celm1990, the issue of clicking twice on the widget when loading the page for the first time has been resolved

…on and validate if possible.

Co-authored-by: Do Anh Duy <[email protected]>

celm1990

Tested locally, LGTM.

Just a non-blocking comment: I think it's possible to squash the administrative commits a bit, following this guide: Merge commits in pull requests.

Thanks for the effort!

ping @sebalix @LoisRForgeFlow @pedrobaeza

StefanRijnhart

Thanks everyone, great work here on the review and the follow up!

StefanRijnhart · 2025-02-04T17:23:51Z

/ocabot merge nobump

OCA-git-bot · 2025-02-04T17:23:59Z

On my way to merge this fine PR!
Prepared branch 18.0-ocabot-merge-pr-992-by-StefanRijnhart-bump-nobump, awaiting test results.

Signed-off-by StefanRijnhart

OCA-git-bot · 2025-02-04T17:28:00Z

@StefanRijnhart your merge command was aborted due to failed check(s), which you can inspect on this commit of 18.0-ocabot-merge-pr-992-by-StefanRijnhart-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

StefanRijnhart · 2025-02-04T17:35:45Z

Merge failed on OCA/oca-addons-repo-template#291. Doh!

StefanRijnhart · 2025-02-04T18:09:23Z

Let's try again after this hour long scheduled brownout

/ocabot merge nobump

OCA-git-bot · 2025-02-04T18:09:30Z

This PR looks fantastic, let's merge it!
Prepared branch 18.0-ocabot-merge-pr-992-by-StefanRijnhart-bump-nobump, awaiting test results.

OCA-git-bot · 2025-02-04T18:13:32Z

Congratulations, your PR was merged at 7526531. Thanks a lot for contributing to OCA. ❤️

bosd · 2025-05-08T08:22:14Z

Thanks for the team work on this migration!

Is any of you using this one already in production?
Have you encountered the following bugs in V18:

After validation of a record, it is impossible to duplicate (An error thrown, regression in V18)
Unable to send email after record validation as discovered in [18.0][MIG] account_move_tier_validation: Unable to mail invoice account-invoicing#1998

miikanissi · 2025-05-14T21:44:55Z

Thanks for the team work on this migration!

Is any of you using this one already in production? Have you encountered the following bugs in V18:

After validation of a record, it is impossible to duplicate (An error thrown, regression in V18)

Unable to send email after record validation as discovered in [18.0][MIG] account_move_tier_validation: Unable to mail invoice account-invoicing#1998

We are using this in production. We have noticed a few issues.

Some computed and/or related fields that change after validation can trigger an under validation exception. This can also happen if some fields are written by an action. This sounds like the same issue as the unable to send email issue. Essentially, nothing can change on the record after validation. We have fixed this by just adding a ton of fields in the validation exceptions rules.
Readonly fields configured in the field definition are editable. For example, the date_approve field on a Purchase Order is a readonly field in the field definition. But after setting up validations the field will be editable. We had to manually make a view level change to set readonly="1" on the affected fields. TLDR; Readonly Field definition is ignored.

LoisRForgeFlow · 2025-06-30T14:38:12Z

Thanks for the team work on this migration!
Is any of you using this one already in production? Have you encountered the following bugs in V18:

After validation of a record, it is impossible to duplicate (An error thrown, regression in V18)

Unable to send email after record validation as discovered in [18.0][MIG] account_move_tier_validation: Unable to mail invoice account-invoicing#1998

We are using this in production. We have noticed a few issues.

Some computed and/or related fields that change after validation can trigger an under validation exception. This can also happen if some fields are written by an action. This sounds like the same issue as the unable to send email issue. Essentially, nothing can change on the record after validation. We have fixed this by just adding a ton of fields in the validation exceptions rules.

Readonly fields configured in the field definition are editable. For example, the date_approve field on a Purchase Order is a readonly field in the field definition. But after setting up validations the field will be editable. We had to manually make a view level change to set readonly="1" on the affected fields. TLDR; Readonly Field definition is ignored.

@miikanissi Thanks for the heads-up.

It looks like there is not feedback from the migrator and reviewers in v18. If your comments are still applicable, I would kindly ask you to create issues in this repository to track the problem. Otherwise this comment will be forgotten here.

xaviedoanhduy mentioned this pull request Jan 16, 2025

[18.0][MIG] base_tier_validation: Migration to 18.0 #966

Closed

ivantodorovich suggested changes Jan 16, 2025

View reviewed changes

ivantodorovich mentioned this pull request Jan 16, 2025

[18.0][MIG] account_move_tier_validation: Migration to 18.0 OCA/account-invoicing#1830

Closed

miikanissi approved these changes Jan 16, 2025

View reviewed changes

xaviedoanhduy force-pushed the 18.0-mig-base_tier_validation branch from bc507f3 to 3108668 Compare January 17, 2025 11:46

bosd suggested changes Jan 18, 2025

View reviewed changes

ivantodorovich approved these changes Jan 20, 2025

View reviewed changes

rjaraspearhead mentioned this pull request Jan 20, 2025

[18.0][MIG] base_tier_validation: Migration to 18.0 #990

Closed

xaviedoanhduy force-pushed the 18.0-mig-base_tier_validation branch from 3108668 to 60977bf Compare January 21, 2025 01:13

xaviedoanhduy requested a review from bosd January 21, 2025 01:19

bosd approved these changes Jan 21, 2025

View reviewed changes

OCA-git-bot added the approved label Jan 21, 2025

OCA-git-bot added this to the 18.0 milestone Jan 22, 2025

OCA-git-bot mentioned this pull request Jan 22, 2025

Migration to version 18.0 #948

Open

26 tasks

xaviedoanhduy force-pushed the 18.0-mig-base_tier_validation branch 2 times, most recently from 9aedf95 to e7545d3 Compare January 23, 2025 09:42

celm1990 suggested changes Jan 25, 2025

View reviewed changes

OCA-git-bot removed the approved label Jan 25, 2025

Ricardo-MC mentioned this pull request Jan 27, 2025

[18][MIG] purchase_request_tier_validation: Migration to 18 OCA/purchase-workflow#2482

Merged

1 task

LoisRForgeFlow added 4 commits February 3, 2025 10:31

[9.0][ADD] base_tier_validation

cea0793

fix: blocking unneded records

5b6ff6a

[9.0][IMP] base_tier_validation: tries automatically request validati…

d902bfe

…on and validate if possible.

[9.0][IMP] base_tier_validation: filter out reviews not pending

9e7bd61

AlexPForgeFlow and others added 4 commits February 3, 2025 10:31

[IMP] base_tier_validation: add icon

b1516a5

[BOT] post-merge updates

7608efa

[IMP] base_tier_validation: pre-commit auto fixes

0867915

[IMP] base_tier_validation: odoo upgrade_code fixes

008fffb

Co-authored-by: Do Anh Duy <[email protected]>

xaviedoanhduy force-pushed the 18.0-mig-base_tier_validation branch from e7545d3 to a01b6d5 Compare February 3, 2025 08:37

kevinkhao and others added 2 commits February 3, 2025 15:40

[MIG] base_tier_validation: Migration to 18.0

20f557a

Co-authored-by: Do Anh Duy <[email protected]>

[IMP] base_tier_validation: using _read_group for better performance

4c6095b

xaviedoanhduy force-pushed the 18.0-mig-base_tier_validation branch from a01b6d5 to 4c6095b Compare February 3, 2025 08:42

bosd approved these changes Feb 3, 2025

View reviewed changes

celm1990 approved these changes Feb 3, 2025

View reviewed changes

OCA-git-bot added the approved label Feb 3, 2025

StefanRijnhart approved these changes Feb 4, 2025

View reviewed changes

OCA-git-bot added a commit that referenced this pull request Feb 4, 2025

Merge PR #992 into 18.0

dfc7b29

Signed-off-by StefanRijnhart

OCA-git-bot added the bot is merging ⏳ label Feb 4, 2025

OCA-git-bot removed the bot is merging ⏳ label Feb 4, 2025

OCA-git-bot added the bot is merging ⏳ label Feb 4, 2025

OCA-git-bot merged commit b2d6731 into OCA:18.0 Feb 4, 2025
5 of 7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Feb 4, 2025

celm1990 mentioned this pull request Feb 4, 2025

[18.0][MIG] sale_tier_validation: Migration to 18.0 OCA/sale-workflow#3417

Closed

Uh oh!

[18.0][MIG] base_tier_validation: Migration to 18.0 #992

[18.0][MIG] base_tier_validation: Migration to 18.0 #992

Uh oh!

Conversation

xaviedoanhduy commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bosd commented Jan 16, 2025

Uh oh!

xaviedoanhduy commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bosd commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivantodorovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miikanissi left a comment

Choose a reason for hiding this comment

Uh oh!

bosd left a comment

Choose a reason for hiding this comment

Uh oh!

xaviedoanhduy commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bosd left a comment

Choose a reason for hiding this comment

Uh oh!

sebalix commented Jan 22, 2025

Uh oh!

xaviedoanhduy commented Jan 23, 2025

Uh oh!

bosd commented Jan 23, 2025

Uh oh!

celm1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celm1990 left a comment

Choose a reason for hiding this comment

Uh oh!

StefanRijnhart left a comment

Choose a reason for hiding this comment

Uh oh!

StefanRijnhart commented Feb 4, 2025

xaviedoanhduy commented Jan 16, 2025 •

edited

Loading

xaviedoanhduy commented Jan 16, 2025 •

edited

Loading

bosd commented Jan 16, 2025 •

edited

Loading

xaviedoanhduy commented Jan 21, 2025 •

edited

Loading