[16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup #799

jbaudoux · 2024-04-17T21:00:54Z

In summary:

Cleanup code (use a single context variable to prevent re-execution, make dedicated methods for each purpose).
Increase performance (there was many recursive execution of the same delete/add line - code is now executed once and only what needs to be modified).
Make the module compatible with sale_order_carrier_auto_assign. Move carrier assignment to that module so that this module concentrate on cost delivery line update (i.e. refresh as from its name). See [16.0][IMP] sale_order_carrier_auto_assign: assign carrier on create sale-workflow#3081 Marked as migration v17.0 improvement
Make module more robust. Implement SO line create/write

Depends on:

~~[16.0][IMP] sale_order_carrier_auto_assign: assign carrier on create sale-workflow#3081~~

Settings:

In details (one commit for each) - in reverse order:

config parameters moved to company. "sale_" prefix added to the parameters
add tests from [14.0] [FIX] delivery_auto_refresh: dynamic carrier line #790 & [14.0] [FIX] delivery_auto_refresh: manually delete delivery line #791
improve help & readme
refresh on SO line create/write
set carrier: ~~use [16.0][IMP] sale_order_carrier_auto_assign: assign carrier on create sale-workflow#3081~~ integrated here and marked to move on v17.0
refresh the minimum: Create or delete line only if necessary. Update existing line if necessary
iterate over list, not set
fix write & discount
Don't pass discount variable in context
fix create in batch (api.model_create_multi instead of api.model)
docstring: Don't modify standard method docstring

Compatibility checks/fixes with other modules triggering refresh on SO create/write:

cc @pedrobaeza @santostelmo @yvaucher @simahawk @francesco-ooops @renda-dev

Don't modify standard method docstring

Limit change of write to calling the auto refresh feature Prevent to call auto refresh multiple times Don't pass discount variable in context

pedrobaeza · 2024-04-18T06:38:52Z

delivery_auto_refresh/__manifest__.py

    "category": "Delivery",
    "website": "https://github.com/OCA/delivery-carrier",
    "author": "Tecnativa, Odoo Community Association (OCA)",
    "license": "AGPL-3",
    "application": False,
    "installable": True,
-    "depends": ["delivery"],
+    "depends": ["delivery", "sale_order_carrier_auto_assign"],


You can't add a new dependency in this module in stable, but also because those using this module since the beginning don't want to have a new one.

Luckily it is in Beta stage and not Stable/Production
Don't you find it better to have the features implemented in the proper corresponding modules?

It's not really beta. It's that nobody has changed its maturity level (because nobody is worrying about that). I prefer to have it as is, and do the change on the next version.

@pedrobaeza I removed the dependency. I marked the code to drop in v17.0. I placed this in a separate commit.

I still have to change the config parameters to make them configurable per company

OK, thanks for the efforts. Please include migration script for converting them smoothly out of the system parameters.

for sure :)
It's done

delivery_auto_refresh/migrations/16.0.2.0.0/post-migration.py

delivery_auto_refresh/tests/test_delivery_auto_refresh.py

delivery_auto_refresh/models/res_config_settings.py

delivery_auto_refresh/models/sale_order.py

jbaudoux · 2024-04-18T13:28:53Z

Config parameters moved to company. sale_ prefix added to the parameters

jbaudoux · 2024-04-18T14:05:23Z

Ready for final review

delivery_auto_refresh/models/sale_order.py

pedrobaeza · 2024-04-18T18:44:32Z

delivery_auto_refresh/models/sale_order.py

+        if self.env.context.get("auto_refresh_delivery"):
+            return
+        if not self._is_auto_add_delivery_line():
+            return
+        if self.state not in ("draft", "sent"):
+            return


Why not compacting them for less lines and better readability?

Suggested change

if self.env.context.get("auto_refresh_delivery"):

return

if not self._is_auto_add_delivery_line():

return

if self.state not in ("draft", "sent"):

return

if (

self.env.context.get("auto_refresh_delivery")

or not self._is_auto_add_delivery_line()

or self.state not in ("draft", "sent")

):

return

pedrobaeza · 2024-04-18T18:45:01Z

delivery_auto_refresh/models/sale_order.py

+        if not self.carrier_id:
+            self._remove_delivery_line()
+        elif self.is_all_service:
+            self._remove_delivery_line()


Suggested change

if not self.carrier_id:

self._remove_delivery_line()

elif self.is_all_service:

self._remove_delivery_line()

if not self.carrier_id or self.is_all_service:

self._remove_delivery_line()

delivery_auto_refresh/models/sale_order.py

delivery_auto_refresh/models/sale_order_line.py

pedrobaeza · 2024-04-18T18:48:55Z

delivery_auto_refresh/models/sale_order_line.py

+        return fields
+
+    @api.model_create_multi
+    def create(self, vals_list):


Why all the create/write overrides? It seems a great overhead.

For implementing the missing feature from the roadmap :)
* This is only working from user interface, as there's no way of making compatible the auto-refresh intercepting create/write methods from sale order lines.

Create or delete line only if necessary Update existing line if necessary

This commit can be reverted in the migration to 17.0

Move config to company level Prefix config settings by sale

cyrilmanuel · 2024-06-13T07:16:20Z

hi @pedrobaeza could you review this PR please ?

pedrobaeza

Thanks for your patience, Jacques. Apart from the comments inline (which there can be several occurrences, but I have written only once):

As this code was originally mine, I would appreciate that you follow my coding style not adding empty lines inside a method (it's also something recommended - but not enforced - in Python style guides).

There are docstring changed to comments.

pedrobaeza · 2024-06-13T19:26:44Z

delivery_auto_refresh/models/sale_order.py

@@ -130,7 +196,7 @@ def _is_delivery_line_voidable(self):
        # nothing to be done either. If there are more than one delivery lines
        # we won't be doing anything as well.
        if (
-            self.state not in {"done", "sale"}
+            self.state not in ("done", "sale")


The set was correct for faster operation. Why changing it?

I may answer: the usage of set or list is kind of wrong in all the cases where you have a fixed set of values and you don't need to purge duplicates or manipulate values afterwards.
Plus, have a look at the memory size:

>>> sys.getsizeof(("done", "sale")) 56 >>> sys.getsizeof({"done", "sale"}) 216

In fact, sounds like something that the linter could prevent :)

Yet, such changes might be applied afterwards to reduce diff size in the future. You have a point here 😉

@yajo do you have at hand the times tests where sets beats inclusion comparison against lists? This is something of performance, not memory size.

@pedrobaeza is right that sets are better optimized for inclusion comparison. But this is only relevant if the set is big enough.

>>> def in_test(iterable): ... for i in range(10): ... if i in iterable: ... pass ... >>> from timeit import timeit >>> timeit( ... "in_test(iterable)", ... setup="from __main__ import in_test; iterable = set(range(5))", ... number=10000) 0.030677038012072444 >>> timeit( ... "in_test(iterable)", ... setup="from __main__ import in_test; iterable = list(range(5))", ... number=10000) 0.014917187043465674 >>> timeit( ... "in_test(iterable)", ... setup="from __main__ import in_test; iterable = tuple(range(5))", ... number=10000) 0.009921401971951127

Hi there! Your test is doomed:

>>> from timeit import timeit >>> def in_test(iterable): ... return 1000 in iterable >>> timeit("in_test(iterable)", setup="from __main__ import in_test; iterable = set(range(5))") 0.06247029999940423 >>> timeit("in_test(iterable)", setup="from __main__ import in_test; iterable = list(range(5))") 0.11310941600095248 >>> timeit("in_test(iterable)", setup="from __main__ import in_test; iterable = tuple(range(5))") 0.1125952110014623

Using set for an in construct is always faster. In a list/tuple of 5 elements, it's still 50% faster.

For this use case, the tuple contains 2 elements. Let's see:

>>> timeit("in_test(iterable)", setup="from __main__ import in_test; iterable = set(range(2))") 0.07049787299911259 >>> timeit("in_test(iterable)", setup="from __main__ import in_test; iterable = list(range(2))") 0.07827233200077899 >>> timeit("in_test(iterable)", setup="from __main__ import in_test; iterable = tuple(range(2))") 0.07647827899927506

Using set is still faster, although barely noticeable. This means that, if you see a pattern such as a in (0, 1), it's probably not worth it to optimize it. But if you see one like a in {0, 1}, it's definitely not worth it un-optimizing it to use a tuple.

It's not a matter of % of improvement, but of algorithmic complexity. Using a set will be of O1 complexity (constant), while using a list or tuple will be On complexity (linear). Thus, using a set is the default rule of thumb here.

It's not worth reverting this change for such a small improvement now that it's merged, but at least I hope you learned something today 😄

delivery_auto_refresh/models/sale_order.py

delivery_auto_refresh/models/res_config_settings.py

pedrobaeza

Thanks again for your efforts.

/ocabot merge nobump

OCA-git-bot · 2024-06-14T15:40:16Z

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-799-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2024-06-14T15:47:22Z

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

jbaudoux · 2024-06-14T17:11:44Z

Thanks again for your efforts.

@pedrobaeza Thanks for your time on this review :)

rafaelbn · 2024-06-17T11:18:33Z

This PR affects MT-5997 @moduon and #815 which is being reviewed by @yajo

@Gelojr @fcvalgar please deep functional review 😄 ❤️

rafaelbn · 2024-06-17T11:41:26Z

Hello @jbaudoux ,

We don't want to use sale_order_carrier_auto_assign module. It's mandatory?

We wouldn't be forced yo use it 😢

Let me know please! 😄

Thank you!

jbaudoux · 2024-06-17T11:56:07Z

Hello @jbaudoux ,

We don't want to use sale_order_carrier_auto_assign module. It's mandatory?

We wouldn't be forced yo use it 😢.

Let me know please! 😄

Thank you!

There is no new dependency in v16.

rafaelbn · 2024-06-17T12:26:50Z

OK, sorry. I miss some comments 🙏🏼 . Thank you! 😄

yajo · 2024-06-17T12:43:05Z

It seems v16 settings layout broke:

rousseldenis · 2024-07-17T12:24:44Z

It seems v16 settings layout broke:

@jbaudoux

toita86 · 2024-07-17T15:13:13Z

@rousseldenis I'm not sure if it can help, but this is my attempt to fix the layout e6214da

jbaudoux added 4 commits April 16, 2024 20:07

delivery_auto_refresh: docstring

49d2e7b

Don't modify standard method docstring

delivery_auto_refresh: fix create in batch

8a44424

delivery_auto_refresh: fix write & discount

63fbcb1

Limit change of write to calling the auto refresh feature Prevent to call auto refresh multiple times Don't pass discount variable in context

delivery_auto_refresh: iterate over list, not set

bff30d4

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch 2 times, most recently from 71f141e to 568d49a Compare April 17, 2024 21:18

jbaudoux mentioned this pull request Apr 17, 2024

[16.0][FIX] delivery_auto_refresh: dynamic carrier line #797

Closed

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch from 568d49a to 46041b5 Compare April 17, 2024 21:26

jbaudoux marked this pull request as ready for review April 17, 2024 21:43

jbaudoux changed the title ~~[16.0][IMP-FIX] delivery_aut_refresh~~ [16.0][IMP-FIX] delivery_auto_refresh - code cleanup Apr 17, 2024

jbaudoux changed the title ~~[16.0][IMP-FIX] delivery_auto_refresh - code cleanup~~ [16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup Apr 17, 2024

jbaudoux mentioned this pull request Apr 17, 2024

[15.0][FIX] delivery_auto_refresh: dynamic carrier line #796

Closed

pedrobaeza requested changes Apr 18, 2024

View reviewed changes

jbaudoux mentioned this pull request Apr 18, 2024

[16.0][IMP] sale_order_carrier_auto_assign: assign carrier on create OCA/sale-workflow#3081

Closed

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch 3 times, most recently from b73434f to 1d19556 Compare April 18, 2024 10:46

simahawk reviewed Apr 18, 2024

View reviewed changes

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch 3 times, most recently from a142e47 to 5ac2bf6 Compare April 18, 2024 13:28

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch from 5ac2bf6 to 59b6e2c Compare April 18, 2024 13:51

pedrobaeza added this to the 16.0 milestone Apr 18, 2024

pedrobaeza reviewed Apr 18, 2024

View reviewed changes

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch from 59b6e2c to d3a54a1 Compare April 19, 2024 10:12

jbaudoux added 3 commits April 19, 2024 12:22

delivery_auto_refresh: refresh the minimum

4e803c7

Create or delete line only if necessary Update existing line if necessary

delivery_auto_refresh: set carrier

b638f3c

delivery_auto_refresh: refresh on SO line create/write

e432af5

jbaudoux added 2 commits April 19, 2024 12:22

delivery_auto_refresh: improve help & readme

e561393

delivery_auto_refresh: add tests

cb54e44

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch from d3a54a1 to 4c45d35 Compare April 19, 2024 10:22

jbaudoux added 2 commits April 19, 2024 16:20

delivery-carrier: postpone new dependency to 17.0

5838619

This commit can be reverted in the migration to 17.0

delivery_auto_refresh: multi-company

516b510

Move config to company level Prefix config settings by sale

jbaudoux force-pushed the 16-fix-delivery_auto_refresh branch from 4c45d35 to 516b510 Compare April 19, 2024 14:20

pedrobaeza reviewed Jun 13, 2024

View reviewed changes

pedrobaeza approved these changes Jun 14, 2024

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Jun 14, 2024

OCA-git-bot merged commit b2e5168 into OCA:16.0 Jun 14, 2024
4 of 5 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Jun 14, 2024

This was referenced Jun 17, 2024

[FIX] delivery_auto_refresh: compatibility with sale_tier_validation #815

Closed

base_tier_validation: blocking notifications cron, a.k.a. should it block sudo writes? OCA/server-ux#875

Closed

toita86 mentioned this pull request Jul 4, 2024

[14.0][FIX] delivery_auto_refresh #849

Closed

rafaelbn mentioned this pull request Jul 19, 2024

[FIX] base_tier_validation: allow sudo writes OCA/server-ux#876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup #799

[16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup #799

jbaudoux commented Apr 17, 2024 •

edited

Loading

pedrobaeza Apr 18, 2024

jbaudoux Apr 18, 2024

pedrobaeza Apr 18, 2024

jbaudoux Apr 18, 2024

pedrobaeza Apr 18, 2024

jbaudoux Apr 18, 2024

jbaudoux commented Apr 18, 2024

jbaudoux commented Apr 18, 2024

pedrobaeza Apr 18, 2024

pedrobaeza Apr 18, 2024

pedrobaeza Apr 18, 2024

jbaudoux Apr 19, 2024 •

edited

Loading

cyrilmanuel commented Jun 13, 2024

pedrobaeza left a comment

pedrobaeza Jun 13, 2024

simahawk Jun 14, 2024 •

edited

Loading

pedrobaeza Jun 14, 2024

jbaudoux Jun 14, 2024 •

edited

Loading

yajo Jun 17, 2024

pedrobaeza left a comment

OCA-git-bot commented Jun 14, 2024

OCA-git-bot commented Jun 14, 2024

jbaudoux commented Jun 14, 2024

rafaelbn commented Jun 17, 2024

rafaelbn commented Jun 17, 2024

jbaudoux commented Jun 17, 2024

rafaelbn commented Jun 17, 2024

yajo commented Jun 17, 2024

rousseldenis commented Jul 17, 2024

toita86 commented Jul 17, 2024

[16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup #799

[16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup #799

Conversation

jbaudoux commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux commented Apr 18, 2024

jbaudoux commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

cyrilmanuel commented Jun 13, 2024

pedrobaeza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simahawk Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaudoux Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Jun 14, 2024

OCA-git-bot commented Jun 14, 2024

jbaudoux commented Jun 14, 2024

rafaelbn commented Jun 17, 2024

rafaelbn commented Jun 17, 2024

jbaudoux commented Jun 17, 2024

rafaelbn commented Jun 17, 2024

yajo commented Jun 17, 2024

rousseldenis commented Jul 17, 2024

toita86 commented Jul 17, 2024

jbaudoux commented Apr 17, 2024 •

edited

Loading

jbaudoux Apr 19, 2024 •

edited

Loading

simahawk Jun 14, 2024 •

edited

Loading

jbaudoux Jun 14, 2024 •

edited

Loading