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

[12.0][REF] extract delivery_carrier_info from base_delivery_carrier_label #283

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

sebastienbeau
Copy link
Member

@sebastienbeau sebastienbeau commented Aug 30, 2020

In e-commerce case you need some extract info on carrier (code, description) installing the module base_delivery_carrier_label is too much for having this two field, so I have split the module to reduce the dependency

@florian-dacosta @bealdav @guewen

Merge with nobump I have updated the version manually

@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 31, 2020

Hello, it's me again for bothering you 😝

This refactoring is good, but this breaks existing installation that don't include the new module in their addons definition. For avoiding this problem, you can:

  • Create the new module as it's done.
  • Keep the fields in base_delivery_carrier_label and don't add the new dependency.
  • Load dynamically through fields_view_get the fields if not found.
  • Add a TODO in the code for that lines, stating the new dependency for v13 and the removal of the fields.

What do you think about this proposal?

@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 31, 2020 via email

@bealdav
Copy link
Member

bealdav commented Aug 31, 2020

Hi @pedrobaeza as you know when anybody update a branch to a recent commit, you may need to make some adjustments on your installation. Then, each conscientious integrator'll check that before to put in production (sh has a staging branch too).
In this case, it only consists to add a module to your addons path (which is in the same repo). It's a normal task compatible with the OCA policy which allow to make substantial changes inside a version. Many changes in OCA are more substantial than this one.
It seems to me quite acceptable to release this module as it.

@pedrobaeza
Copy link
Member

Well, it's not the same changing data structure, than can be adjusted through migration scripts, than changing dependencies. For me that's the red line, but if others don't think so, go ahead.

@sebastienbeau
Copy link
Member Author

After playing a little, I do not think it a major issue as the split is in the same commit and same repo.

Indeed if you deploy on odoo.sh them the whole repo will be updated (they clone the branch)
If you deploy with pip the dependency will be downloaded.
If you deploy with doodba I think you also download the repo, so an update will download the missing module.

Then if you run a update

  • odoo -u base_delivery_carrier_label
  • click-odoo-update
  • manual update on the interface

The module delivery_carrier_info will be installed

The only issue you can have is if you download the module on apps odoo and some copy paste, but in that case if you do it manually you should manually take care of the dependency

What do you think?

@pedrobaeza
Copy link
Member

Thanks for the extensive investigation. On doodba we restrict the available addons through addons.yaml for not allowing to install undesired addons, but it's OK as we only have this module in one instance and we have CI for detecting it. If Odoo.sh is not a problem as well, go ahead.

@bealdav
Copy link
Member

bealdav commented Aug 31, 2020

Thanks for your pragmatism @pedrobaeza

@sebastienbeau
Copy link
Member Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-283-by-sebastienbeau-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit fb602bc into OCA:12.0 Aug 31, 2020
@OCA-git-bot
Copy link
Contributor

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

@len-foss
Copy link

Just a little update: Pedro's warning was justified as this PR did break Pip installs.
The addon https://pypi.org/project/odoo12-addon-delivery-carrier-info/ was just released 12 hours ago thanks to #321.
(and thanks to Denis who quickly understood the problem).

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.

7 participants