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

[MIG] base_view_inheritance_extension: Migration to 16.0 #2494

Merged
merged 42 commits into from
Dec 7, 2022

Conversation

etobella
Copy link
Member

@etobella etobella commented Dec 6, 2022

Migration to 16.0

@legalsylvain

hbrunn and others added 30 commits December 6, 2022 20:24
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
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: server-tools-13.0/server-tools-13.0-base_view_inheritance_extension
Translate-URL: https://translation.odoo-community.org/projects/server-tools-13-0/server-tools-13-0-base_view_inheritance_extension/
* Removed `list_add` and `list_remove`, they've been deprecated and
implemented in Odoo core since several versions ago.

* Removed `move`, as it has also already been implemented in core several
versions ago.

* Replaced `python_dict` by `update`, that performs an operation similar
to :meth:`dict.update`, but on the ast.Dict.
sebalix and others added 4 commits December 6, 2022 20:24
@rven
Copy link
Contributor

rven commented Dec 6, 2022

Superseeded by #2495

It was not just enough to do the upgrade like @legalsylvain mentioned on Twitter.
The attributes where perfectly changed, but the demo view was not showing stuff correctly when creating a parent from the simple contact form.
I've changed the fields so it reflects them correctly when verifying it in the application.

@etobella
Copy link
Member Author

etobella commented Dec 6, 2022

@rven There is no issue in superseeding my PR, but I think some things must be followed before doing it:

  1. When we superseed a PR, we are expecting an answer from the author and he does not answer.
  2. We ask the author for supersseding and let him/her some time to answer (usually a week, but at least a day)
  3. On a superseeding PRs, we keep original PR history and commits

So, if you want to superseed this PR, follow at least the last point, as there is no problem from my side in doing the superseed.

@etobella
Copy link
Member Author

etobella commented Dec 6, 2022

Meanwhile, I added your change 😉

@etobella
Copy link
Member Author

etobella commented Dec 6, 2022

/ocabot migration base_view_inheritance_extension

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Dec 6, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Dec 6, 2022
33 tasks
@legalsylvain
Copy link
Contributor

It was not just enough to do the upgrade like @legalsylvain mentioned on Twitter.

? Which tweet ?

@rven
Copy link
Contributor

rven commented Dec 7, 2022

It was not just enough to do the upgrade like @legalsylvain mentioned on Twitter.

? Which tweet ?

It wasn't yours it was https://twitter.com/alexisdlattre/status/1600095838700707840 :-)
I read it wrong, he didn't do any changes at all...
This happens when in the meantime watch football ;-)

@rven
Copy link
Contributor

rven commented Dec 7, 2022

@rven There is no issue in superseeding my PR, but I think some things must be followed before doing it:

  1. When we superseed a PR, we are expecting an answer from the author and he does not answer.
  2. We ask the author for supersseding and let him/her some time to answer (usually a week, but at least a day)
  3. On a superseeding PRs, we keep original PR history and commits

So, if you want to superseed this PR, follow at least the last point, as there is no problem from my side in doing the superseed.

@etobella It was just a timing issue of a couple minutes I guess, we both were working on the same migration at the same time :-)
I wasn't aware of the guidelines you mention. I only superseded your PR because mine was only completer (!= better) ;-).
I only superseded yours because it also happened to one of my PR's in the past, with the same reason.
Maybe it's a good thing to take notice of the superseding guidelines in the OCA guidelines?

When I started on the migration, there was no PR, no mentioning on the issue that someone was working on the migration as mentioned in the guidelines.

For me it's fine to leave this PR open, I'l close mine and approve yours, because I also verified it functionally 💯

The only thing I wan't is a happy community :-p

Copy link
Contributor

@rven rven left a comment

Choose a reason for hiding this comment

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

Working fine!

Reviewed code and functionally tested 👍

@etobella
Copy link
Member Author

etobella commented Dec 7, 2022

I know it happened because we where too fast, for that reason I wasn't angry at all. It happens sometimes to everyone, don't worry. About the changes from your PR thanks for them 😁

@etobella
Copy link
Member Author

etobella commented Dec 7, 2022

It was not just enough to do the upgrade like @legalsylvain mentioned on Twitter.

? Which tweet ?

My fault, I confused github users, it was a tweet by @alexis-via 🥲

@pedrobaeza
Copy link
Member

Please propose at https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#github a section about how to act for doing pull requests and this protocol about superseding.

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Pure code review: LG

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit ee2b883 into OCA:16.0 Dec 7, 2022
@OCA-git-bot
Copy link
Contributor

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

@etobella etobella deleted the 16.0-mig-base_view branch December 7, 2022 08:03
@alexis-via
Copy link
Contributor

Thank you very much for this migration.

In fact, I had done myself the same kind of "easy" migration of base_view_inheritance_extension with just an update of tests, but I had some crash with my port of account_invoice_transmit_method which depend on base_view_inheritance_extension, and I thought that the crash was caused by base_view_inheritance_extension v16. I eventually figured out that the crash was caused by a missing coma in a domain in a view of account_invoice_transmit_method !!! But the backtrace was so horrible that I thought the bug was in some low-level code like the code of base_view_inheritance_extension.

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.