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: reset password checks with languages #787

Merged

Conversation

monsieurtanuki
Copy link
Contributor

What

  • Not that long ago there was no country or language parameters on reset password. We lousily tried to detect sentences in the resulting html.
  • Now that we have country or language parameters, we get a localized html. The sentences we expected are localized. That's why in this PR we use html tags instead of localized terms.
  • That's still very lousy: we would be better off calling a more appropriate server URL that would return a json. But that does not exist AFAIK.

Part of

Impacted files

  • open_food_api_client.dart: relying on html tags for error detection instead of English localizations
  • user_management_test_prod.dart: additional tests with / without language/country and on existing / not existing users

Impacted files:
* `open_food_api_client.dart`: relying on html tags for error detection instead of English localizations
* `user_management_test_prod.dart`: additional tests with / without language/country and on existing / not existing users
Copy link
Contributor

@g123k g123k left a comment

Choose a reason for hiding this comment

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

A better implementation indeed.
FYI, I know there is an opened issue to have error codes instead, but during the meantime, it's still better

@monsieurtanuki
Copy link
Contributor Author

Thank you @g123k for your review!

@monsieurtanuki monsieurtanuki merged commit 0fee449 into openfoodfacts:master Aug 17, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants