Skip to content

[18.0][FIX] upgrade_analysis: avoid caring about non stored fields #3290

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

Merged

Conversation

MiquelRForgeFlow
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow commented May 20, 2025

Long time ago, I added OCA/OpenUpgrade@b724b4f in v12. I did it because Odoo started to add computed/related stored fields in v13. But Odoo in last versions has been adding non stored fields that are not compute nor related, like https://github.com/odoo/odoo/blob/18.0/addons/account/models/account_account.py#L142 and https://github.com/odoo/odoo/blob/18.0/addons/account/models/account_move.py#L186. This kind of field adds noise in the analysis file, so let's remove them.

NOTE: Usual company_dependent fields and usual translate fields are marked as stored, so no problem with these cases.

@OCA-git-bot
Copy link
Contributor

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

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.

Makes sense !

Thanks !

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.

I don't know. For me as a developer, reading about these fields do add to my understanding of the way that Odoo develops. Would it be an option to update the analysis to add unstored for NEW or DEL entries of unstored fields?

@MiquelRForgeFlow
Copy link
Contributor Author

I don't know. For me as a developer, reading about these fields do add to my understanding of the way that Odoo develops. Would it be an option to update the analysis to add unstored for NEW or DEL entries of unstored fields?

Do you mean an option in the form view (a new field) to enable unstored? Yeah, but it will be unfilled by default and will not be used for the analysis files in OU. Are you ok with this?

@StefanRijnhart
Copy link
Member

No, I mean that it says in the analysis something like

account      / account.account          / display_mapping_tab (boolean) : NEW hasdefault: default unstored

Instead of

account      / account.account          / display_mapping_tab (boolean) : NEW hasdefault: default

That way the new field still shows up in the analysis, but it is immediately clear to the developer of the scripts that nothing needs to be done.

@legalsylvain
Copy link
Contributor

I don't know. For me as a developer, reading about these fields do add to my understanding of the way that Odoo develops. Would it be an option to update the analysis to add unstored for NEW or DEL entries of unstored fields?

At a first sight, i'm not agree to add such extra change.
For me, the objective of the analysis file is to enumerate changes that could require openupgrade scripts. If we are sure that a change will never ask a script, i think it should not be added.

I understand your need as a développer, but there are other interesting changes (like changes in functions names / args) that are not listed for the same reason : no action required.

If we décide to add this line, we could add an extra line juste after : Nothing to do : non stored fiield. So we'll save time for openupgraders.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 18.0-fix-upgrade_analysis-non-stored branch 3 times, most recently from acb9794 to f989a15 Compare May 21, 2025 09:09
@MiquelRForgeFlow
Copy link
Contributor Author

@pedrobaeza opinions?

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.

Approving this version, as it implements my suggestion. Thanks!

@pedrobaeza pedrobaeza added this to the 18.0 milestone May 21, 2025
@pedrobaeza
Copy link
Member

The code doesn't highlight the results in the file. Can you please show an example?

@MiquelRForgeFlow
Copy link
Contributor Author

Example:

account      / account.account          / display_mapping_tab (boolean) : NEW hasdefault: default

will be changed to

account      / account.account          / display_mapping_tab (boolean) : NEW hasdefault: default, stored: False

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 18.0-fix-upgrade_analysis-non-stored branch from f989a15 to afa5370 Compare May 21, 2025 14:10
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 18.0-fix-upgrade_analysis-non-stored branch from afa5370 to cd01a7b Compare May 21, 2025 14:11
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I suppose this will require an adaptation when migrated to 19.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit edf9e33 into OCA:18.0 May 21, 2025
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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