-
Notifications
You must be signed in to change notification settings - Fork 5
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
update school validations to return list of errors for frontend use #270
Conversation
9df626c
to
cfe98c8
Compare
ee04a35
to
a172e17
Compare
We require contributors to sign our Contributor License Agreement, and we don't have you on file. In order for us to review and merge your code, please complete this form and we'll get you added and review your contribution as soon as possible. |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
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 great :)
2d8c2d3
to
d61778e
Compare
d61778e
to
bcde08b
Compare
report list of errored fields in lib fix spec fix typo check for type rubocop
bd13bf4
to
9d42d5e
Compare
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.
perfect!
@conorriches have we thought about the i18n story around these error messages? I notice we do try to translate/localise strings that appear in editor-standalone, but I'm not sure how that would work here. I can think of two approaches off the top of my head 1) pass a language parameter in API requests so the API can return the correct version of the strings, or 2) return an identifier for each error instead of a string and use that to look up the localisation on the client side. I'm also slightly nervous that we're relying on the keys (e.g. |
@conorriches - I should say I don't think these queries are caused by things you've introduced in this PR, I think we had the same two issues with the previous approach. I guess I'm mostly interested in having a cross-team approach to them to make sure we keep things consistent. |
Hey @chrislo - yeah indeed a good thought, my running assumption was to keep the i18n on the frontend side of things, as we already have i18n set up there, and I was mostly relying on the backend just providing a list of fields that need flagging. But it's a good point and I think we should look at i18n more broadly, specifically about how we want the api to respond to locales. The error messages appear to be coming out of rails itself, so passing in a locale and having i18n translations would probably be the best approach if we do want the api to be aware of i18n. I've asked in slack about whether we will need locales - but so far in the frontend we only have the I'd like to understand more the concerns about keys staying in sync - the reason I felt safe doing this is that if they keys do go out of sync, then the endpoint won't be able to process the form properly as fields will be missing, meaning the form won't submit even without validation messages. |
Discussed on Slack and we have captured the requirement in #272 |
Issue
Related to https://github.com/RaspberryPiFoundation/editor-standalone/issues/28
What changed
When a school is trying to be created, but validations fail, we now get this response:
This is useful, as the FE application can now iterate on the keys, and selectively update each input field to show the error message.
Before this change we got:
which isn't useful, as we can't associate errors to fields