Update / test remove MP letters task #9089
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
There were a few edge cases which came up when I ran this task last night: 1) MySQL LIKE statements are case insensitive, so the query found things like "Francis Implementation Team" (as it's got a lower case mp in). 2) Even if it was case sensitive, %MP% would match people with MPhil degrees. 3) The `puts` statement being outside of the if statement caused some confusion - it would log things as updated that matched the query, but not the ruby gsub, even though they hadn't been updated. I considered making the MySQL query stricter, but then we might actually miss things (like someone with MP in their letters, but separated with a comma, or lower case). Instead, I've just made it clear which ones are being skipped in the logs. While writing tests for this, I also noticed an edge case where it would replace letters like "CBE MP VC" with "CBEVC" (i.e. it would remove both surrounding spaces from MP). Better to do it with split / join to avoid this I think.
richardTowers
force-pushed
the
update-remove-mp-letters-task
branch
from
0568dce to
ba5c9d2
Compare
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.
There were a few edge cases which came up when I ran this task last night:
%MP%would match people with MPhil degrees.putsstatement being outside of the if statement caused some confusion - it would log things as updated that matched the query, but not the ruby gsub, even though they hadn't been updated.I considered making the MySQL query stricter, but then we might actually miss things (like someone with MP in their letters, but separated with a comma, or lower case). Instead, I've just made it clear which ones are being skipped in the logs.
While writing tests for this, I also noticed an edge case where it would replace letters like "CBE MP VC" with "CBEVC" (i.e. it would remove both surrounding spaces from MP). Better to do it with split / join to avoid this I think.