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

Add validation when paying excess amount #230

Merged
merged 8 commits into from
Nov 2, 2021

Conversation

ankitsinghaniyaz
Copy link
Contributor

No description provided.

piyushsinghania and others added 3 commits October 29, 2021 18:26
…played-while-paying-excess-amount

Added validation to give error message when trying to pay excess amount for an invoice
Comment on lines 112 to 114
throw new frappe.errors.ValidationError(
`Payment amount ${value} cannot be accepted. Please enter a value between 0 and ${amount}`
);
Copy link
Member

Choose a reason for hiding this comment

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

@piyushsinghania on checking the change, it doesn't allow me to set a value greater than what is expected:

Screen.Recording.2021-11-01.at.10.55.45.mov

So keeping that in mind I suggest a few changes:

  1. Reword the message to something like "Payment amount cannot exceed ₹ ${amount}." , or you can reword it to also account for the fact that the value has been reset to the max viable amount (in haste they may not notice this, and later on it can be an issue).
  2. And "Payment amount cannot be ₹ 0.00" in case the amount is 0.
  3. Don't forget the currency sigil. For this, we could use a helper function to format currencies to the correct precision with the correct sigil.
  4. Don't forget to wrap the message in frappe._ to support translations later when we add them.

Other than that it works perfectly, I think capping it to the max viable amount is a nice touch!

(cc: @ankitsinghaniyaz)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1 we'll verify and 100% on the other points.

@ankitsinghaniyaz
Copy link
Contributor Author

@18alantom addressed all the feedback

@18alantom 18alantom merged commit 99b24d4 into frappe:master Nov 2, 2021
@18alantom
Copy link
Member

Didn't realize the capping was auto 😅
Either way, better than spamming with errors until change.

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

Successfully merging this pull request may close these issues.

3 participants