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.

Sign up for GitHub

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

Jump to bottom

[17.0][MIG] delivery_carrier_multi_zip: Migration to 17.0 #990

Open
wants to merge 5 commits into
base: 17.0
Choose a base branch
from
Open

[17.0][MIG] delivery_carrier_multi_zip: Migration to 17.0 #990

wants to merge 5 commits into from

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Feb 20, 2025 •
edited
Loading

Migration to 17.0

Please @pedrobaeza can you review it?

@Tecnativa TT52409

pedrobaeza and others added 4 commits February 19, 2025 16:22
@pedrobaeza @victoralmau
[ADD] delivery_carrier_multi_zip: Multiple ZIP intervals for the same…
7410fb8 
… delivery method

This module allows to set more than one interval of zip codes for matching each
shipping method.

TT35326

[UPD] Update delivery_carrier_multi_zip.pot

delivery_carrier_multi_zip 13.0.1.0.1

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: delivery-carrier-13.0/delivery-carrier-13.0-delivery_carrier_multi_zip
Translate-URL: https://translation.odoo-community.org/projects/delivery-carrier-13-0/delivery-carrier-13-0-delivery_carrier_multi_zip/
@rizzel @victoralmau
[MIG] delivery_carrier_multi_zip: Migration to 14.0
f20c909 
[UPD] Update delivery_carrier_multi_zip.pot

[UPD] README.rst
@victoralmau
[MIG] delivery_carrier_multi_zip: Migration to 15.0
d48e308 
TT40224

[UPD] Update delivery_carrier_multi_zip.pot

[UPD] README.rst

Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: delivery-carrier-15.0/delivery-carrier-15.0-delivery_carrier_multi_zip
Translate-URL: https://translation.odoo-community.org/projects/delivery-carrier-15-0/delivery-carrier-15-0-delivery_carrier_multi_zip/

[UPD] README.rst
@victoralmau
[IMP] delivery_carrier_multi_zip: pre-commit auto fixes
a5c8296
@victoralmau
Copy link
Member Author

IMO you should remove the post_init_hook method and try to convert the prefixes (zip_prefix_ids) to delivery.carrier.zip values because it is not possible. What do you think @pedrobaeza ?

@victoralmau victoralmau mentioned this pull request Feb 20, 2025
Closed
@pedrobaeza pedrobaeza added this to the 17.0 milestone Feb 20, 2025
@pedrobaeza
Copy link
Member

The hook doesn't make sense now in this context, and I don't think we should do any kind of conversion of the prefixes. Add a new selection option for choosing if this is going to be determined by prefixes or ZIP ranges, with the default in prefixes, and that's all.

@victoralmau victoralmau force-pushed the 17.0-mig-delivery_carrier_multi_zip branch from 88b940f to 0488fcc Compare February 20, 2025 16:27
pedrobaeza
pedrobaeza reviewed Feb 20, 2025
View reviewed changes
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 think you don't need to depend on stock_delivery, or the method _match_address is there?

@victoralmau victoralmau force-pushed the 17.0-mig-delivery_carrier_multi_zip branch from 0488fcc to c1ef07e Compare February 21, 2025 07:44
@victoralmau
Copy link
Member Author

The _match_address() method is in delivery, but the ir.model.access records that are set for delivery.carrier.zip are linked to stock (stock.group_stock_user and stock.group_stock_manager), so the stock_delivery dependency is necessary.

This same (or similar) explanation applies to other delivery_carrier modules in this repository in 17.0.

@rousseldenis
Copy link
Contributor

/ocabot migration delivery_carrier_multi_zip

@OCA-git-bot OCA-git-bot mentioned this pull request Feb 21, 2025
Open
33 tasks
rousseldenis
rousseldenis reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

@victoralmau It seems this misses DESCRIPTION.rst as section is removed from README

@victoralmau
Copy link
Member Author

@victoralmau It seems this misses DESCRIPTION.rst as section is removed from README

I'm sorry but I don't understand what you are saying, the readme/DESCRIPTION.md file exists (as do others) and the README.rst file is automatically generated with the contents of the readme folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousseldenis rousseldenis rousseldenis left review comments

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
migration needs review
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@victoralmau @pedrobaeza @rousseldenis @rizzel