Skip to content

[18.0][FIX] base_user_role_company: Roles are filtered so none which exceed date_to are used#347

Merged
OCA-git-bot merged 1 commit intoOCA:18.0from
DynAppsNV:18.0-fix-base_user_role_company
May 18, 2025
Merged

[18.0][FIX] base_user_role_company: Roles are filtered so none which exceed date_to are used#347
OCA-git-bot merged 1 commit intoOCA:18.0from
DynAppsNV:18.0-fix-base_user_role_company

Conversation

@TomVermeerenDynapps
Copy link
Copy Markdown
Contributor

Fixed a bug where disabled roles (because past date_to) were used.

Copy link
Copy Markdown
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.

Maybe use this convenience method: https://github.com/OCA/server-backend/blob/14.0/base_user_role/models/user.py#L54-L55
Can you add a small test?

@TomVermeerenDynapps
Copy link
Copy Markdown
Contributor Author

@StefanRijnhart _get_enabled_roles is the method I am overwriting, so instead of calling it (recursion error) again I will use res, this has the same functionality. I also added test scenarios.

Copy link
Copy Markdown
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.

Great thanks! Only some minor comments now.

Comment thread base_user_role_company/tests/test_use_only_enabled_roles.py Outdated
Comment thread base_user_role_company/tests/test_use_only_enabled_roles.py Outdated
Comment thread base_user_role_company/tests/test_use_only_enabled_roles.py Outdated
Copy link
Copy Markdown
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 for the update! Please squash the commits.

@TomVermeerenDynapps TomVermeerenDynapps force-pushed the 18.0-fix-base_user_role_company branch from 3adfa8b to e73df67 Compare May 6, 2025 11:25
@TomVermeerenDynapps
Copy link
Copy Markdown
Contributor Author

Comments resolved & commits squashed! Thank you for review!

Copy link
Copy Markdown
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.

Apologies, one thing I missed: the module name needs to be featured in the commit message, so [FIX] base_user_role_company: ... (like the PR title actually).

@TomVermeerenDynapps TomVermeerenDynapps force-pushed the 18.0-fix-base_user_role_company branch from e73df67 to 8d7fc9a Compare May 6, 2025 11:48
@TomVermeerenDynapps TomVermeerenDynapps force-pushed the 18.0-fix-base_user_role_company branch from 8d7fc9a to 75348ff Compare May 6, 2025 11:52
@TomVermeerenDynapps
Copy link
Copy Markdown
Contributor Author

Should be resolved now:) thank you!

Copy link
Copy Markdown
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 for the quick responses!

@StefanRijnhart StefanRijnhart changed the title [FIX] base_user_role_company: Roles are filtered so none which exceed date_to are used [18.0][FIX] base_user_role_company: Roles are filtered so none which exceed date_to are used May 6, 2025
@OCA-git-bot
Copy link
Copy Markdown
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). 🤖

@dreispt
Copy link
Copy Markdown
Member

dreispt commented May 18, 2025

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 18.0-ocabot-merge-pr-347-by-dreispt-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 6db1dc7 into OCA:18.0 May 18, 2025
7 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 4940e8c. 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