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

Fix snail issue for all shipping method fees #9756

Conversation

vsmay98
Copy link

@vsmay98 vsmay98 commented Oct 9, 2022

What? Why?

Closes #8249

What should we test?

flat percent --> snail
flat rate (per order) --> OK
flexible rate --> snail
flat rate (per item) --> OK
price sack --> snail
weight (per kg or lb) --> probably snail

As an Admin:

  • edit a shipping method
  • on the calculator section choose one of the calculators marked with "snail" above
  • click update
  • type in an invalid value (e.g. some text or numbers with comma as separator)
  • click update --> snail

Release notes

Changelog Category: User facing changes | Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@vsmay98
Copy link
Author

vsmay98 commented Oct 9, 2022

This PR is ready for review.

@vsmay98
Copy link
Author

vsmay98 commented Oct 10, 2022

Hello @drummer83
Could you please get this PR review?

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

This looks like a good solution. Can you fix the remaining Rubocop warning?

@vsmay98 vsmay98 requested a review from mkllnk October 10, 2022 14:59
@vsmay98
Copy link
Author

vsmay98 commented Oct 10, 2022

Done @mkllnk Please re-review

@mkllnk mkllnk requested a review from jibees October 11, 2022 00:46
Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

Thanks!

I'd squashed all of those 3 commits to avoid fix rubocop warning commit.

@vsmay98 vsmay98 force-pushed the 8249-fix-snail-for-all-shipping-method-fee branch from 97b9116 to e02de2b Compare October 11, 2022 10:00
@drummer83 drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Oct 20, 2022
@drummer83 drummer83 self-assigned this Oct 20, 2022
@drummer83
Copy link
Contributor

drummer83 commented Oct 20, 2022

Hi @vsmay98,
Thanks for this pull request!

I have first made sure that I can still reproduce the issue on our staging server.
Before your PR the situation was like this:

  • flat percent --> snail
  • flat rate (per order) --> OK
  • flexible rate --> snail
  • flat rate (per item) --> OK
  • price sack --> snail
  • weight (per kg or lb) --> snail

After your PR I tested again:
Peek 2022-10-20 12-10

In the screen capture I always used the string 'invalid' as input but I have also tested:

  • numbers with comma separator ✔️
  • numbers with two '-' signs ✔️
  • numbers with leading spaces, trailing spaces or spaces in between ✔️
  • quotes ' and " ✔️

For the different calculators we now have:

  • flat percent --> OK
  • flat rate (per order) --> OK
  • flexible rate --> OK
  • flat rate (per item) --> OK
  • price sack --> OK
  • weight (per kg or lb) --> OK

Worth mentioning:

  • For 'Flexible rate' the amount value is not validated as other fields, but it is set to '0' for invalid input while for 'Price sack' the amount values are validated as other fields - the error message is displayed for invalid input. Flexible rate should do the same.
  • Invalid input field(s) should be highlighted (red border of input field) and all other updated information should be kept (including description, tags, display, ...)
  • Flat rate (per order), Flexible rate, Flat rate (per item), Price sack: remove currency setting (the amount is considered the instance's currency anyway)

This is looking really great and it's removing quite a few snails at one shot. 💥

We are ready to go here and I will create a follow-up issue for the topics of 'worth mentioning' (probably later today). Maybe you would like to continue your great work on this! ➡️ #9836

Thanks again! 🥳

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.

Snail when editing non-numeric characters on shipping method fees
5 participants