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

[16.0][FIX] audit_log: fix regression (Add/Remove User Group) #2834

Merged

Conversation

BT-anieto
Copy link
Contributor

When adding/removing a group in a user (and tracking a user or a partner), odoo sends a 'reified' val 'in_group_{group_id}'. We must convert this value into the real groups_id one.

This bug is now appearing after the merge of #2814 because of

self.invalidate_recordset(vals.keys())

Steps to reproduce:

  1. Set a rule for re.users models
  2. Go to a user and select/unselect one of the groups he belongs to (only visible in debug mode)
  3. Save

image

@BT-anieto BT-anieto force-pushed the 16.0.audit_log-users-in_group_id-fix branch 2 times, most recently from 5573382 to 04a8b85 Compare February 8, 2024 13:50
@BT-anieto
Copy link
Contributor Author

@StefanRijnhart Could you please take a look?

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks! This should be safe as it's what core Odoo does first thing in res.users' own write method: https://github.com/odoo/odoo/blob/47efddb/odoo/addons/base/models/res_users.py#L1611-L1612

To make sure this does not drop out in a future version, would it be possible to add a test?

@BT-anieto
Copy link
Contributor Author

@StefanRijnhart I'll do it asap (Next week)

When adding/removing a group in a user (and tracking a user or a partner), odoo sends a 'reified' val 'in_group_{group_id}'. We must convert this value into the real groups_id one.
@BT-anieto BT-anieto force-pushed the 16.0.audit_log-users-in_group_id-fix branch from 04a8b85 to 8de1324 Compare February 15, 2024 08:39
@BT-anieto
Copy link
Contributor Author

@StefanRijnhart Done! :)

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Those are beautiful tests, thank you!

@StefanRijnhart StefanRijnhart changed the title [16.0][FIX] audit_log: Add/Remove User Group [16.0][FIX] audit_log: fix regression (Add/Remove User Group) Feb 15, 2024
@StefanRijnhart StefanRijnhart added this to the 16.0 milestone Feb 15, 2024
@BT-anieto
Copy link
Contributor Author

@StefanRijnhart Do you know if it needs another review in order to be merged?

@StefanRijnhart
Copy link
Member

@BT-anieto Yes, it does.

Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

3 lines of code
80lines of test !
thanks for this
LGTM

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@StefanRijnhart
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-2834-by-StefanRijnhart-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4e8afe7 into OCA:16.0 Feb 19, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants