Fixes for batch invitations #2371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Trello: https://trello.com/c/aO4nVCZT When setting flash messages prior to *rendering* vs redirecting, it's important to use `flash.now` vs `flash` to avoid the error potentially appearing on a later page thus confusing the user. Although I've changed the controller test to assert against `flash.now` instead of `flash` this doesn't seem to force me to make the change in the controller itself. So I've ended up adding an integration test to force me to make the change.
Trello: https://trello.com/c/aO4nVCZT Strip leading & trailing whitespace from BatchInvitationUser#email before validation to avoid unnecessary validation errors confusing users. Note that unlike the strip_whitespace_from_name method, we need to call this from a before_validation callback rather than a before_save callback, because otherwise the email validation can fail due to the presence of the whitespace. As a general principle, I think we should probably implement this whitespace-stripping normalization in a before_validation callback, but I'm going to leave that to whoever picks up this other Trello card [1]. [1]: https://trello.com/c/4QiHXdtE
Trello: https://trello.com/c/aO4nVCZT Strip leading & trailing whitespace from BatchInvitationUser#organisation_slug before save to avoid unnecessary failed organisaion lookups. I decided to add this normalization even though we aren't aware of failed organisation lookups happening, because the call to `Organisation.find_by(slug: organisation_slug)` in `BatchInvitationUser#organisation_from_slug` wouldn't find an organisation if an organisation_slug had e.g. leading whitespace. Since there's no validation on organisation_slug, I've elected to trigger strip_whitespace_from_organisation_slug from a before_save callback c.f. BatchInvitationUser#email. Also the lack of organisation_slug presence validation means that strip_whitespace_from_organisation_slug can't rely on the organisation_slug not being nil, so we have to use the safe navigation operator (&.) in this method. As a general principle, I think we should probably implement this whitespace-stripping normalization in a before_validation callback, but I'm going to leave that to whoever picks up this other Trello card [1]. [1]: https://trello.com/c/4QiHXdtE
Trello: https://trello.com/c/aO4nVCZT Having implemented strip_whitespace_from_organisation_slug I realised I'd probably missed a scenario for strip_whitespace_from_email. And sure enough there was no test coverage for the case when BatchInvitationUser#email is nil. In this commit I've improved the test coverage for email validation which for one thing explicitly forces the presence validation to be enabled for the email attribute, and for another, it forces me to use the safe navigation operator in strip_whitespace_from_email. This is because the latter is called from a before_validation callback and so email could be nil at this point.
mike29736
approved these changes
Sep 22, 2023
floehopper
added a commit
that referenced
this pull request
Sep 25, 2023
Trello: https://trello.com/c/aO4nVCZT I forgot to do this as part of #2371. I've constructed the query in the migration slightly differently to similar previous migrations, because I noticed the whitespace character in the regular expression wasn't working as intended. I've had to use ActiveRecord::Persistence#update_attribute in order to skip model validation, because some of the existing BatchInvitationUser records are invalid for other reasons. I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds!
floehopper
added a commit
that referenced
this pull request
Sep 25, 2023
Trello: https://trello.com/c/aO4nVCZT I forgot to do this as part of #2371. I've constructed the query in the migration slightly differently to similar previous migrations, because I noticed the whitespace character in the regular expression wasn't working as intended. I've had to use ActiveRecord::Persistence#update_attribute in order to skip model validation, because some of the existing BatchInvitationUser records are invalid for other reasons. I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds!
floehopper
added a commit
that referenced
this pull request
Sep 25, 2023
Trello: https://trello.com/c/aO4nVCZT I forgot to do this as part of #2371. I've constructed the query in the migration slightly differently to similar previous migrations, because I noticed the whitespace character in the regular expression wasn't working as intended. I've had to use ActiveRecord::Persistence#update_attribute in order to skip model validation, because some of the existing BatchInvitationUser records are invalid for other reasons. I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds!
floehopper
added a commit
that referenced
this pull request
Sep 25, 2023
Trello: https://trello.com/c/aO4nVCZT I forgot to do this as part of #2371. I've constructed the query in the migration slightly differently to similar previous migrations, because I noticed the whitespace character in the regular expression wasn't working as intended. I've had to use ActiveRecord::Persistence#update_attribute in order to skip model validation, because some of the existing BatchInvitationUser records are invalid for other reasons. I did consider writing the migration as a SQL UPDATE, but generating a TRIM expression that is exactly equivalent to String#strip isn't as trivial as it sounds!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/aO4nVCZT
BatchInvitationUser#emailbefore validation to avoid unnecessarily confusing validation errors.BatchInvitationUser#organisation_slugbefore save to avoid the potential for failing organisation lookups.BatchInvitationUser#emailvalidation.