[17.0][MIG] delivery_roulier: Migration to 17.0 #826

yankinmax · 2024-06-04T16:28:25Z

Depends on:

delivery_roulier/tests/test_delivery_roulier.py

Camille0907

Migration technically LGTM

delivery_roulier/models/stock_move_line.py

yankinmax · 2024-06-28T12:43:17Z

delivery_roulier/models/stock_picking.py

@@ -290,3 +290,17 @@ def open_website_url(self):
        action["domain"] = [("id", "in", packages.ids)]
        action["context"] = {"picking_id": self.id}
        return action
+
+    def _put_in_pack(self, move_line_ids):


I've splitted correctly the migration commit to pre-commit one.
Here is the latest fixup to update quant_ids on the package with all quants from move lines in this package.
@rousseldenis @Camille0907
thanks for the reviews

Hello,
I don't get why we need this, not why it is in this module. Is this necessary for this module to work ? Why ?

Hello @florian-dacosta
From my POV, when the package is created and quants are put in pack, it should be linked to a package.
Probably it was a case in previous versions, but not in v17.
As a result here:

def _roulier_get_parcel(self, picking): self.ensure_one() weight = self.shipping_weight or self.weight parcel = {"weight": weight, "reference": self.name} return parcel

shipping_weight or weight are missed and roulier response is wrong.
If you can check the code and suggest better solution it would be great.
BTW, I'm not an expert using this module. But, I need it to be migrated for my project.
Thanks in advance.

Well, I believe it has always been this way. When you click on "put in pack" button, the pack is set on the operations (stock.move.line) but if you check the pack, it is still empty (no quant inside). This is not specific to the v17.
I understand it may be an issue, if you want to generate the label, before validating the picking, which is not the way odoo works.

The label generation is triggered at the end of "stock.picking._action_done". At this stage, the quant are already linked to the pack, so the weight is ok.

I guess it would be nice to be able to generate label before the validation of the shipping, but :

This is not a problem specific to delivery_roulier, I think it could be an issue for any module managing the carrier label. If I should pick a module with something like that, I'd probably create a new dedicated module, which you could choose to install or not, along with delivery_roulier (or any other module managing shipping labels).

The way to do it does not seem ok to me. Indeed, the button put_in_pack is optional, you can also set the result_package_id of the stock.move.line manually (in this case the weight won't be ok either)
After you set a result package id, you can remove it manually... You can delete the stock.move.line, you can change the pack, you can do anything. Then, the package on some quants will be wrong, which is an issue.
Also, you have a source package and a result package on stock.move.line. When a quant is already in a pack, when you transfer it, the source package is filled. If you write the pack on the quant with the "put in pack" button, then the source package should probably be set, not sure about the impacts.

Anyway, I have no solution to propose around this subject now, but this one should definitely not go here and should probably be well tested as well, with the different cases.

yankinmax · 2024-07-08T18:40:01Z

@rousseldenis is this migration ok now?

florian-dacosta · 2024-07-09T09:37:52Z

delivery_roulier/models/stock_picking.py

@@ -290,3 +290,17 @@ def open_website_url(self):
        action["domain"] = [("id", "in", packages.ids)]
        action["context"] = {"picking_id": self.id}
        return action
+
+    def _put_in_pack(self, move_line_ids):


Well, I believe it has always been this way. When you click on "put in pack" button, the pack is set on the operations (stock.move.line) but if you check the pack, it is still empty (no quant inside). This is not specific to the v17.
I understand it may be an issue, if you want to generate the label, before validating the picking, which is not the way odoo works.

The label generation is triggered at the end of "stock.picking._action_done". At this stage, the quant are already linked to the pack, so the weight is ok.

I guess it would be nice to be able to generate label before the validation of the shipping, but :

This is not a problem specific to delivery_roulier, I think it could be an issue for any module managing the carrier label. If I should pick a module with something like that, I'd probably create a new dedicated module, which you could choose to install or not, along with delivery_roulier (or any other module managing shipping labels).

The way to do it does not seem ok to me. Indeed, the button put_in_pack is optional, you can also set the result_package_id of the stock.move.line manually (in this case the weight won't be ok either)
After you set a result package id, you can remove it manually... You can delete the stock.move.line, you can change the pack, you can do anything. Then, the package on some quants will be wrong, which is an issue.
Also, you have a source package and a result package on stock.move.line. When a quant is already in a pack, when you transfer it, the source package is filled. If you write the pack on the quant with the "put in pack" button, then the source package should probably be set, not sure about the impacts.

Anyway, I have no solution to propose around this subject now, but this one should definitely not go here and should probably be well tested as well, with the different cases.

yankinmax · 2024-07-09T09:43:18Z

I'll remove the part I've proposed.
I don't have any other better solution for now.

florian-dacosta · 2024-07-09T10:53:07Z

I'll remove the part I've proposed. I don't have any other better solution for now.

Thanks, I'll approve then.
Can you also have a look here #851 and make a cherry pick to include the v16 fix here please?

yankinmax · 2024-07-09T10:55:10Z

I'll remove the part I've proposed. I don't have any other better solution for now.

Thanks, I'll approve then. Can you also have a look here #851 and make a cherry pick to include the v16 fix here please?

of course, thanks

yankinmax · 2024-07-09T11:08:16Z

@florian-dacosta I've checked your fix in v17. Still the same issue:

_____________________________________________________________________________________ DeliveryRoulierCase.test_roulier _____________________________________________________________________________________

self = <odoo.addons.delivery_roulier.tests.test_delivery_roulier.DeliveryRoulierCase testMethod=test_roulier>

    def test_roulier(self):
        roulier.get_carriers_action_available = MagicMock(
            return_value={"test": ["get_label"]}
        )
        with patch("roulier.roulier.get") as mock_roulier:
            mock_roulier.return_value = roulier_ret
            self.picking.send_to_shipper()
            roulier_args = mock_roulier.mock_calls[0][1]
            self.assertEqual("get_label", roulier_args[1])
            roulier_payload = roulier_args[2]
            self.assertEqual(len(roulier_payload["parcels"]), 1)
>           self.assertEqual(roulier_payload["parcels"][0].get("weight"), 1.2)
E           AssertionError: 0.0 != 1.2

so weight is not properly updated in _roulier_get_parcel

florian-dacosta · 2024-07-09T11:13:34Z

@florian-dacosta I've checked your fix in v17. Still the same issue:

_____________________________________________________________________________________ DeliveryRoulierCase.test_roulier _____________________________________________________________________________________

self = <odoo.addons.delivery_roulier.tests.test_delivery_roulier.DeliveryRoulierCase testMethod=test_roulier>

    def test_roulier(self):
        roulier.get_carriers_action_available = MagicMock(
            return_value={"test": ["get_label"]}
        )
        with patch("roulier.roulier.get") as mock_roulier:
            mock_roulier.return_value = roulier_ret
            self.picking.send_to_shipper()
            roulier_args = mock_roulier.mock_calls[0][1]
            self.assertEqual("get_label", roulier_args[1])
            roulier_payload = roulier_args[2]
            self.assertEqual(len(roulier_payload["parcels"]), 1)
>           self.assertEqual(roulier_payload["parcels"][0].get("weight"), 1.2)
E           AssertionError: 0.0 != 1.2

so weight is not properly updated in _roulier_get_parcel

The fix was not related to the weight issue, it was about picking tracking link.
Where do you see this error ? The test on this PR seem fine, am I missing something ?

yankinmax · 2024-07-09T11:27:34Z

ah sorry I forgot to update, wait a sec

yankinmax · 2024-07-09T11:34:45Z

@florian-dacosta
For this error my fix was applied with put_in_pack override:
https://github.com/OCA/delivery-carrier/actions/runs/9856183670/job/27212631968?pr=826#step:8:74

florian-dacosta · 2024-07-09T12:45:24Z

Oh, I understand. After checking the test both on v16 and v17, here is the change that did break the delivery_roulier test :
99e3d58#diff-8e3509b96799ba2c6bfb09b79df02a224f2ed73f5b529a0e2fa210f105fd64d2R25
Condition was changed from if not pack.quant_ids to if pack.quant_ids
I don't understand why this was changed, but this is the reason of the test failing now.

yankinmax · 2024-07-09T13:23:56Z

Oh, I understand. After checking the test both on v16 and v17, here is the change that did break the delivery_roulier test : 99e3d58#diff-8e3509b96799ba2c6bfb09b79df02a224f2ed73f5b529a0e2fa210f105fd64d2R25 Condition was changed from if not pack.quant_ids to if pack.quant_ids I don't understand why this was changed, but this is the reason of the test failing now.

I'll prepare a fix

Currently translated at 100.0% (13 of 13 strings) Translation: delivery-carrier-16.0/delivery-carrier-16.0-delivery_roulier Translate-URL: https://translation.odoo-community.org/projects/delivery-carrier-16-0/delivery-carrier-16-0-delivery_roulier/it/

yankinmax · 2024-07-09T15:30:13Z

@florian-dacosta I'm feeling like we can finish the job finally)
Can you approve?

delivery_roulier/__manifest__.py

yankinmax · 2024-07-09T15:50:58Z

BTW, if we could merge this one today we can also merge one more so it can be a very fast line day 😄

[17.0][MIG] delivery_roulier_option: Migration to 17.0 #829

florian-dacosta · 2024-07-09T15:54:49Z

@rousseldenis I guess your comment is now obsolete, but if you could confirm it changing your review it would be great!

yankinmax · 2024-07-10T07:36:22Z

@rousseldenis can you please update your review and in case you approve trigger the merge?

delivery_roulier/models/delivery_carrier.py

delivery_roulier/tests/test_delivery_roulier.py

requirements.txt

simahawk · 2024-07-11T06:46:26Z

/ocabot migration delivery_roulier

yankinmax · 2024-07-11T07:52:12Z

@simahawk updated with fixup

simahawk

@yankinmax cool, tnx. Don't forget to squash 😉

…ad of a list of urls

simahawk · 2024-07-12T07:19:35Z

/ocabot merge nopbump

OCA-git-bot · 2024-07-12T07:19:38Z

Hi @simahawk. Your command failed:

Invalid options for command merge: nopbump.

Ocabot commands

ocabot merge major|minor|patch|nobump
ocabot rebase
ocabot migration {MODULE_NAME}

More information

ocabot documentation
OCA guidelines, specially the "Version Numbers" section.

simahawk · 2024-07-12T07:19:46Z

/ocabot merge nobump

OCA-git-bot · 2024-07-12T07:19:51Z

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-826-by-simahawk-bump-nobump, awaiting test results.

OCA-git-bot · 2024-07-12T07:23:50Z

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

yankinmax force-pushed the 17.0-mig-delivery_roulier branch 2 times, most recently from 2b31945 to 14f3bb5 Compare June 5, 2024 08:51

yankinmax commented Jun 5, 2024

View reviewed changes

delivery_roulier/tests/test_delivery_roulier.py Outdated Show resolved Hide resolved

This was referenced Jun 5, 2024

Migration to version 17.0 #729

Open

[17.0][MIG] delivery_roulier_option: Migration to 17.0 #829

Merged

[17.0][MIG] delivery_roulier_chronopost_fr: Migration to 17.0 #830

Open

vvrossem reviewed Jun 6, 2024

View reviewed changes

delivery_roulier/tests/test_delivery_roulier.py Outdated Show resolved Hide resolved

yankinmax force-pushed the 17.0-mig-delivery_roulier branch 2 times, most recently from e569ac5 to 71d8f32 Compare June 18, 2024 08:28

vvrossem approved these changes Jun 18, 2024

View reviewed changes

delivery_roulier/tests/test_delivery_roulier.py Outdated Show resolved Hide resolved

yankinmax force-pushed the 17.0-mig-delivery_roulier branch from 71d8f32 to c98ece8 Compare June 18, 2024 11:49

ivantodorovich approved these changes Jun 27, 2024

View reviewed changes

OCA-git-bot added the approved label Jun 27, 2024

Camille0907 approved these changes Jun 28, 2024

View reviewed changes

delivery_roulier/models/stock_move_line.py Outdated Show resolved Hide resolved

rousseldenis reviewed Jun 28, 2024

View reviewed changes

delivery_roulier/models/stock_move_line.py Outdated Show resolved Hide resolved

yankinmax force-pushed the 17.0-mig-delivery_roulier branch from c98ece8 to 82d3d6a Compare June 28, 2024 12:41

yankinmax commented Jun 28, 2024

View reviewed changes

OCA-git-bot removed the approved label Jul 9, 2024

florian-dacosta suggested changes Jul 9, 2024

View reviewed changes

yankinmax force-pushed the 17.0-mig-delivery_roulier branch from 82d3d6a to 0fa989c Compare July 9, 2024 11:27

mymage and others added 3 commits July 9, 2024 18:25

Added translation using Weblate (Italian)

828a454

Translated using Weblate (Italian)

c0bb4cd

Currently translated at 100.0% (13 of 13 strings) Translation: delivery-carrier-16.0/delivery-carrier-16.0-delivery_roulier Translate-URL: https://translation.odoo-community.org/projects/delivery-carrier-16-0/delivery-carrier-16-0-delivery_roulier/it/

[IMP] delivery_roulier: pre-commit auto fixes

4d2effc

yankinmax force-pushed the 17.0-mig-delivery_roulier branch from 0fa989c to 225eaa1 Compare July 9, 2024 15:26

florian-dacosta reviewed Jul 9, 2024

View reviewed changes

delivery_roulier/__manifest__.py Outdated Show resolved Hide resolved

florian-dacosta approved these changes Jul 9, 2024

View reviewed changes

OCA-git-bot added the approved label Jul 9, 2024

simahawk requested changes Jul 11, 2024

View reviewed changes

delivery_roulier/models/delivery_carrier.py Outdated Show resolved Hide resolved

delivery_roulier/tests/test_delivery_roulier.py Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

OCA-git-bot removed the approved label Jul 11, 2024

OCA-git-bot added this to the 17.0 milestone Jul 11, 2024

simahawk approved these changes Jul 12, 2024

View reviewed changes

OCA-git-bot added the approved label Jul 12, 2024

yankinmax and others added 2 commits July 12, 2024 10:15

[MIG] delivery_roulier: Migration to 17.0

881d2ab

[FIX] delivery_roulier : get_tracking_link should return an url inste…

9b07dd9

…ad of a list of urls

yankinmax force-pushed the 17.0-mig-delivery_roulier branch from e0820aa to 9b07dd9 Compare July 12, 2024 07:16

OCA-git-bot added the bot is merging ⏳ label Jul 12, 2024

OCA-git-bot merged commit 8f86277 into OCA:17.0 Jul 12, 2024
5 of 7 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[17.0][MIG] delivery_roulier: Migration to 17.0 #826

[17.0][MIG] delivery_roulier: Migration to 17.0 #826

yankinmax commented Jun 4, 2024 •

edited by gurneyalex

Loading

Camille0907 left a comment

yankinmax Jun 28, 2024 •

edited

Loading

florian-dacosta Jul 9, 2024

yankinmax Jul 9, 2024

florian-dacosta Jul 9, 2024

yankinmax commented Jul 8, 2024

florian-dacosta Jul 9, 2024

yankinmax commented Jul 9, 2024

florian-dacosta commented Jul 9, 2024

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024

florian-dacosta commented Jul 9, 2024

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024 •

edited

Loading

florian-dacosta commented Jul 9, 2024 •

edited

Loading

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024

florian-dacosta commented Jul 9, 2024

yankinmax commented Jul 10, 2024

simahawk commented Jul 11, 2024

yankinmax commented Jul 11, 2024

simahawk left a comment

simahawk commented Jul 12, 2024

OCA-git-bot commented Jul 12, 2024

simahawk commented Jul 12, 2024

OCA-git-bot commented Jul 12, 2024

OCA-git-bot commented Jul 12, 2024

[17.0][MIG] delivery_roulier: Migration to 17.0 #826

[17.0][MIG] delivery_roulier: Migration to 17.0 #826

Conversation

yankinmax commented Jun 4, 2024 • edited by gurneyalex Loading

Camille0907 left a comment

Choose a reason for hiding this comment

yankinmax Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

florian-dacosta Jul 9, 2024

Choose a reason for hiding this comment

yankinmax Jul 9, 2024

Choose a reason for hiding this comment

florian-dacosta Jul 9, 2024

Choose a reason for hiding this comment

yankinmax commented Jul 8, 2024

florian-dacosta Jul 9, 2024

Choose a reason for hiding this comment

yankinmax commented Jul 9, 2024

florian-dacosta commented Jul 9, 2024

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024

florian-dacosta commented Jul 9, 2024

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024 • edited Loading

florian-dacosta commented Jul 9, 2024 • edited Loading

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024

yankinmax commented Jul 9, 2024

florian-dacosta commented Jul 9, 2024

yankinmax commented Jul 10, 2024

simahawk commented Jul 11, 2024

yankinmax commented Jul 11, 2024

simahawk left a comment

Choose a reason for hiding this comment

simahawk commented Jul 12, 2024

OCA-git-bot commented Jul 12, 2024

simahawk commented Jul 12, 2024

OCA-git-bot commented Jul 12, 2024

OCA-git-bot commented Jul 12, 2024

yankinmax commented Jun 4, 2024 •

edited by gurneyalex

Loading

yankinmax Jun 28, 2024 •

edited

Loading

yankinmax commented Jul 9, 2024 •

edited

Loading

florian-dacosta commented Jul 9, 2024 •

edited

Loading