-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[12.0][MIG] base_view_inheritance_extension: Migration to 12.0 #1529
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
[12.0][MIG] base_view_inheritance_extension: Migration to 12.0 #1529
Conversation
Trivial changes: * Version in README changed * Version in manifest changed OCA Transbot updated translations from Transifex OCA Transbot updated translations from Transifex
…extension. (OCA#804) * Add list_add operation. * Add list_remove operation. OCA Transbot updated translations from Transifex
expression not match the node any more. Fixes OCA#885
* warning about dynamic context * import for safe_eval
|
It seems you forgot the update of the version number. |
578d34c to
d3e9534
Compare
|
@alexis-via sorry. I now amended the commit with the right version |
d3e9534 to
61a83bb
Compare
|
@alexis-via sorry, I did it wrong as I was in a meeting with Renato. I should not have changed the version in the Transiflex pot commit, so I undo that and I added a new commit for it 61a83bb |
|
v12 has its own version of It seems to me both can coexist, and I find the way they have chosen a little bit clumsy, but it serves the same purpose. Should we drop support for |
|
@hbrunn I would people encourage to use the standard Odoo way as much as possible. So keep the position="move" for now, but clearly mark/document it as deprecated. |
|
But are they going to conflict having the same name? |
|
no, because for Odoo standard, the attribute is supposed to be added to a child of the element you match, in our version it's on the element you match |
|
Pretty confusing, isn't it? I prefer to remove it... |
|
Any decision here ? |
|
I would prefer to use odoo core behaviour, but just my opinion. nothing against this feature, but to have both will be quite confusing. |
|
I would prefer the position="move" removed from this module. Just a preference, not blocking for this. |
|
@rvalyi, what conclusion do we get here? What can we do to conclude with this migration? |
|
note that since version 9, also the |
Yes, i agree with you @hbrunn 👌 |
|
Hi, when will you merge this? A lot of module depends from this. |
| @@ -0,0 +1,2 @@ | |||
| # License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make license lines https:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
NL66278
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that all the methods that are now in standard Odoo should be removed. Either completely, or at least by giving a Deprecated message when still used (to be turned into an Exception in 13.0).
|
Hello folks, I dont really use this module myself. So if somebody wants to redo the migration in another PR/another way please feel free to go ahead as I cannot dedicate time to this one these days unfortunately... |
|
I continued the work of @rvalyi and removed the features which are now native. The readme of the module has been updated. Tests works fine. I also tested it with my PR OCA/account-invoicing#570 which makes use of this module in v12 and it works well. So it's ready to merge now. |
bealdav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
4a230fd to
61a83bb
Compare
Use https in URL to licence
|
As said by Pedro in the PR for v13, the list_add and list_remove feature is NOT native. So I cancelled my previous commit and push a new commit with only the removal of the move feature. |
|
/ocabot merge |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at d4a489f. Thanks a lot for contributing to OCA. ❤️ |
Syncing from upstream OCA/server-tools (16.0)
standard migration: nothing to be done: same code as version 11 (except version number)
cc @hbrunn @NL66278 @pedrobaeza @sergio-teruel @alexis-via