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

[14.0][OU-UPD] rerun analysis for noupdate_changes.xml #4358

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

remi-filament
Copy link
Contributor

@remi-filament remi-filament commented Mar 25, 2024

As per #4356 multi-company security rules have changed.
This PR is the outcome of rerunning analysis, then checking manually each file to keep only what is relevant (and not overwriting noupdate_changes.xml which were manually modified while writing migration scripts)

@OCA-git-bot
Copy link
Contributor

Hi @StefanRijnhart, @MiquelRForgeFlow, @pedrobaeza,
some modules you are maintaining are being modified, check this out!

@remi-filament remi-filament force-pushed the 14.0-ou-upd-rerun-analysis branch 2 times, most recently from fb2a209 to 19bb70c Compare March 27, 2024 08:48
@remi-filament remi-filament marked this pull request as ready for review March 27, 2024 08:49
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Thanks !

@remi-filament
Copy link
Contributor Author

Thank @MiquelRForgeFlow for review, I indeed missed a few points ! All resolved

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM

@legalsylvain
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-4358-by-legalsylvain-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit cdbf530 into OCA:14.0 Mar 27, 2024
2 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza
Copy link
Member

Thanks for this, Rémi. Remember that UPD is not a valid tag. It should be IMP or FIX. I think that something similar should be done for v15, as there will be some new company rules there. And maybe it's interesting to force the rewriting unconditionally for those coming from a v14 with old rules.

@remi-filament remi-filament deleted the 14.0-ou-upd-rerun-analysis branch March 28, 2024 10:55
@remi-filament
Copy link
Contributor Author

And maybe it's interesting to force the rewriting unconditionally for those coming from a v14 with old rules.

I tried asking Odoo to get update script directly in v14 core, but they answered that there is no need, they would get it when upgrading to next version...
I am not sure what would be the best way-forward for existing DB in v14+ :

  1. should we enforce in OpenUpgrade in v15, v16, v17 these noupdate changes for all ir_rules ?
  2. should we create a script that could be run directly in existing DB outside of a migration and that could be called by OpenUpgrade (or listed in the doc as prerequisite) when upgrading to v15 / v16 / v17 ?

Solution 1 is probably cleaner, but solution 2 would allow to get performance improvements for existing DB without waiting for migration... (also I am not sure how to make solution 2 since we would need to check for each module if module is installed before updating rules)

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