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.

Sign up for GitHub

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

Jump to bottom

fix: Shopping cart Exchange rate validation #27040

Merged
merged 2 commits into from
Aug 20, 2021
Merged

fix: Shopping cart Exchange rate validation #27040

merged 2 commits into from
Aug 20, 2021

Conversation

marination
Copy link
Collaborator

@marination marination commented Aug 19, 2021 •
edited

Issue:

  • Create a Company ABC with currency INR and have a Selling Price List with currency INR
  • In Shopping Cart Settings, enable it, set company as ABC and Price list as INR selling price list. Save.
  • Go to this Selling Price List, change it's currency to USD/any other currency
  • This appears:
    Screenshot 2021-08-19 at 5 52 44 PM
  • This same issue occurs on changing company currency too. If it is changed to AUD (with country Australia) from INR, while creating sales taxes and charges template for Australia, the same error occurs

Actual Issue:

  • This validation is called from hooks.py and on saving Shopping Cart Settings
  • The validation is in place to make sure that "missing currency" error does not appear while someone is using the shopping cart.
  • How it validates this? By checking if a currency exchange record exists. In almost all cases people rely on the currency exchange API to fetch exchange rate
  • Currency Exchange record is made for manual cases.
  • In this case it almost always throws this error because Currency Exchange record does not exist (and is not mandatory)

Fix:

  • Use get_exchange_rate to check for price list exchange rate. It returns value from Currency Exchange` record if a fresh on exists else it hits the exchange rate API
  • Move cart exchange rate validation for Price List from hooks to doc event.
  • Call cart exchange rate validation on PL update only if PL is in cart and currency is changed

@marination
fix: Shopping cart Exchange rate validation
45617ae 
- Use `get_exchange_rate` to check for price list exchange rate in cart settings
- Move cart exchange rate validation for Price List from hooks to doc event
- Call cart exchange rate validation on PL update only if PL is in cart and currency is changed
@marination
chore: Comment out obsolete test
8046d15 
- Modifying this test means considering extreme edge cases, which seems pointless now
@nabinhait nabinhait merged commit b8c9981 into frappe:develop Aug 20, 2021
@marination marination deleted the cart-pl-exchange-rate branch August 20, 2021 05:30
@frappe-pr-bot
Copy link
Collaborator

The backport to version-13-hotfix failed.
Please backport the PR manually. 🤖

The process '/usr/bin/git' failed with exit code 1

@marination marination mentioned this pull request Aug 20, 2021
Merged
@marination
Copy link
Collaborator Author

tests that are failing are in POS (due to framework) and in SLA

asoral pushed a commit to asoral/erpnext that referenced this pull request Nov 12, 2021
@nabinhait
Merge pull request frappe#27040 from marination/cart-pl-exchange-rate
a25c513 
fix: Shopping cart Exchange rate validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabinhait nabinhait nabinhait approved these changes

@ankush ankush Awaiting requested review from ankush

@rohitwaghchaure rohitwaghchaure Awaiting requested review from rohitwaghchaure rohitwaghchaure is a code owner

Assignees
No one assigned
Labels
backport version-13-hotfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marination @frappe-pr-bot @nabinhait