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

[17.0][FIX] base_user_role : fix tests to group groups into a role #330

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

mb-ife
Copy link

@mb-ife mb-ife commented Jan 9, 2025

No description provided.

@OCA-git-bot
Copy link
Contributor

Hi @jcdrubay, @sebalix, @novawish,
some modules you are maintaining are being modified, check this out!

@mb-ife mb-ife changed the title [16.0][FIX] base_user_role : fix tests to group groups into a role [17.0][FIX] base_user_role : fix tests to group groups into a role Jan 17, 2025
@ng-ife ng-ife force-pushed the 3015-17.0-base_user_role_fix_tests branch from e1f58eb to 34acea2 Compare January 23, 2025 13:44
@ng-ife
Copy link

ng-ife commented Jan 23, 2025

Add force group recalculation in role assignment tests to handle
group synchronization issues that occur when base_user_role and
auditlog modules are both installed. Without forcing updates, tests
fail in odoo.sh and OCA CI environments due to group assignment
mismatches.

Force parameter in set_groups_from_roles() ensures proper group
synchronization regardless of auditlog's presence. No issues occur
when base_user_role runs standalone.

Fix applied to all role assignment test methods to maintain consistent
behavior across test scenarios.

@ng-ife
Copy link

ng-ife commented Jan 24, 2025

Hello @pedrobaeza
Can you review this PR? Thank you

@pedrobaeza pedrobaeza added this to the 17.0 milestone Jan 24, 2025
@pedrobaeza
Copy link
Member

I can't say about this one.

@ng-ife
Copy link

ng-ife commented Jan 24, 2025

@pedrobaeza Do you know how to ping? You made the "mistake" to help me once at last OCA days but I'm more then willing to spread my pings.

@pedrobaeza
Copy link
Member

You can ping module maintainers. Formally, the PR shouldn't change the module version (it's done by the bot), and that way, you clean it without the README changes.

@mb-ife mb-ife force-pushed the 3015-17.0-base_user_role_fix_tests branch 2 times, most recently from 60cb771 to 43458cb Compare January 24, 2025 16:05
@sebalix
Copy link
Contributor

sebalix commented Jan 28, 2025

There are fixes which have been done for other Odoo versions, and we should probably try to port them first (the use of sorted for instance has been fixed in 15.0 by using set). I'm trying to port some of the PRs done in 15.0 to 17.0 first:

@sebalix
Copy link
Contributor

sebalix commented Jan 28, 2025

@mb-ife @ng-ife , can you check if the mentioned PR above fixed your issues? And/or adapt your PR if it remains fixes to apply

@mb-ife
Copy link
Author

mb-ife commented Jan 28, 2025

Hello @sebalix ,
Thanks for the updates , we have issues mainly with testing we'll double check again and give you feedback.

@sebalix
Copy link
Contributor

sebalix commented Jan 28, 2025

@mb-ife mb-ife force-pushed the 3015-17.0-base_user_role_fix_tests branch 2 times, most recently from afea8cd to 1926428 Compare January 28, 2025 16:03
…tegration tests

Add force group recalculation in role assignment tests to handle
group synchronization issues that occur when base_user_role and
auditlog modules are both installed. Without forcing updates, tests
fail in odoo.sh and OCA CI environments due to group assignment
mismatches.

Force parameter in set_groups_from_roles() ensures proper group
synchronization regardless of auditlog's presence. No issues occur
when base_user_role runs standalone.

Fix applied to all role assignment test methods to maintain consistent
behavior across test scenarios.
@mb-ife mb-ife force-pushed the 3015-17.0-base_user_role_fix_tests branch from 1926428 to 7b6a642 Compare January 28, 2025 17:16
@mb-ife
Copy link
Author

mb-ife commented Jan 28, 2025

Hello @sebalix

After checking the new version it solve for us this issue here with the sorted groups #327 , but still have an issue when combined with auditlog module #321 , the current PR is currently solving this at the moment.

@sebalix
Copy link
Contributor

sebalix commented Jan 29, 2025

@mb-ife Ok, thanks for the feedback. What I do not like the way it is fixed here, is that it doesn't really test the feature as it should work: we update the roles on the user, and the call to set_groups_from_roles is implicit (we should not have to call it manually in tests) and we should get the expected results.

So it's like hidding the issue, without explaining why it behaves this way: we probably have something else to fix in the design (in auditlog or base_user_role, or if not, a comment explaining the reason why we are taking this shortcut).

@StefanRijnhart
Copy link
Member

Indeed, if this is an issue in combination with auditlog, it's the issue that needs fixing, not the test. Can you tell us how to reproduce the issue?

@mb-ife
Copy link
Author

mb-ife commented Feb 19, 2025

Hello @sebalix, @StefanRijnhart

After investigating the test failure, I realized that simply adding a manual call to set_groups_from_roles(force=True) is not an adequate solution to the underlying issue. When the auditlog and base_user_role modules are installed together, the automatic group assignment for roles does not work as expected.

To reproduce we only need to run a test with base_user_role and auditlog on the same folder. This is happening on Odoo 16 as well, and you can see a similar issue here #321. The error is triggered on OCA CI as well as on Odoo.sh.

The easiest approach seems to be forcing the groups update in base_user_role. I would appreciate a second opinion on this approach.

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.

6 participants