Skip to content

[18.0][MIG] base_user_role_history: Migration to 18.0. #338

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

Open
wants to merge 15 commits into
base: 18.0
Choose a base branch
from

Conversation

mcodes-o
Copy link

Migrated to 18.0

@mcodes-o mcodes-o mentioned this pull request Jan 28, 2025
6 tasks
@mcodes-o
Copy link
Author

@ThomasBinsfeld Since you're listed as a maintainer, do you have time to look at this PR? Thanks!

Copy link

@JessBrandl JessBrandl left a comment

Choose a reason for hiding this comment

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

Code LGMT, tested in runboat and works as intended

@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). 🤖

self.ensure_one()
domain = [("user_id", "=", self.id)]
return {
"name": _("Roles history"),
Copy link

@CRogos CRogos Mar 18, 2025

Choose a reason for hiding this comment

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

You can replace the call to translate a string ( _ ) with self.env._ for getting some performance improvement in some cases. See odoo/odoo#174844.

Copy link
Author

Choose a reason for hiding this comment

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

@CRogos Thanks, has been fixed in commit f59c810

Copy link

Choose a reason for hiding this comment

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

Can you squash this together with the "Migration to 18.0" commit?

Copy link

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

LGTM, but some small findings:

Squash administrative commits (if any) with the previous commit for reducing commit noise. Check https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate for details.

image

@mcodes-o mcodes-o force-pushed the 18.0-mig-base_user_role_history branch from 3ac3b98 to f59c810 Compare April 6, 2025 10:24
@mcodes-o
Copy link
Author

mcodes-o commented Apr 6, 2025

LGTM, but some small findings:

Squash administrative commits (if any) with the previous commit for reducing commit noise. Check https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate for details.

@CRogos I've squashed the commits and pushed. Thanks for the review!

@CRogos
Copy link

CRogos commented Apr 10, 2025

@mcodes-o these need to be squashed too:
image

Copy link

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

@mcodes-o could you merge the last two commits?



# DEPRECATED: This tests are deprecated but stay to show that the new code is working.
class TestBaseUserRoleHistory(TransactionCase):
Copy link

@CRogos CRogos Apr 22, 2025

Choose a reason for hiding this comment

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

Use BaseCommon as base test class to reduce overhead from tracking and boost the test suite.

Maybe also check if the depricated tests can be removed?

@CRogos
Copy link

CRogos commented May 21, 2025

@mcodes-o are you still working on this?

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.

10 participants