Skip to content
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

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

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

QuocDuong1306
Copy link

@QuocDuong1306 QuocDuong1306 commented May 22, 2024

The issue:

Context:

  • sale.order:_update_programs_and_rewards() must be called at least once to create reward lines

  • In odoo native, it is done when clicking on button PROMOTIONS and on sale.order:action_confirm()

  • In oca, we have module sale_loyalty_auto_refresh ([16.0][MIG] sale_loyalty_auto_refresh: Migration to 16.0 OCA/sale-promotion#208) to do it whenever the sale order is updated

  • So in shopinvader_api_sale_loyalty, sale.order:_update_programs_and_rewards() was explicitly called on GET /sales to make sure reward lines are created before returning the requested sale orders data

  • But sale.order:_update_programs_and_rewards() implementation calls write() through _reset_loyalty() which is not what we expect from a GET endpoint (should be read-only); one direct consequence is that GET /sales endpoint can fail when called for locked orders

  • This PR fixes it by calling sale.order:_update_programs_and_rewards() only when strictly needed (when no reward line exists), and of course only if not locked.

Improvements:

@QuocDuong1306 QuocDuong1306 changed the title [IMP] shopinvader_api_sale_loyalty: wrong write on GET /sales [16.0][IMP] shopinvader_api_sale_loyalty: wrong write on GET /sales May 22, 2024
@simahawk
Copy link
Contributor

  • Would it be a good addition to add a new POST route in the module to explicitly call sale.order:_update_programs_and_rewards()?

I think so. Yet, if you install sale_loyalty_auto_refresh this won't be needed AFAIU. If yes, we should simply mention this in the readme.

@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-imp-shopinvader_api_sale_loyalty branch from 8836c13 to 58c2b41 Compare May 22, 2024 08:57
@QuocDuong1306
Copy link
Author

I updated the PR

claimable_rewards = odoo_rec._get_claimable_rewards()
if not odoo_rec.state == "done" and not claimable_rewards.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After think twice, I think we can simply remove the call to _update_programs_and_rewards

I just test it with this PR : #1552 and the test are green

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @sebastienbeau for your work, I have included them into this PR (code, description, title). Could you close this PR to make sure all improvements are in one place

@cyrilmanuel
Copy link

@QuocDuong1306 can you make the modification ?

@QuocDuong1306 QuocDuong1306 force-pushed the 16.0-imp-shopinvader_api_sale_loyalty branch from 58c2b41 to 0e4a21c Compare June 27, 2024 04:43
@QuocDuong1306 QuocDuong1306 changed the title [16.0][IMP] shopinvader_api_sale_loyalty: wrong write on GET /sales [16.0][IMP] shopinvader_api_sale_loyalty: do not update rewards when reading a sale Jun 27, 2024
@QuocDuong1306
Copy link
Author

This PR has been updated including changes in this PR @simahawk @cyrilmanuel

@shopinvader-git-bot
Copy link

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). 🤖

1 similar comment
@shopinvader-git-bot
Copy link

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). 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants