-
-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[17.0][ADD] pos_loyalty_card_fixed_expiration_date: New module pos_loyalty_card_fixed_expiration_date #1287
Conversation
e6a951e
to
845b497
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Code review and tested in runboat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested in runboat and reviewed code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR has the |
…card_fixed_expiration_date
845b497
to
9e2ce75
Compare
Hi @ivantodorovich, could you review please. Thanks! |
# If the expiration date is not set, we set it to | ||
# the current date + the fixed duration | ||
if vals_list: | ||
if not vals_list[0].get("expiration_date", False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right.
vals_list
here can be a list containing completely different values. You shouldn't rely only on the first one, for all the records.
Instead you can do something like this:
for record, vals in zip(res, vals_list):
...
This works because create will return the records on the same order than the provided values that created them.
But moreover, I wonder if it wouldn't be a better idea to simply make the expiration_date
a computed field with store=True
, readonly=False
, precompute=True
, and @api.depends('program_id')
, instead of overriding create
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see the real dependency on Point of Sale.
It looks like a generic loyalty module, that should be proposed in https://github.com/OCA/sale-promotion instead (without the pos_sale
dependency)
@ivantodorovich Totally agree with your changes! Thanks for your time. |
This module introduces the ability to set a fixed expiration date for gift cards.
Follow these steps to configure and use the module:
cc https://github.com/APSL 165899
@miquelalzanillas @lbarry-apsl @mpascuall @peluko00 @javierobcn @ppyczko please review