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

SafeNames does not abbreviate first names #35216

Merged
merged 1 commit into from Jun 9, 2020
Merged

Conversation

islemaster
Copy link
Contributor

FND-1146: Fix the name abbreviation logic on section login page.

There were some documented problems with name abbreviations when one first-name was a strict subset of another. For instance, for the set of names Anna, Annabel, Annabelle, Brad, Bradford, Bradlee, Bradley, Branford, Maria, Mariamu the login page would show the following:

Screenshot from 2020-06-08 13-07-00

Notice that Anna and Maria have been reduced to one character, and Annabel has oddly been truncated to five characters.

The logic abbreviating names here is designed to find the smallest unique prefix that can be used to tell two students with the same name apart; for example, reducing Tom Sawyer, Tom Smith to Tom Sa, Tom Sm. However, we never want to abbreviate a first name, and if the first name is already unique, we should be able to use it without trying to compute a minimum prefix.

We already had logic to that effect here:

    # If the first name is unique, simply use that
    return first_name if trie.words(first_name).count == 1

But trie.words gets all the words that start with a particular string. If you put in John Smith and Johnny Smith then trie.words("John").count is 2 even though the first name is unique.

I've added a different check for first-name-uniqueness to fix this behavior. Now the login page looks like this:

Screenshot from 2020-06-08 14-08-09

Testing story

I've extended the existing test with new assertions capturing known problem cases.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

# Preserves duplicate names
verify(['John Smith', 'John Smith'], ['John', 'John'])
verify(['John Smith', 'John Smith', 'John Smythe'], ['John Smi', 'John Smi', 'John Smy'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Captured this because I found it to be an interesting and useful behavior - I think this is a better outcome than John, John, John Smy.

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -25,13 +25,17 @@ def self.get_safe_names(collection, first_last_name_proc)
item_descriptions << {full_name: full_name, item: item, first_name: first_name}
end

# Count cases of each first name to check first-name uniqueness
# {"Aaron" => 2, "Bey" => 1, "Chloe" => 1, ...}
first_name_counts = item_descriptions.group_by {|item| item[:first_name]}.transform_values(&:count)
Copy link
Contributor

Choose a reason for hiding this comment

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

concise!

@islemaster islemaster merged commit 8b461e8 into staging Jun 9, 2020
@islemaster islemaster deleted the safe-names-update branch June 9, 2020 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants