-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
[18.0] [MIG] delivery_postlogistics: Migration to 18.0 #940
[18.0] [MIG] delivery_postlogistics: Migration to 18.0 #940
Conversation
* fix 'PostLogistics' terms * make 'Package Code' required for PostLogistics delivery packaging * show a link listing available package codes for PostLogistics delivery packaging * update README
Some chars are disallowed from postlogistics API ("|", "", "<", ">") and output json should be sanitized
history_ids has been removed as below link. So the code is obsoleted. odoo/odoo@b3180c8#diff-9a22420c4a72cc9b6d558b3d832846c2e44a4f3731c6df9579d66c6279c4804eL54-L57 _get_origin_pickings is to determine number of pickings of a sale order. So it should get pickings from sale_id of found picking.
The cassette `tests/fixtures/cassettes/test_store_label.yaml` should be updated as the label was failed to decode in both 13.0 and 14.0. * https://github.com/odoo/odoo/blob/13.0/odoo/addons/base/models/ir_attachment.py#L212 * https://github.com/odoo/odoo/blob/14.0/odoo/addons/base/models/ir_attachment.py#L554 We can remove the method _selection_file_type with below reasons. * It is used in base_delivery_carrier_label but was removed on Sep 5, 2019 https://github.com/OCA/delivery-carrier/blame/12.0/base_delivery_carrier_label/models/shipping_label.py#L15 * PR migration to 13.0 of base_delivery_carrier_label was merged on Oct 22, 2020 https://github.com/OCA/delivery-carrier/pull/251/files * Dependency on base_delivery_carrier_label was removed in migration to 14.0 on Apr 14 2021 OCA@4ba2b56#diff-f16078e35000c05a9b63f5e7e04b6f5fd50eadae8b14a45aa9c1eb078031060cL12 OCA@4ba2b56#diff-08c11b571d9c0b0c509c3922c77f407ef4c429473fda9bc2d4d542520e7a4e3fR12
… for delivery type other than postlogistics.
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: delivery-carrier-16.0/delivery-carrier-16.0-delivery_postlogistics Translate-URL: https://translation.odoo-community.org/projects/delivery-carrier-16-0/delivery-carrier-16-0-delivery_postlogistics/
054d771
to
f9389fe
Compare
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.
Thanks @bizzappdev !
Minor comments here and there
Please note there's a "WIP" commit, that possibly needs squashing
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.
Thank you for this migration work in progress 🙏
Please find below some suggestions
Squash administrative commits (if any) with the previous commit for reducing commit noise. Check https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate for details.
from odoo.tools.translate import LazyTranslate | ||
|
||
_lt = LazyTranslate(__name__, default_lang="en_US") |
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.
why is LazyTranslate
needed? (I'm not familiar with its usage)
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.
When the default language is unavailable and a log with a traceback is generated, test cases fail. This issue arises because the translation system expects a valid language context, which may not always be present.
https://github.com/odoo/odoo/blob/18.0/odoo/tools/translate.py#L587
In this particular case, the PostlogisticsWebService
class is not an Odoo class, which could explain why it lacks Odoo’s built-in translation functionality. In the Odoo codebase, LazyTranslate has been used in similar scenarios to avoid such issues:
https://github.com/odoo/odoo/blob/b2e564f07f6883111da4581bccfb0f45c7588063/odoo/tools/image.py#L88
Additionally, I was not familiar with self.env._, but it would face the same issue here since self
does not have an env
in this context.
"Fixed delivery date", help="Specific delivery date (ZAW3217)" | ||
) | ||
|
||
# TODO: consider refactoring these fields using a partner relation instead |
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.
could this migration be the opportunity to take care of this?
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.
This migration MUST be the opportunity to do so and will. I agreed.
return packages | ||
|
||
def get_shipping_label_values(self, label): | ||
# TODO: consider to depends on base_delivery_carrier_label |
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.
could this migration be the opportunity to take care of this?
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.
This migration MUST be the opportunity to do so and will. I agreed.
"""Pickings using this module must have a package | ||
If not this method put it one silently | ||
""" | ||
# TODO: consider to depends on base_delivery_carrier_label |
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.
could this migration be the opportunity to take care of this?
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.
This migration MUST be the opportunity to do so and will. I agreed.
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.
Remove references in CREDITS.rst of past financed migrations by other companies
dea3dd1
to
7f0e7a9
Compare
@vvrossem @ivantodorovich Changes are done. Can you please review it again? |
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.
Thank you and please add yourself in readme/CONTRIBUTORS.md
🙏
7f0e7a9
to
aae5629
Compare
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.
Thanks for your contribution.
We need to shrink the functional aspect of this module by reusing some of the already exsiting standard objects to manage options. By doing this we could manage these options association explicitly (1 object instead of 3 - i.e. a single A4/300pp/JPG paper format record). The tricky part is to migrate all previously existing data (defining unicity constraint to know when creating a new configuration or reusing it)
"license": "AGPL-3", | ||
"category": "Delivery", | ||
"complexity": "normal", | ||
"depends": ["stock_delivery"], |
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.
"depends": ["stock_delivery"], | |
"depends": [ | |
"stock_delivery", | |
"base_delivery_carrier_label", | |
"delivery_carrier_shipping_label" | |
], |
Please, we need to be closer to the future label management. (Several other delivery carriers implementation will benefits from it)
# Copyright 2013-2016 Camptocamp SA | ||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
||
from odoo import fields, models | ||
|
||
|
||
class PostlogisticsShippingLabel(models.Model): | ||
_name = "postlogistics.shipping.label" | ||
_inherits = {"ir.attachment": "attachment_id"} | ||
_description = "Shipping Label for PostLogistics" | ||
|
||
file_type = fields.Char(string="File type", default="pdf") | ||
attachment_id = fields.Many2one( | ||
comodel_name="ir.attachment", | ||
string="Attachement", | ||
required=True, | ||
ondelete="cascade", | ||
) |
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.
Replaced by "shipping.label" from "delivery_carrier_shipping_label"
postlogistics_label_layout = fields.Many2one( | ||
comodel_name="postlogistics.delivery.carrier.template.option", | ||
string="Label layout", | ||
domain=[("postlogistics_type", "=", "label_layout")], | ||
) | ||
postlogistics_output_format = fields.Many2one( | ||
comodel_name="postlogistics.delivery.carrier.template.option", | ||
string="Output format", | ||
domain=[("postlogistics_type", "=", "output_format")], | ||
) | ||
postlogistics_resolution = fields.Many2one( | ||
comodel_name="postlogistics.delivery.carrier.template.option", | ||
string="Resolution", | ||
domain=[("postlogistics_type", "=", "resolution")], | ||
) |
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.
We should stick to standard by using and extending (if needed) the model "report.paperformat" from base module.
https://github.com/odoo/odoo/blob/2efab4e7b90d36ce60c9858576922b504509993a/odoo/addons/base/models/report_paperformat.py#L167
# Copyright 2013-2016 Camptocamp SA | ||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | ||
|
||
from odoo import fields, models | ||
|
||
POSTLOGISTICS_TYPES = [ | ||
("label_layout", "Label Layout"), | ||
("output_format", "Output Format"), | ||
("resolution", "Output Resolution"), | ||
("basic", "Basic Service"), | ||
("additional", "Additional Service"), | ||
("delivery", "Delivery Instructions"), | ||
("partner_option", "Partner Option"), | ||
] | ||
|
||
|
||
class DeliveryCarrierTemplateOption(models.Model): | ||
_name = "postlogistics.delivery.carrier.template.option" | ||
_description = "Delivery carrier template option" | ||
|
||
partner_id = fields.Many2one(comodel_name="res.partner", string="Carrier") | ||
name = fields.Char(translate=True) | ||
code = fields.Char() | ||
description = fields.Char( | ||
help="Allow to define a more complete description than in the name field.", | ||
) | ||
postlogistics_type = fields.Selection( | ||
selection=POSTLOGISTICS_TYPES, | ||
string="PostLogistics option type", | ||
) |
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.
We should stick to standard by using and extending (if needed) the model "report.paperformat" from base module.
https://github.com/odoo/odoo/blob/2efab4e7b90d36ce60c9858576922b504509993a/odoo/addons/base/models/report_paperformat.py#L167
return packages | ||
|
||
def get_shipping_label_values(self, label): | ||
# TODO: consider to depends on base_delivery_carrier_label |
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.
This migration MUST be the opportunity to do so and will. I agreed.
"Fixed delivery date", help="Specific delivery date (ZAW3217)" | ||
) | ||
|
||
# TODO: consider refactoring these fields using a partner relation instead |
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.
This migration MUST be the opportunity to do so and will. I agreed.
"""Pickings using this module must have a package | ||
If not this method put it one silently | ||
""" | ||
# TODO: consider to depends on base_delivery_carrier_label |
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.
This migration MUST be the opportunity to do so and will. I agreed.
Should depends of #938 |
superseeded by #974 |
replaced by #974 |
No description provided.