[16.0][MIG] sale_loyalty_auto_refresh: Migration to 16.0 #208

nguyenminhchien · 2024-03-21T03:15:37Z

Rename from sale_coupon_auto_refresh to sale_loyalty_auto_refresh

cyrilmanuel

LGTM

OCA-git-bot · 2024-03-26T10:23:25Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

cyrilmanuel · 2024-03-27T06:43:38Z

Hi @pedrobaeza can you merge this PR please ?

pedrobaeza · 2024-03-27T07:20:51Z

/ocabot migration sale_loyalty_auto_refresh

@chienandalu do you see it correct?

chienandalu

Thanks for your work @nguyenminhchien 🙂 Just some comments:

chienandalu · 2024-03-27T07:48:45Z

sale_loyalty_auto_refresh/models/sale_coupon_refresh_mixin.py

-            .get_param("sale_coupon_auto_refresh.%s_triggers" % (self._table), "")
+            .get_param("sale_loyalty_auto_refresh.%s_triggers" % (self._table), "")


chore: there should be a migration script renaming this parameter

chienandalu · 2024-03-27T07:54:23Z

sale_loyalty_auto_refresh/models/sale_order.py

    def _auto_refresh_coupons(self):
        orders = self.filtered(type(self)._allow_recompute_coupon_lines)
        if orders:
-            orders = orders.with_context(skip_auto_refresh_coupons=True)
-            orders.recompute_coupon_lines()
+            orders.action_open_reward_wizard()


I'm not sure about this change of behavior. Why not using the new _update_programs_and_rewards method?

i tried calling it, but it didn't work as expected. anw, i made some changes to deal with the case there was more than 1 applicable promotion. please review.

chienandalu · 2024-03-27T08:01:01Z

sale_loyalty_auto_refresh/tests/test_sale_loyalty_auto_refresh.py

+# Part of Odoo. See LICENSE file for full copyright and licensing details.
+
+from odoo.tests import new_test_user, tagged
+
+from odoo.addons.sale_loyalty.tests.common import TestSaleCouponCommon


It'd be nice to have a diff here. Aside from that, at first glance I see lot of changes:

Copyright and licences aren't respected from the original

Tests cases are trimmed so you're not testing the same effects as before, so avoiding regression isn't granted or hard to tell...

Sorry about the Copyright part, i may remove it by mistake.
I just add 1 more unit test to check the case of having more than 1 applicable promotion.

cyrilmanuel · 2024-03-28T08:04:31Z

Hello @chienandalu is it good for you ? :)

chienandalu

Hi @nguyenminhchien some comments / doubts:

chienandalu · 2024-04-03T08:43:56Z

sale_loyalty_auto_refresh/tests/test_sale_loyalty_auto_refresh.py

The test cases are completely changed and can't see a clear reason why...

because the models of coupon and promotion were changed dramatically. so i trimmed the test to make it more focus on testing the applying of the promotion auto, than re-test the promotion flow, which has been done by sale_loyalty. anw, i'm going to roll it back.

chienandalu · 2024-04-03T08:50:41Z

sale_loyalty_auto_refresh/models/sale_order.py

+        self_ctx = self.with_context(skip_auto_refresh_coupons=True)
+        for order in self_ctx:
+            if order._allow_recompute_coupon_lines():
+                order._update_programs_and_rewards()
+                order.action_apply_rewards()


There's no real need to change the previous structure. Yo can:

Suggested change

self_ctx = self.with_context(skip_auto_refresh_coupons=True)

for order in self_ctx:

if order._allow_recompute_coupon_lines():

order._update_programs_and_rewards()

order.action_apply_rewards()

orders = self.with_context(skip_auto_refresh_coupons=True).filtered(type(self)._allow_recompute_coupon_lines)

for order in orders:

order._update_programs_and_rewards()

order.action_apply_rewards()

chienandalu · 2024-04-03T08:54:20Z

sale_loyalty_auto_refresh/models/sale_order.py

+                # Ignore exception errors to unblock the user when creating/writing
+                logger.warning(e)


This is going to be really noisy. I you need to do it, better just log it in debug mode.

cyrilmanuel · 2024-04-16T06:58:46Z

@chienandalu is it good for you ?

simahawk

Non blocking comments, only code review.
@chienandalu ping :)

simahawk · 2024-05-22T05:57:14Z

sale_loyalty_auto_refresh/models/sale_order.py

+
+        self_ctx = self.with_context(skip_auto_refresh_coupons=True)
+        lines = super(SaleOrderLine, self_ctx).create(vals_list)
+        lines.mapped("order_id")._auto_refresh_coupons()


Suggested change

lines.mapped("order_id")._auto_refresh_coupons()

lines.order_id._auto_refresh_coupons()

simahawk · 2024-05-22T05:59:06Z

sale_loyalty_auto_refresh/models/sale_order.py

+        """Returns whether reward lines in order ``self`` can be recomputed
+        automatically.


Suggested change

"""Returns whether reward lines in order ``self`` can be recomputed

automatically.

"""

Check if reward lines in ``self`` can be recomputed automatically.

simahawk · 2024-05-22T06:00:06Z

sale_loyalty_auto_refresh/models/sale_order.py

+            return super().write(vals)
+
+        old_data = self._read_recs_data()
+        old_orders = self.mapped("order_id")


Suggested change

old_orders = self.mapped("order_id")

old_orders = self.order_id

simahawk · 2024-05-22T06:00:17Z

sale_loyalty_auto_refresh/models/sale_order.py

+        self_ctx = self.with_context(skip_auto_refresh_coupons=True)
+        res = super(SaleOrderLine, self_ctx).write(vals)
+        new_data = self._read_recs_data()
+        new_orders = self.mapped("order_id")


Suggested change

new_orders = self.mapped("order_id")

new_orders = self.order_id

simahawk · 2024-05-22T06:00:43Z

sale_loyalty_auto_refresh/models/sale_order.py

+        if self._check_skip_refresh():
+            return super().unlink()
+
+        orders = self.mapped("order_id")


Suggested change

orders = self.mapped("order_id")

orders = self.order_id

simahawk · 2024-05-22T06:02:33Z

sale_loyalty_auto_refresh/readme/CONFIGURE.rst

+#. In every add the fields seperated by commas that you want to add to the recomputation
+   triggers.
+
+⚠️ After configuring or removing a trigger a restart of Odoo is recommended so the


I don't understand this statement. Either is required or is not. I'd remove it if not necessary because it's confusing.
I left a comment on the compute method.

simahawk · 2024-05-22T06:11:12Z

sale_loyalty_auto_refresh/models/sale_coupon_refresh_mixin.py

+        )
+        return {x for x in additional_triggers if x in self._fields}
+
+    @api.depends(lambda self: list(self._get_auto_refresh_coupons_triggers()))


Are we sure this complexity w/ the check on new triggers is necessary?
Data:

the field you compute is non stored -> will get recomputed at every request and you can invalidate caching if needed

any change on the config param won't have any effect on this depends

in fact what you are doing is to load the triggers at need (eg: create/write)

Wouldn't be better to have an empty api.depends?

santostelmo · 2024-06-28T06:35:52Z

@chienandalu ping

santostelmo · 2024-06-28T06:39:55Z

@SilvioC2C maybe you can help on the review / merge of this PR as it's been under review for some time.

rousseldenis · 2024-06-28T06:54:43Z

sale_loyalty_auto_refresh/models/sale_order.py

+            try:
+                self._apply_program_reward(reward, coupon)
+                self._update_programs_and_rewards()
+            except Exception as e:


Isn't it a too broad except ?

yes, but here, i want to ignore all exception errors to unblock the user when creating/writing

anw, i updated to catch UserError, ValidationError only.

SilvioC2C

Overall LGTM, a few comments here and there

SilvioC2C · 2024-06-28T07:20:54Z

sale_loyalty_auto_refresh/migrations/post-migration.py

+        [("key", "like", "sale_coupon_auto_refresh.")]
+    )
+    for parameter in parameters:
+        parameter.key = (parameter.key).replace(


Suggested change

parameter.key = (parameter.key).replace(

parameter.key = parameter.key.replace(

just curious but what's different or benefit of this suggested change?

Better readability 😄 parenthesis are redundant in this case

SilvioC2C · 2024-06-28T07:49:06Z

sale_loyalty_auto_refresh/models/sale_order.py

+            order._update_programs_and_rewards()
+            order.action_apply_rewards()


Isn't _update_programs_and_rewards() already called by action_apply_rewards() internally? Do we need this double call?

yes, we do. The first call to updates applied with the current state of the order. Then, after calling _apply_program_reward(), need to call it again.

SilvioC2C · 2024-06-28T07:49:47Z

sale_loyalty_auto_refresh/models/sale_order.py

+            try:
+                self._apply_program_reward(reward, coupon)
+                self._update_programs_and_rewards()
+            except Exception as e:


Please catch any possible psycopg2 exception and let them be raised, it'd be better to not ignore DB-level errors.

SilvioC2C · 2024-06-28T07:51:08Z

sale_loyalty_auto_refresh/models/sale_order.py

+        )._update_programs_and_rewards()
+
+
+class SaleOrderLine(models.Model):


Could be worth it to move SaleOrderLine to its own file sale_order_line.py.

Well I've seen different approaches regarding that. Some people prefer to keep it in the parent object file (sale_order.py). I am okay to keep like that. But move SaleOrderLine to his own file it's an option as well

Yeah, usually if I have something like:

from odoo import fields, models class MyModel(models.Model): _name = "my.model" name = fields.Char() line_ids = fields.One2many("my.model.line", "rec_id") class MyModel(models.Model): _name = "my.model" descr = fields.Char() rec_id = fields.Many2one("my.model")

then it can make sense to have both classes in the same file.

But in this module's case, file's ~150 LoC already, and both classes contain business logic... might as well separate them for improved readability.

Currently translated at 6.2% (1 of 16 strings) Translation: sale-promotion-14.0/sale-promotion-14.0-sale_coupon_auto_refresh Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-14-0/sale-promotion-14-0-sale_coupon_auto_refresh/it/

This can allow us to work altogether with flexible triggers like the ones we could need with sale_coupon_criteria_order_based TT37479

We should not refresh the lines before the proccess of applying the coupon is finished. TT38806

TT37307

Use the new `get_depends()` method as the former `depends` attribute (now in `_depends`) is no longer reliable.

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: sale-promotion-15.0/sale-promotion-15.0-sale_coupon_auto_refresh Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-15-0/sale-promotion-15-0-sale_coupon_auto_refresh/

Currently translated at 100.0% (13 of 13 strings) Translation: sale-promotion-15.0/sale-promotion-15.0-sale_coupon_auto_refresh Translate-URL: https://translation.odoo-community.org/projects/sale-promotion-15-0/sale-promotion-15-0-sale_coupon_auto_refresh/es/

cyrilmanuel · 2024-07-08T09:45:32Z

Hi @rousseldenis is it good for you ?

github-actions · 2024-11-10T12:35:54Z

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.

santostelmo · 2024-11-11T04:46:32Z

@simahawk is possible to merge that one ? 4 approvals

simahawk · 2024-11-12T07:44:57Z

/ocabot merge nobump

OCA-git-bot · 2024-11-12T07:45:05Z

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

OCA-git-bot · 2024-11-12T07:51:32Z

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

nguyenminhchien marked this pull request as ready for review March 21, 2024 03:19

santostelmo approved these changes Mar 21, 2024

View reviewed changes

cyrilmanuel approved these changes Mar 26, 2024

View reviewed changes

OCA-git-bot added approved ready to merge labels Mar 26, 2024

OCA-git-bot added this to the 16.0 milestone Mar 27, 2024

OCA-git-bot mentioned this pull request Mar 27, 2024

Migration to version 16.0 #80

Open

15 tasks

chienandalu suggested changes Mar 27, 2024

View reviewed changes

OCA-git-bot removed approved ready to merge labels Mar 27, 2024

nguyenminhchien force-pushed the 16.0-mig-sale_coupon_auto_refresh branch 2 times, most recently from 321bcd8 to 0fb6824 Compare March 27, 2024 10:57

nguyenminhchien requested a review from chienandalu March 27, 2024 11:14

chienandalu suggested changes Apr 3, 2024

View reviewed changes

nguyenminhchien force-pushed the 16.0-mig-sale_coupon_auto_refresh branch 2 times, most recently from 7aca846 to f4d66d1 Compare April 4, 2024 11:41

nguyenminhchien requested a review from chienandalu April 4, 2024 11:46

cyrilmanuel approved these changes Apr 16, 2024

View reviewed changes

jbaudoux mentioned this pull request Apr 17, 2024

[16.0][IMP-FIX] delivery_auto_refresh - performance & compatibility & cleanup OCA/delivery-carrier#799

Merged

4 tasks

QuocDuong1306 mentioned this pull request May 22, 2024

[16.0][IMP] shopinvader_api_sale_loyalty: do not update rewards when reading a sale shopinvader/odoo-shopinvader#1543

Merged

simahawk approved these changes May 22, 2024

View reviewed changes

rousseldenis reviewed Jun 28, 2024

View reviewed changes

SilvioC2C approved these changes Jun 28, 2024

View reviewed changes

OCA-git-bot and others added 15 commits June 28, 2024 17:08

sale_coupon_auto_refresh 14.0.1.0.3

b90ca22

Added translation using Weblate (Italian)

f4f729f

[IMP] sale_coupon_auto_refresh: configurable triggers

4f7cd8e

This can allow us to work altogether with flexible triggers like the ones we could need with sale_coupon_criteria_order_based TT37479

sale_coupon_auto_refresh 14.0.2.0.0

cabe680

sale_coupon_auto_refresh: safely apply coupons

30f433d

We should not refresh the lines before the proccess of applying the coupon is finished. TT38806

sale_coupon_auto_refresh 14.0.2.0.1

ae88a16

[MIG] sale_coupon_auto_refresh: Migration to version 15.0

c277507

TT37307

[FIX] sale_coupon_auto_refresh: tests

b86308b

Use the new `get_depends()` method as the former `depends` attribute (now in `_depends`) is no longer reliable.

[UPD] Update sale_coupon_auto_refresh.pot

8f8d8c6

[UPD] README.rst

b282b94

[UPD] README.rst

7d504d8

[IMP] sale_coupon_auto_refresh: pre-commit stuff

dd347c2

nguyenminhchien force-pushed the 16.0-mig-sale_coupon_auto_refresh branch from f4d66d1 to e48ae33 Compare June 28, 2024 10:08

[MIG] sale_loyalty_auto_refresh: Migration to 16.0

fb20963

nguyenminhchien force-pushed the 16.0-mig-sale_coupon_auto_refresh branch from e48ae33 to fb20963 Compare June 28, 2024 10:15

nguyenminhchien requested a review from rousseldenis June 28, 2024 10:27

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

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

OCA-git-bot merged commit a0a3dbb into OCA:16.0 Nov 12, 2024
4 of 7 checks passed

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

		.get_param("sale_coupon_auto_refresh.%s_triggers" % (self._table), "")
		.get_param("sale_loyalty_auto_refresh.%s_triggers" % (self._table), "")

		# Ignore exception errors to unblock the user when creating/writing
		logger.warning(e)

	lines.mapped("order_id")._auto_refresh_coupons()
	lines.order_id._auto_refresh_coupons()

		"""Returns whether reward lines in order ``self`` can be recomputed
		automatically.

	old_orders = self.mapped("order_id")
	old_orders = self.order_id

	new_orders = self.mapped("order_id")
	new_orders = self.order_id

	parameter.key = (parameter.key).replace(
	parameter.key = parameter.key.replace(

		order._update_programs_and_rewards()
		order.action_apply_rewards()

		)._update_programs_and_rewards()


		class SaleOrderLine(models.Model):

[16.0][MIG] sale_loyalty_auto_refresh: Migration to 16.0 #208

[16.0][MIG] sale_loyalty_auto_refresh: Migration to 16.0 #208

Conversation

nguyenminhchien commented Mar 21, 2024

cyrilmanuel left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Mar 26, 2024

cyrilmanuel commented Mar 27, 2024

pedrobaeza commented Mar 27, 2024

chienandalu left a 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

Choose a reason for hiding this comment

cyrilmanuel commented Mar 28, 2024

chienandalu left a 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

cyrilmanuel commented Apr 16, 2024

simahawk left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simahawk May 22, 2024 • edited Loading

Choose a reason for hiding this comment

santostelmo commented Jun 28, 2024

santostelmo commented Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SilvioC2C left a 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

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

cyrilmanuel commented Jul 8, 2024

github-actions bot commented Nov 10, 2024

santostelmo commented Nov 11, 2024

simahawk commented Nov 12, 2024

OCA-git-bot commented Nov 12, 2024

OCA-git-bot commented Nov 12, 2024

simahawk May 22, 2024 •

edited

Loading

santostelmo commented Jun 28, 2024 •

edited

Loading