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 enterprise fees #9791

Conversation

vsmay98
Copy link

@vsmay98 vsmay98 commented Oct 16, 2022

What? Why?

Closes #8250

What should we test?

This is the current status of the calculators:

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

Steps to Reproduce
As an Admin:

  • edit an enterprise fee
  • 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 16, 2022

Hello @drummer83
This PR is ready for review.

@drummer83
Copy link
Contributor

Thanks @vsmay98!
Your pull request is now in Code Review and we will let you know how it goes.
Thanks for your contribution!

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.

That looks good, thank you.

@mkllnk
Copy link
Member

mkllnk commented Oct 18, 2022

A spec to reproduce the error first would be ideal.

@mkllnk mkllnk requested a review from jibees October 18, 2022 04:31
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.

I think having a system spec that reproduce this specific case and ensure that the case is solved would be a good idea!

@mkllnk
Copy link
Member

mkllnk commented Oct 19, 2022

@vsmay98 Would you be happy to add a spec?

@vsmay98
Copy link
Author

vsmay98 commented Oct 19, 2022

Sure @mkllnk
I'm not well aware of system spec but will give it a try.

@vsmay98 vsmay98 force-pushed the 8250-fix-snail-for-all-enterprise-fee branch from 6c3517f to de28027 Compare October 20, 2022 06:45
@vsmay98
Copy link
Author

vsmay98 commented Oct 20, 2022

Hello @mkllnk @jibees
I have added the specs, but now some other specs are failing due to this. Could you please take a look?

@sigmundpetersen
Copy link
Contributor

@vsmay98 I think this is a "flaky" spec. I'm re-running the build, let's see if it passes.

@vsmay98
Copy link
Author

vsmay98 commented Oct 20, 2022

Thanks @sigmundpetersen 🤝 The build is green now.

@vsmay98 vsmay98 requested a review from jibees October 20, 2022 09:47
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.

Nice, thanks @vsmay98 !

@jibees jibees requested a review from mkllnk October 20, 2022 13:43
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.

Thank you!

@drummer83 drummer83 self-assigned this Oct 24, 2022
@drummer83 drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Oct 24, 2022
@drummer83
Copy link
Contributor

drummer83 commented Oct 24, 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:

Peek 2022-10-24 21-16

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

After your PR I tested again:

Peek 2022-10-24 21-30

In the screen capture I mostly 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 'max items' field 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 and 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 on the page should be kept (including changes on other existing enterprise fees)
  • Flat rate (per order), Flexible rate, Flat rate (per item), Price sack: remove currency setting (the amount is considered the instance's currency anyway) ➡️ wishlist openfoodfoundation/openfoodnetwork#10272

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). ➡️ #9860

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 enterprise fees
6 participants