[15.0][FIX] upgrade_analysis: misleading representation of read/write computed fields #2416
Conversation
|
Hi @legalsylvain, |
|
For new fields,I think it's interesting to know that they are compute / stored (like a default). For existing fields, not sure if being put if any change of a field may trigger a recomputation of this field. |
|
@pedrobaeza that implies you'd want to see the |
|
Not really. It's like the default. When a new field is present, and it has a default, we put in the analysis file that it has default. You go to the definition to see which default is and if it serves. The same can apply to the computed writable: it can put: has compute, and if you want to see which value it will take (or want to disable as very heavy), you see the code and decide, but not having this information will make an immediate "Nothing to do", where it's not. |
|
@pedrobaeza not sure I understand, but let's recap. Before this PR, it is reported for a It is also reported if a field that did not have a default has a default in the new version or the other way around (as per OCA/OpenUpgrade@4e60a82, discussion in OCA/OpenUpgrade#1204 ). This is key What this PR changes, is that for a I think the discussion in #1204 was not very clear. Instead, I do think it is interesting to see for
Is this in line with what you had in mind? -- edit -- actually, the default for required fields was already reported for existing and new fields, so we'll keep it that way. |
StefanRijnhart
force-pushed
the
fix/15.0/openupgrade_analysis/compute_readwrite
branch
4 times, most recently
from
8186713 to
c35b563
Compare
…ted fields Odoo 14 introduced the widescale usage of computed fields with readonly=False. In that case, the compute method functions as a default that can also be used to compute a value some time *after* the initial creation of the record. In the OpenUpgrade analysis files, these fields would be misrepresented as computed fields rather than fields with a default function. This change fixes that.
StefanRijnhart
force-pushed
the
fix/15.0/openupgrade_analysis/compute_readwrite
branch
from
c35b563 to
a01c6d4
Compare
|
@pedrobaeza my refactoring is done. The 'default' key is now merged into the 'hasdefault' key and it can have two values: The information is not shown for deleted fields anymore as I think this is useless noise, and as for changes with existing fields in the previous version, this was not shown and it is still not shown as I still think this information is only useful for new fields. I've already included this change in the initialization of 16.0 (OCA/OpenUpgrade#3583), but you can inspect the change in the analysis more easily in the 15.0 analysis update here: OCA/OpenUpgrade#3587. Let me know if we are on the same line. |
MiquelRForgeFlow
left a comment
MiquelRForgeFlow
left a comment
There was a problem hiding this comment.
Is it retro-compatible with previous versions, right?
|
Checked the result on OU: /ocabot merge major |
|
On my way to merge this fine PR! |
|
@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-2416-by-pedrobaeza-bump-major. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
It seems there's a problem with @NL66278 you are the migrator to this version. Can you please check? Maybe we should disable tests for now? |
|
@pedrobaeza I did a mostly technical migration, not being really an expert on the intricacies of letsencrypt. As far as I can see it is the acme client complaining about a generic domain that the certificates in the request are requested for. I have some hesitancy just disabling the tests... Although that would maybe be the most easy option. |
|
Let's try again to see if it's a transient question: /ocabot merge major |
|
This PR looks fantastic, let's merge it! |
|
@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-2416-by-pedrobaeza-bump-major. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
Maybe the acme library version has to be restricted 🤔 |
|
@MiquelRForgeFlow it is not entirely backwards compatible, in the sense that we need one more backport: #2431 |
|
This fixes the letsencrypt tests, for now: #2432 |
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 0cad19c. Thanks a lot for contributing to OCA. ❤️ |
5 participants
Odoo 14 introduced the widescale usage of computed fields with readonly=False. In that case, the compute method functions as a record specfic default method that can also be used to compute a value some time after the initial creation of the record.
In the OpenUpgrade analysis files, these fields would be misrepresented as computed fields rather than fields with a default function. This change fixes that. See OCA/OpenUpgrade#3587 for the result.