[IMP] delivery_postlogistics: refactor API of web service class to avoid loss when several unchained modules implements new behaviors in it

Author: StephaneMangin

delivery_postlogistics/models/stock_picking.py

@@ -1,14 +1,17 @@
 # Copyright 2013 Camptocamp SA
 # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
 import base64
+from io import BytesIO
 from operator import attrgetter
 
 import lxml.html
+from PIL import Image
+from typing_extensions import deprecated
 
 from odoo import api, fields, models, tools
 from odoo.exceptions import UserError
 
-from ..postlogistics.web_service import PostlogisticsWebService
+from ..postlogistics.web_service import PostlogisticsWebService, sanitize_string
 
 
 class StockPicking(models.Model):
@@ -27,6 +30,14 @@ class StockPicking(models.Model):
         "Mobile", help="For notify delivery by telephone (ZAW3213)"
     )
 
+    @deprecated(
+        "This method will be removed in version 18.0. \
+        Please use _get_quant_packages_from_picking instead."
+    )
+    def _get_packages_from_picking(self):
+        # TODO: remove this method in version > 18.0
+        return self._get_quant_packages_from_picking()
+
     def _get_quant_packages_from_picking(self):
         """Get all the quant packages from the picking"""
         self.ensure_one()
@@ -235,3 +246,126 @@ def action_generate_carrier_label(self):
         if not self.carrier_id:
             raise UserError(self.env._("Please, set a carrier."))
         self.env["delivery.carrier"].postlogistics_send_shipping(self)
+
+    #
+    # Postlogistics specific methods allowing proper override
+    #
+
+    def get_package_number_hook(self, package):
+        """Hook method to customize the package number retrieval"""
+        return None
+
+    def get_recipient_partner_hook(self):
+        """Hook method to customize the partner retrieval"""
+        self.ensure_one()
+        if self.picking_type_id.code != "outgoing":
+            return (
+                self.location_dest_id.company_id.partner_id
+                or self.env.user.company_id.partner_id
+            )
+        return self.partner_id
+
+    def postlogistics_label_prepare_attributes(
+        self, pack=None, pack_num=None, pack_total=None, pack_weight=None
+    ):
+        """This method aims to prepare a dictionary of attributes to be sent
+        to the PostLogistics API"""
+        self.ensure_one()
+        package_type = (
+            pack
+            and pack.package_type_id
+            or self.carrier_id.postlogistics_default_package_type_id
+        )
+        package_codes = package_type._get_shipper_package_code_list()
+
+        if pack_weight:
+            total_weight = pack_weight
+        else:
+            total_weight = pack.shipping_weight if pack else self.shipping_weight
+        total_weight *= 1000
+
+        if not package_codes:
+            raise UserError(
+                self.env._(
+                    "No PostLogistics packaging services found "
+                    "in package type {package_type_name}, for picking {picking_name}."
+                ).format(package_type_name=package_type.name, picking_name=self.name)
+            )
+
+        # Activate phone notification ZAW3213
+        # if phone call notification is set on partner
+        if self.partner_id.postlogistics_notification == "phone":
+            package_codes.append("ZAW3213")
+
+        attributes = {
+            "weight": int(total_weight),
+        }
+
+        # Remove the services if the delivery fixed date is not set
+        if "ZAW3217" in package_codes:
+            if self.delivery_fixed_date:
+                attributes["deliveryDate"] = self.delivery_fixed_date
+            else:
+                package_codes.remove("ZAW3217")
+
+        # parcelNo / parcelTotal cannot be used if service ZAW3218 is not activated
+        if "ZAW3218" in package_codes:
+            if pack_total > 1:
+                attributes.update(
+                    {"parcelTotal": pack_total - 1, "parcelNo": pack_num - 1}
+                )
+            else:
+                package_codes.remove("ZAW3218")
+
+        if "ZAW3219" in package_codes and self.delivery_place:
+            attributes["deliveryPlace"] = self.delivery_place
+        if self.carrier_id.postlogistics_proclima_logo:
+            attributes["proClima"] = True
+        else:
+            attributes["proClima"] = False
+
+        attributes["przl"] = package_codes
+
+        return attributes
+
+    def postlogistics_label_prepare_customer(self):
+        """Create a ns0:Customer as a dict from picking
+
+        This is the PostLogistics Customer, thus the sender
+
+        :param picking: picking browse record
+        :return a dict containing data for ns0:Customer
+
+        """
+        self.ensure_one()
+        company = self.company_id
+        partner = company.partner_id
+        if self.picking_type_id.code != "outgoing":
+            partner = self.partner_id
+
+        partner_name = partner.name or partner.parent_id.name
+        if not partner_name:
+            raise UserError(self.env._("Customer name is required."))
+        customer = {
+            "name1": sanitize_string(partner_name)[:25],
+            "street": sanitize_string(partner.street)[:25],
+            "zip": sanitize_string(partner.zip)[:10],
+            "city": sanitize_string(partner.city)[:25],
+            "country": partner.country_id.code,
+            "domicilePostOffice": self.carrier_id.postlogistics_office or None,
+        }
+        logo = self.carrier_id.postlogistics_logo
+        if logo:
+            logo_image = Image.open(BytesIO(base64.b64decode(logo)))
+            logo_format = logo_image.format
+            customer["logo"] = logo.decode()
+            customer["logoFormat"] = logo_format
+        return customer
+
+    def postlogistics_label_cash_on_delivery(self, package=None):
+        amount = (package or self).postlogistics_cod_amount()
+        amount = f"{amount:.2f}"
+        return [{"Type": "NN_BETRAG", "Value": amount}]
+
+    def postlogistics_label_get_item_additional_data(self, package=None):
+        return []

delivery_postlogistics/postlogistics/web_service.py

@@ -8,11 +8,10 @@
 import threading
 import urllib.parse
 from datetime import datetime, timedelta
-from io import BytesIO
 from json import JSONDecodeError
 
 import requests
-from PIL import Image
+from typing_extensions import deprecated
 
 from odoo.exceptions import UserError
 
@@ -33,6 +32,21 @@
 }
 
 
+def sanitize_string(value, mapping=None):
+    """Remove disallowed characters ("|", "\", "<", ">", "’", "‘") from a string
+
+    :param value: string to sanitize
+    :param mapping: dict of disallowed characters to remove
+    :return: sanitized string
+
+    """
+    mapping = mapping or DISALLOWED_CHARS_MAPPING
+    value = value or ""
+    for char, repl in mapping.items():
+        value = value.replace(char, repl)
+    return value
+
+
 class PostlogisticsWebService:
     """Connector with PostLogistics for labels using post.ch API
 
@@ -71,25 +85,21 @@ def _get_language(self, lang):
             return lang_code
         return "en"
 
-    def _prepare_recipient(self, picking):
+    def _prepare_recipient(self, picking, sanitize_mapping=None):
         """Create a ns0:Recipient as a dict from a partner
 
         :param partner: partner browse record
         :return a dict containing data for ns0:Recipient
 
         """
-        partner = picking.partner_id
-        if picking.picking_type_id.code != "outgoing":
-            location_dest = picking.location_dest_id
-            partner = (
-                location_dest.company_id.partner_id
-                or self.env.user.company_id.partner_id
-            )
+        partner = picking.get_recipient_partner_hook()
 
-        partner_mobile = self._sanitize_string(
-            picking.delivery_mobile or partner.mobile
+        partner_mobile = sanitize_string(
+            picking.delivery_mobile or partner.mobile, sanitize_mapping
+        )
+        partner_phone = sanitize_string(
+            picking.delivery_phone or partner.phone, sanitize_mapping
         )
-        partner_phone = self._sanitize_string(picking.delivery_phone or partner.phone)
 
         if partner.postlogistics_notification == "email" and not partner.email:
             raise UserError(picking.env._("Email is required for notification."))
@@ -115,10 +125,10 @@ def _prepare_recipient(self, picking):
             raise UserError(picking.env._("Partner city is required."))
 
         partner_name = partner.name or partner.parent_id.name
-        sanitized_partner_name = self._sanitize_string(partner_name)
-        partner_street = self._sanitize_string(partner.street)
-        partner_zip = self._sanitize_string(partner.zip)
-        partner_city = self._sanitize_string(partner.city)
+        sanitized_partner_name = sanitize_string(partner_name, sanitize_mapping)
+        partner_street = sanitize_string(partner.street, sanitize_mapping)
+        partner_zip = sanitize_string(partner.zip, sanitize_mapping)
+        partner_city = sanitize_string(partner.city, sanitize_mapping)
         recipient = {
             "name1": sanitized_partner_name[:35],
             "street": partner_street[:35],
@@ -127,66 +137,37 @@ def _prepare_recipient(self, picking):
         }
 
         if partner.country_id.code:
-            country_code = self._sanitize_string(partner.country_id.code.upper())
+            country_code = sanitize_string(
+                partner.country_id.code.upper(), sanitize_mapping
+            )
             recipient["country"] = country_code
 
         if partner.street2:
             # addressSuffix is shown before street on label
             recipient["addressSuffix"] = recipient["street"]
-            recipient["street"] = self._sanitize_string(partner.street2[:35])
+            recipient["street"] = sanitize_string(
+                partner.street2[:35], sanitize_mapping
+            )
 
         company_partner_name = partner.commercial_company_name
         if company_partner_name and company_partner_name != partner_name:
-            parent_name = self._sanitize_string(partner.parent_id.name)
+            parent_name = sanitize_string(partner.parent_id.name, sanitize_mapping)
             recipient["name2"] = parent_name[:35]
             recipient["personallyAddressed"] = False
 
         # Phone and / or mobile should only be displayed if instruction to
         # Notify delivery by telephone is set
         if partner.postlogistics_notification == "email":
-            recipient["email"] = self._sanitize_string(partner.email)
+            recipient["email"] = sanitize_string(partner.email, sanitize_mapping)
         elif partner.postlogistics_notification == "phone":
-            recipient["phone"] = self._sanitize_string(partner_phone)
+            recipient["phone"] = sanitize_string(partner_phone, sanitize_mapping)
             if partner_mobile:
                 recipient["mobile"] = partner_mobile
         elif partner.postlogistics_notification == "sms":
             recipient["mobile"] = partner_mobile
 
         return recipient
 
-    def _prepare_customer(self, picking):
-        """Create a ns0:Customer as a dict from picking
-
-        This is the PostLogistics Customer, thus the sender
-
-        :param picking: picking browse record
-        :return a dict containing data for ns0:Customer
-
-        """
-        company = picking.company_id
-        partner = company.partner_id
-        if picking.picking_type_id.code != "outgoing":
-            partner = picking.partner_id
-
-        partner_name = partner.name or partner.parent_id.name
-        if not partner_name:
-            raise UserError(picking.env._("Customer name is required."))
-        customer = {
-            "name1": self._sanitize_string(partner_name)[:25],
-            "street": self._sanitize_string(partner.street)[:25],
-            "zip": self._sanitize_string(partner.zip)[:10],
-            "city": self._sanitize_string(partner.city)[:25],
-            "country": partner.country_id.code,
-            "domicilePostOffice": picking.carrier_id.postlogistics_office or None,
-        }
-        logo = picking.carrier_id.postlogistics_logo
-        if logo:
-            logo_image = Image.open(BytesIO(base64.b64decode(logo)))
-            logo_format = logo_image.format
-            customer["logo"] = logo.decode()
-            customer["logoFormat"] = logo_format
-        return customer
-
     def _get_label_layout(self, picking):
         """
         Get Label layout define in carrier
@@ -215,66 +196,6 @@ def _get_license(self, picking):
         franking_license = picking.carrier_id.postlogistics_license_id
         return franking_license.number
 
-    def _prepare_attributes(
-        self, picking, pack=None, pack_num=None, pack_total=None, pack_weight=None
-    ):
-        package_type = (
-            pack
-            and pack.package_type_id
-            or picking.carrier_id.postlogistics_default_package_type_id
-        )
-        package_codes = package_type._get_shipper_package_code_list()
-
-        if pack_weight:
-            total_weight = pack_weight
-        else:
-            total_weight = pack.shipping_weight if pack else picking.shipping_weight
-        total_weight *= 1000
-
-        if not package_codes:
-            raise UserError(
-                picking.env._(
-                    "No PostLogistics packaging services found "
-                    "in package type {package_type_name}, for picking {picking_name}."
-                ).format(package_type_name=package_type.name, picking_name=picking.name)
-            )
-
-        # Activate phone notification ZAW3213
-        # if phone call notification is set on partner
-        if picking.partner_id.postlogistics_notification == "phone":
-            package_codes.append("ZAW3213")
-
-        attributes = {
-            "weight": int(total_weight),
-        }
-
-        # Remove the services if the delivery fixed date is not set
-        if "ZAW3217" in package_codes:
-            if picking.delivery_fixed_date:
-                attributes["deliveryDate"] = picking.delivery_fixed_date
-            else:
-                package_codes.remove("ZAW3217")
-
-        # parcelNo / parcelTotal cannot be used if service ZAW3218 is not activated
-        if "ZAW3218" in package_codes:
-            if pack_total > 1:
-                attributes.update(
-                    {"parcelTotal": pack_total - 1, "parcelNo": pack_num - 1}
-                )
-            else:
-                package_codes.remove("ZAW3218")
-
-        if "ZAW3219" in package_codes and picking.delivery_place:
-            attributes["deliveryPlace"] = picking.delivery_place
-        if picking.carrier_id.postlogistics_proclima_logo:
-            attributes["proClima"] = True
-        else:
-            attributes["proClima"] = False
-
-        attributes["przl"] = package_codes
-
-        return attributes
-
     def _get_itemid(self, picking, package):
         """Allowed characters are alphanumeric plus `+`, `-` and `_`
         Last `+` separates picking name and package number (if any)
@@ -290,28 +211,21 @@ def _get_itemid(self, picking, package):
         codes = [name, pack_no]
         return "+".join(c for c in codes if c)
 
-    def _cash_on_delivery(self, picking, package=None):
-        amount = (package or picking).postlogistics_cod_amount()
-        amount = f"{amount:.2f}"
-        return [{"Type": "NN_BETRAG", "Value": amount}]
-
-    def _get_item_additional_data(self, picking, package=None):
-        if package and not package.package_type_id:
-            raise UserError(
-                picking.env._("The package %s must have a package type.") % package.name
-            )
-
-        result = []
-        packaging_codes = (
-            package and package.package_type_id._get_shipper_package_code_list() or []
-        )
-
-        if set(packaging_codes) & {"BLN", "N"}:
-            cod_attributes = self._cash_on_delivery(picking, package=package)
-            result += cod_attributes
-        return result
+    def _prepare_data(
+        self, lang, frankingLicense, post_customer, labelDefinition, item
+    ):
+        return {
+            "language": lang.upper(),
+            "frankingLicense": frankingLicense,
+            "ppFranking": False,
+            "customer": post_customer,
+            "customerSystem": None,
+            "labelDefinition": labelDefinition,
+            "sendingID": None,
+            "item": item,
+        }
 
-    def _get_item_number(self, picking, package):
+    def _get_item_number(self, picking, package, index=1):
         """Generate the tracking reference for the last 8 digits
         of tracking number of the label.
 
@@ -321,15 +235,17 @@ def _get_item_number(self, picking, package):
         e.g. 03000042 for 3rd pack of picking OUT/19000042
         """
         picking_num = _compile_itemnum.sub("", picking.name)
-        package_number = self.get_package_number_hook(package)
+        package_number = picking.get_package_number_hook(package)
+        if not package_number:
+            package_number = index
         return "%02d%s" % (package_number, picking_num[-6:].zfill(6))
 
     def _prepare_item_list(self, picking, recipient, packages):
         """Return a list of item made from the pickings"""
         carrier = picking.carrier_id
         item_list = []
 
-        def add_item(package_number=1, package=None):
+        def add_item(index=1, package=None):
             assert picking or package
             itemid = self._get_itemid(picking, package)
             item = {
@@ -344,7 +260,7 @@ def add_item(package_number=1, package=None):
                     picking_num = _compile_itemnum.sub("", picking.name)
                     item_number = f"9{picking_num[-7:].zfill(7)}"
                 else:
-                    item_number = self._get_item_number(picking, package_number)
+                    item_number = self._get_item_number(picking, package, index)
                 item["itemNumber"] = item_number
 
             additional_data = self._get_item_additional_data(picking, package=package)
@@ -354,8 +270,10 @@ def add_item(package_number=1, package=None):
             item_list.append(item)
 
         total_packages = len(packages)
-        for package in packages:
-            package_number = self.get_package_number_hook(package)
+        for index, package in enumerate(packages):
+            package_number = picking.get_package_number_hook(package)
+            if not package_number:
+                package_number = index + 1
             attributes = self._prepare_attributes(
                 picking, package, package_number, total_packages
             )
@@ -403,20 +321,6 @@ def _prepare_label_definition(self, picking):
             "printPreview": False,
         }
 
-    def _prepare_data(
-        self, lang, frankingLicense, post_customer, labelDefinition, item
-    ):
-        return {
-            "language": lang.upper(),
-            "frankingLicense": frankingLicense,
-            "ppFranking": False,
-            "customer": post_customer,
-            "customerSystem": None,
-            "labelDefinition": labelDefinition,
-            "sendingID": None,
-            "item": item,
-        }
-
     @classmethod
     def _request_access_token(cls, delivery_carrier):
         if not delivery_carrier.postlogistics_endpoint_url:
@@ -500,13 +404,6 @@ def get_access_token(cls, picking_carrier):
             cls.access_token_expiry = now + timedelta(seconds=response["expires_in"])
             return cls.access_token
 
-    def _sanitize_string(self, value):
-        """Removes disallowed chars ("|", "\", "<", ">", "’", "‘") from strings."""
-        value = value or ""
-        for char, repl in DISALLOWED_CHARS_MAPPING.items():
-            value = value.replace(char, repl)
-        return value
-
     def generate_label(self, picking, packages):
         """Generate a label for a picking
 
@@ -601,6 +498,64 @@ def generate_label(self, picking, packages):
             results.append(res)
         return results
 
-    def get_package_number_hook(self, package):
-        """Hook method to customize the package number retrieval"""
-        return package
+    # These methods could be overridden in a custom module, thus if several
+    # modules are installed but not chained properly, the last one will be used.
+    # Meaning if you doesn't inherit from the last module, this module override
+    # will not be used. This is a big issue in a modular environment, thus we
+    # need to provide a better way to allow to override these methods by
+    # implementing them in the picking model.
+
+    @deprecated(
+        "This method will be removed in version > 18.0. Please use \
+        `stock.picking::postlogistics_label_cash_on_delivery` instead."
+    )
+    def _cash_on_delivery(self, picking, package=None):
+        picking.postlogistics_label_cash_on_delivery(package=package)
+
+    @deprecated(
+        "This method will be removed in version > 18.0. Please use \
+        `stock.picking::postlogistics_label_get_item_additional_data` instead."
+    )
+    def _get_item_additional_data(self, picking, package=None):
+        # TODO: remove this method in versions > 18.0 and reimplement current
+        #  behavior inside stock.picking::postlogistics_label_get_item_additional_data
+        if package and not package.package_type_id:
+            raise UserError(
+                self.env._("The package %s must have a package type.") % package.name
+            )
+        result = picking.postlogistics_label_get_item_additional_data(package=package)
+        packaging_codes = (
+            package and package.package_type_id._get_shipper_package_code_list() or []
+        )
+        if set(packaging_codes) & {"BLN", "N"}:
+            cod_attributes = self._cash_on_delivery(picking, package=package)
+            result += cod_attributes
+        return result
+
+    @deprecated(
+        "This method will be removed in version > 18.0. \
+        Please use global `sanitize_string` instead."
+    )
+    def _sanitize_string(self, value, mapping=None):
+        # TODO: remove this method in versions > 18.0
+        return sanitize_string(value, mapping)
+
+    @deprecated(
+        "This method will be removed in version > 18.0. Please use \
+        `stock.picking::postlogistics_label_prepare_attributes` instead."
+    )
+    def _prepare_attributes(
+        self, picking, pack=None, pack_num=None, pack_total=None, pack_weight=None
+    ):
+        # TODO: remove this method in versions > 18.0
+        return picking.postlogistics_label_prepare_attributes(
+            pack=pack, pack_num=pack_num, pack_total=pack_total, pack_weight=pack_weight
+        )
+
+    @deprecated(
+        "This method will be removed in version > 18.0. Please use \
+        `stock.picking::postlogistics_label_prepare_recipient` instead."
+    )
+    def _prepare_customer(self, picking):
+        # TODO: remove this method in versions > 18.0
+        return picking.postlogistics_label_prepare_customer()

delivery_postlogistics/tests/test_sanitize_values.py

@@ -46,7 +46,7 @@ def check_strings_in_list(self, values):
             )
 
     def test_sanitize(self):
-        customer = self.service_class._prepare_customer(self.picking)
+        customer = self.picking.postlogistics_label_prepare_customer()
         self.check_strings_in_dict(customer)
         recipient = self.service_class._prepare_recipient(self.picking)
         self.check_strings_in_dict(recipient)
@@ -55,9 +55,7 @@ def test_sanitize(self):
             self.picking, recipient, packages
         )
         self.check_strings_in_list(item_list)
-        attributes = self.service_class._prepare_attributes(
-            self.picking, packages, 1, 1
-        )
+        attributes = self.picking.postlogistics_label_prepare_attributes(packages, 1, 1)
         self.check_strings_in_dict(attributes)
 
     def test_cleanup_error_message(self):