-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: dash slug names #5709
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: dash slug names #5709
Conversation
michael-genson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already do basically this on the backend, I think it would be better to keep the logic there and throw an error that we catch on the frontend. That way we don't duplicate the slug logic in both the frontend and backend.
We could even do a custom error message like "unable to generate slug" which the frontend could read. We already have some error messages that we directly show the user using i18n on the backend (though not many).
e.g. from LDAP:
if self.user.password == "LDAP" or self.user.auth_method == AuthMethod.LDAP:
raise HTTPException(
status.HTTP_400_BAD_REQUEST, ErrorResponse.respond(self.t("user.ldap-update-password-unavailable"))
)This will automatically flash an error to the user on the frontend (we have a custom plugin for this).
c0f7438 to
abc7237
Compare
…ix/5532-fix-dash-slug-names
Now when slug is invalid raise a SlugError, that I create in core.exceptions and raise a HTTPException in RecipeController class in recipe_crud_routes.py file |
michael-genson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for the language files; translations are handled through Crowdin. You should only add to/update the en-US.json file. Everything else shouldn't be modified here
michael-genson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and finished this one up. I ended out removing the custom message and just stuck with the existing flow. The user can always check the logs if they're confused, but at least now it won't create an invalid recipe.
Thanks for your work on this!
|
I'm sorry the past few months have been really crazy. Please forgive my absence. I think your solution is much cleaner and better suited for such a rare case. Thank you so much for the opportunity to collaborate on this wonderful project. |
|
No worries, appreciate your work on this! |
What this PR does / why we need it:
Before
After
Which issue(s) this PR fixes:
Fixes #5532
Special notes for your reviewer:
I have created a function to generate the slug in the frontend to be able to check it before sending it to the backend, I don't know if I should use some library like in the backend
Testing
Tested locally, w/Chrome dev tools for mobile UX.