Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] base: propagate changes on implied groups update #177301

Closed

Conversation

ivantodorovich
Copy link
Contributor

Steps to reproduce:

  1. Add a stored computed field on res.users that depends on groups_id
  2. Add a constraint on res.users that depends on groups_id
  3. Give the user a group
  4. Modify the given group and add an implied group

Result:

  • The computed field is not recomputed
  • The constraint is not checked

The reason for this is that the users groups are updated with a raw SQL query, and the ORM is not aware of the changes.

This is a regression since 5f12e24. This issue was partially detected and fixed in 459e6dc, but only for a single constraint.

This commit makes sure that the ORM properly propagates the changes to computed fields and all constraints, not just the one that was explicitly checked.


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@ivantodorovich ivantodorovich marked this pull request as ready for review August 20, 2024 19:37
@robodoo
Copy link
Contributor

robodoo commented Aug 20, 2024

Pull request status dashboard

@C3POdoo C3POdoo requested review from a team, kmagusiak and rco-odoo and removed request for a team August 20, 2024 19:39
odoo/addons/base/models/res_users.py Outdated Show resolved Hide resolved
odoo/addons/base/models/res_users.py Show resolved Hide resolved
@ivantodorovich ivantodorovich force-pushed the 16.0-fix-res-group-implied branch from b3a68a6 to d3f3f65 Compare August 21, 2024 13:55
@ivantodorovich
Copy link
Contributor Author

Thanks for your review, @kmagusiak ❤️

@ivantodorovich
Copy link
Contributor Author

Gentle ping.
Any chance we can merge this one? 😄

@kmagusiak
Copy link
Contributor

@ryv-odoo @Gorash can you review?

Copy link
Contributor

@Gorash Gorash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When changing a group on a user we clear the cache (see _get_invalidation_fields). It is not useful to be specific when changing a group. Just use self.clear_caches()
The code will be simpler and will have the same behavior as when you modify a user (which is actually the case). In addition, some rights may be cached and must be invalidated.
ping @ryv-odoo

@ivantodorovich
Copy link
Contributor Author

Hmmm 🤔 I fail to see how clear_caches would help to solve this:

  • The computed field is not recomputed
  • The constraint is not checked

Am I missing something?

Copy link
Member

@rco-odoo rco-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add some test?

Copy link
Contributor

@Gorash Gorash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I'm confusing it with future devs. All this code will be deleted no longer be needed.
Could you add a test please.

Steps to reproduce:

1. Add a stored computed field on `res.users` that depends on `groups_id`
2. Add a constraint on `res.users` that depends on `groups_id`
3. Give the user a group
4. Modify the given group and add an implied group

Result:

- The computed field is not recomputed
- The constraint is not checked

The reason for this is that the users groups are updated with a raw SQL query,
and the ORM is not aware of the changes.

This is a regression since 5f12e24. This issue was partially detected and fixed
in 459e6dc, but only for a single constraint.

This commit makes sure that the ORM properly propagates the changes to computed
fields and all constraints, not just the one that was explicitly checked.
@ivantodorovich ivantodorovich force-pushed the 16.0-fix-res-group-implied branch from bef6175 to ba8a7d2 Compare December 9, 2024 14:25
@ivantodorovich
Copy link
Contributor Author

ivantodorovich commented Dec 9, 2024

Could you please add some test?

I added the unit tests ✅

Runbot is failing with some unrelated errors, though, with a message saying "This error is already known."

FAIL: TestConfigurator.test_01_configurator_flow
Traceback (most recent call last):
  File "/data/build/odoo/addons/test_website_modules/tests/test_configurator.py", line 17, in test_01_configurator_flow
    self.start_tour('/web#action=website.action_website_configuration', 'configurator_flow', login="admin")
  File "/data/build/odoo/odoo/tests/common.py", line 1869, in start_tour
    return self.browser_js(url_path=url_path, code=code, ready=ready, **kwargs)
  File "/data/build/odoo/odoo/tests/common.py", line 1849, in browser_js
    self.fail('%s\n\n%s' % (message, error))
AssertionError: The test code "odoo.startTour('configurator_flow')" failed

Tour configurator_flow failed at step click next (trigger: button.o_configurator_show)

@Gorash
Copy link
Contributor

Gorash commented Dec 10, 2024

lgfm
ping @rco-odoo ;)

Copy link
Member

@rco-odoo rco-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robodoo
Copy link
Contributor

robodoo commented Dec 10, 2024

@rco-odoo you may want to rebuild or fix this PR as it has failed CI.

@rco-odoo
Copy link
Member

rco-odoo commented Dec 10, 2024

@ivantodorovich good job, and thanks for your contribution!
I am taking care of the unrelated failing test.

@ivantodorovich
Copy link
Contributor Author

Thanks 🙏🏻!

robodoo pushed a commit that referenced this pull request Dec 10, 2024
Steps to reproduce:

1. Add a stored computed field on `res.users` that depends on `groups_id`
2. Add a constraint on `res.users` that depends on `groups_id`
3. Give the user a group
4. Modify the given group and add an implied group

Result:

- The computed field is not recomputed
- The constraint is not checked

The reason for this is that the users groups are updated with a raw SQL query,
and the ORM is not aware of the changes.

This is a regression since 5f12e24. This issue was partially detected and fixed
in 459e6dc, but only for a single constraint.

This commit makes sure that the ORM properly propagates the changes to computed
fields and all constraints, not just the one that was explicitly checked.

closes #177301

Signed-off-by: Raphael Collet <rco@odoo.com>
robodoo pushed a commit that referenced this pull request Dec 10, 2024
Steps to reproduce:

1. Add a stored computed field on `res.users` that depends on `groups_id`
2. Add a constraint on `res.users` that depends on `groups_id`
3. Give the user a group
4. Modify the given group and add an implied group

Result:

- The computed field is not recomputed
- The constraint is not checked

The reason for this is that the users groups are updated with a raw SQL query,
and the ORM is not aware of the changes.

This is a regression since 5f12e24. This issue was partially detected and fixed
in 459e6dc, but only for a single constraint.

This commit makes sure that the ORM properly propagates the changes to computed
fields and all constraints, not just the one that was explicitly checked.

closes #177301

Signed-off-by: Raphael Collet <rco@odoo.com>
@robodoo robodoo closed this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants