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

Translate and fix fallback errors when creating or updating OC #2660

Merged

Conversation

kristinalim
Copy link
Member

What? Why?

Closes #2641

This translates the fallback errors when creating or updating an OC fails. Fallback, because some errors that we explicitly support in the code (e.g. when close date is set before open date) give a more specific error.

Then, for the error when updating, this was actually saying "Failure to create order cycle" (should be "update"). This has also been addressed.

What should we test?

One way I let these errors appear is by removing authorization of a user for an enterprise while the user is filling in the OC form.

@RachL if you know a simpler way, kindly share. It's likely that when you encountered these errors after leaving some fields blank was only because staging had inconsistent data as @luisramos0 found. I couldn't get these errors to appear that way.

  1. Browser Session 1: Add test user as manager to an enterprise.
  2. Browser Session 2: As test user, open the new OC page and fill in the form, but do not submit.
  3. Browser Session 1: Remove test user as manager for the enterprise.
  4. Browser Session 2: Being unauthorized for the enterprise now, test user should get the translated error when submitting the form.

Repeat the same for editing an OC.

Release notes

  • Fix and translate fallback errors when creating and updating an order cycle

Changelog Category: Fixed

@kristinalim kristinalim self-assigned this Sep 7, 2018
@kristinalim kristinalim force-pushed the translations-failed_oc_creation branch from 2f06c14 to 003e65f Compare September 7, 2018 18:16
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

Approving this PR as we have agreed the convention is to keep js translations under js to reduce assets size.
We need to:

  • create issue to represent the clean up - moving js existing translations under js
  • create issue to change the assets pipeline to incude only translations under js
  • write this convention on wiki

@kristinalim
Copy link
Member Author

I created issues openfoodfoundation/wishlist#342 and #2722, and also added the section "Javascript translations: Put translation key under the "js." namespace" to the i18n wiki page.

There are already 2 code review approvals here, so moving to "Test Ready".

@sstead
Copy link

sstead commented Sep 14, 2018

Testing notes

The error messages appeared correctly in English. One note, the message read 'failed to create OC' regardless of whether the use is trying to create or edit an order. I don't think this really matters, but I suppose one could say 'failed to edit OC'

https://docs.google.com/document/d/1Ikt8tJ1gYX-a1Mmtpgp6B99XbapRz5y92wicmrQAnTA/edit#heading=h.xw2hpxaxawc9

@mkllnk
Copy link
Member

mkllnk commented Sep 14, 2018

There should be two error messages:

  • "Failed to create order cycle"
  • "Failed to update order cycle"

Actually, I just checked the code and if see only the create error, then this pull request doesn't work properly. Let me check.

@kristinalim
Copy link
Member Author

@mkllnk I suspect this hasn't been staged yet.

@mkllnk
Copy link
Member

mkllnk commented Sep 14, 2018

I deployed it again and re-tested: it works.
screenshot from 2018-09-14 16-51-59

@sstead
Copy link

sstead commented Sep 14, 2018

Testing Notes

There are unique error messages now- "failed to update" and "failed to create"

https://docs.google.com/document/d/1Ikt8tJ1gYX-a1Mmtpgp6B99XbapRz5y92wicmrQAnTA/edit#

@mkllnk mkllnk merged commit ce7be68 into openfoodfoundation:master Sep 14, 2018
@luisramos0
Copy link
Contributor

luisramos0 commented Sep 30, 2018

hey @kristinalim I just now saw the issues to move translation to js namespace and also the wiki update. great stuff, thanks!

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.

Translation missing in error message / OC creation
5 participants