[IMP] base_delivery_carrier_label: Externalize automatic package creation #643

rousseldenis · 2023-05-23T14:50:03Z

Depends on :

[16.0][ADD] delivery_automatic_package #642

I've added delivery_postlogistics in rebel modules as it overrides base_delivery_carrier_label methods without calling super. @lehoangan @simahawk

rousseldenis · 2023-05-23T14:55:35Z

@florian-dacosta Following our discussion about this.

I've added a separate module in order to isolate the functionnality and did it configurable.

So, this is a breaking change from former behaviour but easily configurable (see module README)

@bealdav

lmignon

LGTM (Code review)

florian-dacosta

@rousseldenis I wonder if we really want all this..

I mean, base_delivery_carrier_package has now no link at all with delivery_automatic_package
In the tests, you actually just test that delivery_automatic_package works, we could do the same in delivery module or any module depending on it. It is quite strange to add these tests in this module when there is really no link.

I understand the initial purpose of course, which would be not to remove a functionality from the module out of nowhere, but even with the dependency is is a breaking change, since configured with no package by default.

Couldn't we just remove drop the tests and the dependency on delivery_automatic_package and just remove _set_a_default_package method in this PR?
It would still be a breaking change quite easily solved with the intallation of the module... I mean, since the user will need to be aware of this change to make it continue to work...

Moreover, this feature is probably not really use yet in v16, at least willingly, for now...

IMO, the new module is mainly usefull for carrier modules which needs pack to work, and for customer that' don't want to manage PACK when there is only one PACK by picking, so it should be a dependency of those module but not of base_delivery_carrier_label.
For instance, delivery_roulier needs packs on the picking to be able to generate the labels. This one should/may depend on delivery_automatic_package. And in fact, I think I won't add this dependency in delivery_roulier, I'd probably prefer to raise in case there is no pack and willingly install/configure delivery_automatic_package for customers that need it.

What do you think?

github-actions · 2023-10-01T12:23:52Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

florian-dacosta · 2023-10-02T07:36:17Z

@rousseldenis Any news ?
I really think it is strange to add test on this module for a feature added in a submodule, but if you think it is important, I'll approve as it is. Also, can you rebase?

sbidoul · 2023-12-21T09:16:21Z

@rousseldenis what's the status of this one?

rousseldenis · 2023-12-21T09:26:35Z

@rousseldenis what's the status of this one?

I'll fix this now

rousseldenis · 2023-12-21T13:02:24Z

delivery_roulier/tests/test_delivery_roulier.py

@@ -55,6 +55,7 @@ def setUp(self):
                "delivery_type": "test",
                "product_id": delivery_product.id,
                "carrier_account_id": self.account.id,
+                "automatic_package_creation_at_delivery": True,


@florian-dacosta I've updated the test here to get the same behavior as before.

A migration script is missing to set the value also on existing carriers,
otherwise it is a breaking change.

…tion

rousseldenis · 2024-02-09T08:00:39Z

@chienandalu Could you review this too as this is a breaking change ? I think we need to update your ctt express module too to get the automatic package creation with the new module.

chienandalu

Hi @rousseldenis thanks for the heads up 🙂 Anyway, we don't use base_delivery_carrier_label in our carrier integrations, so I can't judge properly

rousseldenis · 2024-02-09T08:13:57Z

Hi @rousseldenis thanks for the heads up 🙂 Anyway, we don't use base_delivery_carrier_label in our carrier integrations, so I can't judge properly

Ok, I haven't put my head in the code. But could you explain the test fail here ? Thanks

chienandalu · 2024-02-09T08:16:28Z

Ok, I didn't see that 🤦‍♂️

chienandalu · 2024-02-09T08:31:37Z

I can't see how your changes might affect that integration. Could relaunch them to give them another shot?

rousseldenis · 2024-02-09T08:32:28Z

I can't see how your changes might affect that integration. Could relaunch them to give them another shot?

Done

rousseldenis · 2024-02-09T08:52:27Z

@chienandalu @florian-dacosta That's because delivery_package_number uses a 'set_default_package' context... but does not depends on base_delivery_carrier_label...

@chienandalu So, your module was working by chance (I suppose you have base_delivery_carrier_label installed on your projects too 😅 )

rousseldenis · 2024-02-09T08:53:56Z

@chienandalu @florian-dacosta That's because delivery_package_number uses a 'set_default_package' context... but does not depends on base_delivery_carrier_label...

@chienandalu So, your module was working by chance (I suppose you have base_delivery_carrier_label installed on your projects too 😅 )

@florian-dacosta @chienandalu I propose to add package_automatic module in delivery_package_number dependencies

chienandalu · 2024-02-09T09:04:27Z

That's because delivery_package_number uses a 'set_default_package' context... but does not depends on base_delivery_carrier_label...

I see it's intentional and the only purpose is to avoid OCA's tests fail:

delivery-carrier/delivery_package_number/wizard/stock_inmediate_transfer.py

Lines 13 to 16 in 31b6df1

    
           # put context key for avoiding `base_delivery_carrier_label` auto-packaging feature 
        
           res = super( 
        
               StockImmediateTransfer, self.with_context(set_default_package=False) 
        
           ).process()

rousseldenis · 2024-02-09T09:10:07Z

Mmmh ok, didn't see the False value. So, there is something that put a new package before this that makes your tests happy because cttexpress requires a package I see.

chienandalu · 2024-02-09T09:45:48Z

So, there is something that put a new package before this that makes your tests happy because cttexpress requires a package I see.

I'd say is the opposite, as the issue is that we're declaring 0 packages to the carrier (and this will happen for every carrier integration using this methods). Somehow this new chain of dependencies make that this compute is triggered after we've already set the number of packages:

delivery-carrier/delivery_package_number/models/stock_picking.py

Lines 19 to 23 in 31b6df1

    
           @api.depends("package_ids") 
        
           def _compute_number_of_packages(self): 
        
               for picking in self: 
        
                   if picking.package_ids: 
        
                       picking.number_of_packages = len(picking.package_ids)

My only guess is that somehow the company stays with the automatic package setting to True, but I didn't find where could that be set.

Things I'd try:

Try out that theory printing that flag value when it fails so we can probe it at the moment of fail.
Maybe a context to that compute that avoids being triggered when the number of packages is confirmed from the wizard.

mt-software-de

Can you cherry-pick
1984571
86b0685
f38d3d3

mt-software-de · 2024-03-20T13:43:59Z

delivery_roulier/tests/test_delivery_roulier.py

@@ -55,6 +55,7 @@ def setUp(self):
                "delivery_type": "test",
                "product_id": delivery_product.id,
                "carrier_account_id": self.account.id,
+                "automatic_package_creation_at_delivery": True,


A migration script is missing to set the value also on existing carriers,
otherwise it is a breaking change.

mt-software-de · 2024-03-20T13:55:42Z

Which methods are called in delivery_postlogistics from base_delivery_carrier_label without super?
Because there is not dependency on delivery_postlogistics for base_delivery_carrier_label

mt-software-de · 2024-04-15T22:27:34Z

Which methods are called in delivery_postlogistics from base_delivery_carrier_label without super? Because there is not dependency on delivery_postlogistics for base_delivery_carrier_label

@rousseldenis could you give an update on this?
My PR #786 is waiting.
We should refactor the module or run the tests in a separate flow like i did it in 786.

hparfr · 2024-05-21T16:20:17Z

I'm ok with @florian-dacosta: don't add a new dependency to base_delivery_label (opposit of this pr)
Remove: def _set_a_default_package(self): (already in the pr)

In delivery_roulier package have to exist and it has to fail if no packages are defined.
How the packages are created is not in the scope of delivery_roulier

github-actions · 2024-09-22T12:30:43Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

rousseldenis added enhancement needs review labels May 23, 2023

rousseldenis added this to the 16.0 milestone May 23, 2023

rousseldenis force-pushed the 16.0-imp-delivery_carrier_label-dro branch 2 times, most recently from 69c4907 to 2b12c8d Compare May 24, 2023 08:13

lmignon approved these changes May 24, 2023

View reviewed changes

florian-dacosta reviewed May 31, 2023

View reviewed changes

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 1, 2023

rousseldenis force-pushed the 16.0-imp-delivery_carrier_label-dro branch from 2b12c8d to 6ba7e0c Compare October 2, 2023 09:21

github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 8, 2023

rousseldenis force-pushed the 16.0-imp-delivery_carrier_label-dro branch 5 times, most recently from 90a7232 to 1f08c71 Compare December 21, 2023 12:44

rousseldenis commented Dec 21, 2023

View reviewed changes

rousseldenis added 6 commits February 9, 2024 08:51

[IMP] base_delivery_carrier_label: Externalize automatic package crea…

6e437f9

…tion

[IMP] delivery_postlogistics: Add to rebel module

06125d2

[FIX] base_delivery_carrier_label: Fix parameter in tests

7aa53a6

[IMP] base_delivery_carrier_label: Separate test models

25eae62

[UPD] delivery_roulier: In tests flow, set an automatic package creation

4521cb3

[IMP] base_delivery_carrier_label: Check warnings in tests

242cb68

rousseldenis force-pushed the 16.0-imp-delivery_carrier_label-dro branch from bd9244c to 242cb68 Compare February 9, 2024 07:51

chienandalu reviewed Feb 9, 2024

View reviewed changes

rousseldenis mentioned this pull request Mar 19, 2024

[14.0][FIX] Unittests base_delivery_carrier_label: Separate github test run #786

Closed

mt-software-de suggested changes Mar 20, 2024

View reviewed changes

mt-software-de mentioned this pull request Apr 15, 2024

[14.0][FIX] delivery_schenker: correct float_round return #710

Merged

hparfr mentioned this pull request Jul 9, 2024

[16.0][BC] base_delivery_carrier_label: remove _set_a_default_package #853

Merged

1 task

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Sep 22, 2024

github-actions bot closed this Oct 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] base_delivery_carrier_label: Externalize automatic package creation #643

[IMP] base_delivery_carrier_label: Externalize automatic package creation #643

rousseldenis commented May 23, 2023 •

edited by sbidoul

Loading

rousseldenis commented May 23, 2023

lmignon left a comment

florian-dacosta left a comment

github-actions bot commented Oct 1, 2023

florian-dacosta commented Oct 2, 2023 •

edited

Loading

sbidoul commented Dec 21, 2023

rousseldenis commented Dec 21, 2023

rousseldenis Dec 21, 2023

mt-software-de Mar 20, 2024 •

edited

Loading

rousseldenis commented Feb 9, 2024

chienandalu left a comment

rousseldenis commented Feb 9, 2024

chienandalu commented Feb 9, 2024

chienandalu commented Feb 9, 2024

rousseldenis commented Feb 9, 2024

rousseldenis commented Feb 9, 2024 •

edited

Loading

rousseldenis commented Feb 9, 2024

chienandalu commented Feb 9, 2024 •

edited

Loading

rousseldenis commented Feb 9, 2024

chienandalu commented Feb 9, 2024

mt-software-de left a comment

mt-software-de Mar 20, 2024 •

edited

Loading

mt-software-de commented Mar 20, 2024

mt-software-de commented Apr 15, 2024

hparfr commented May 21, 2024

github-actions bot commented Sep 22, 2024

[IMP] base_delivery_carrier_label: Externalize automatic package creation #643

[IMP] base_delivery_carrier_label: Externalize automatic package creation #643

Conversation

rousseldenis commented May 23, 2023 • edited by sbidoul Loading

rousseldenis commented May 23, 2023

lmignon left a comment

Choose a reason for hiding this comment

florian-dacosta left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 1, 2023

florian-dacosta commented Oct 2, 2023 • edited Loading

sbidoul commented Dec 21, 2023

rousseldenis commented Dec 21, 2023

rousseldenis Dec 21, 2023

Choose a reason for hiding this comment

mt-software-de Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

rousseldenis commented Feb 9, 2024

chienandalu left a comment

Choose a reason for hiding this comment

rousseldenis commented Feb 9, 2024

chienandalu commented Feb 9, 2024

chienandalu commented Feb 9, 2024

rousseldenis commented Feb 9, 2024

rousseldenis commented Feb 9, 2024 • edited Loading

rousseldenis commented Feb 9, 2024

chienandalu commented Feb 9, 2024 • edited Loading

rousseldenis commented Feb 9, 2024

chienandalu commented Feb 9, 2024

mt-software-de left a comment

Choose a reason for hiding this comment

mt-software-de Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

mt-software-de commented Mar 20, 2024

mt-software-de commented Apr 15, 2024

hparfr commented May 21, 2024

github-actions bot commented Sep 22, 2024

rousseldenis commented May 23, 2023 •

edited by sbidoul

Loading

florian-dacosta commented Oct 2, 2023 •

edited

Loading

mt-software-de Mar 20, 2024 •

edited

Loading

rousseldenis commented Feb 9, 2024 •

edited

Loading

chienandalu commented Feb 9, 2024 •

edited

Loading

mt-software-de Mar 20, 2024 •

edited

Loading