Skip to content

Typeahead remove diacritics #5292

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 16, 2022
Merged

Typeahead remove diacritics #5292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba68387
typeahead: Remove filtering by user initials.
Fingel Mar 11, 2022
8dfcde1
shared: Upgrade from 0.0.8 to 0.0.9.
Fingel Mar 14, 2022
d73e764
typeahead: Ignore diacritics when filtering users.
Fingel Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"@react-navigation/native": "^5.7.6",
"@react-navigation/stack": "^5.9.3",
"@sentry/react-native": "^3.1.1",
"@zulip/shared": "^0.0.8",
"@zulip/shared": "^0.0.9",
"base-64": "^1.0.0",
"blueimp-md5": "^2.10.0",
"color": "^4.0.1",
Expand Down
73 changes: 53 additions & 20 deletions src/users/__tests__/userHelpers-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
getAutocompleteUserGroupSuggestions,
sortAlphabetically,
filterUserStartWith,
filterUserByInitials,
filterUserThatContains,
filterUserMatchesEmail,
getUniqueUsers,
Expand Down Expand Up @@ -115,8 +114,8 @@ describe('getAutocompleteSuggestion', () => {
expect(filteredUsers).toEqual(shouldMatch);
});

test('result should be in priority of startsWith, initials, contains in name, matches in email', () => {
const user1 = eg.makeUser({ name: 'M Apple', email: '[email protected]' }); // satisfy initials condition
test('result should be in priority of startsWith, contains in name, matches in email', () => {
const user1 = eg.makeUser({ name: 'M Apple', email: '[email protected]' }); // does not match
const user2 = eg.makeUser({ name: 'Normal boy', email: '[email protected]' }); // satisfy full_name contains condition
const user3 = eg.makeUser({ name: 'example', email: '[email protected]' }); // random entry
const user4 = eg.makeUser({ name: 'Example', email: '[email protected]' }); // satisfy email match condition
Expand All @@ -125,7 +124,7 @@ describe('getAutocompleteSuggestion', () => {
const user7 = eg.makeUser({ name: 'Match App Normal', email: '[email protected]' }); // satisfy all conditions
const user8 = eg.makeUser({ name: 'match', email: '[email protected]' }); // duplicate
const user9 = eg.makeUser({ name: 'Laptop', email: '[email protected]' }); // random entry
const user10 = eg.makeUser({ name: 'Mobile App', email: '[email protected]' }); // satisfy initials and email condition
const user10 = eg.makeUser({ name: 'Mobile App', email: '[email protected]' }); // satisfy email condition
Comment on lines -128 to +127
Copy link
Member

Choose a reason for hiding this comment

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

nit: this and the remaining changes in this test should go in the first commit, together with the change in the code they describe

The commands I'd use for making this sort of revision -- pulling some changes forward from a later commit in a branch to an earlier one -- would be:

  • git rebase -i; say to edit the first commit
  • when the rebase reaches that commit, use git checkout -p to select the later changes to add.
    • For example, git checkout -p main if your local branch is main; or git checkout -p typeahead-remove-diacritics if you're using the same name locally as you have here.
    • In general you could also add the filename, or a directory containing it: git checkout -p main -- src/users/__tests__/. In this case there aren't a lot of other files to skip past, so you might not bother.
    • To get your bearings before doing the git checkout -p, you can use git diff --stat -p -R main. That will show you all the changes you'll get to choose from if you do git checkout -p main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just 2 or 3 changes in the comments, so I just edited the first commit manually and then the rest of the rebase was clean, but thanks for the info. I could definitely see it getting more complicated with larger changesets.

Copy link
Member

Choose a reason for hiding this comment

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

Cool.

I just edited the first commit manually and then the rest of the rebase was clean

Yeah, I forgot to mention: that's also how the git checkout -p approach works, except for the word "manually".

That is, it's just a way of editing what you have in the tree. The rebase doesn't know anything special about whether you edited with git checkout -p vs. with your editor or anything else.

But then when you continue the rebase and it goes to apply the later commit, when some of the hunks in that diff are already present, it doesn't complain, and instead just makes a commit using the remaining hunks.

const user11 = eg.makeUser({ name: 'Normal', email: '[email protected]' }); // satisfy contains in name and matches in email condition
const allUsers = deepFreeze([
user1,
Expand All @@ -145,11 +144,10 @@ describe('getAutocompleteSuggestion', () => {
user5, // name starts with 'ma'
user6, // have priority as starts with 'ma'
user7, // have priority as starts with 'ma'
user1, // initials 'MA'
user10, // have priority because of initials condition
user2, // name contains in 'ma'
user11, // have priority because of 'ma' contains in name
user4, // email contains 'ma'
user10, // email contains 'ma'
];
const filteredUsers = getAutocompleteSuggestion(
allUsers,
Expand Down Expand Up @@ -259,22 +257,31 @@ describe('filterUserStartWith', () => {
const expectedUsers = [user1, user3];
expect(filterUserStartWith(users, 'app', selfUser.user_id)).toEqual(expectedUsers);
});
});

describe('filterUserByInitials', () => {
test('returns users whose full_name initials matches filter excluding self', () => {
const user1 = eg.makeUser({ name: 'Apple', email: '[email protected]' });
const user2 = eg.makeUser({ name: 'mam', email: '[email protected]' });
const user3 = eg.makeUser({ name: 'app', email: '[email protected]' });
const user4 = eg.makeUser({ name: 'Mobile Application', email: '[email protected]' });
const user5 = eg.makeUser({ name: 'Mac App', email: '[email protected]' });
const user6 = eg.makeUser({ name: 'app', email: '[email protected]' });
const selfUser = eg.makeUser({ name: 'app', email: '[email protected]' });

const users = deepFreeze([user1, user2, user3, user4, user5, user6, selfUser]);
test('returns users whose name contains diacritics but otherwise starts with filter', () => {
const withDiacritics = eg.makeUser({ name: 'Frödö', email: '[email protected]' });
const withoutDiacritics = eg.makeUser({ name: 'Frodo', email: '[email protected]' });
const nonMatchingUser = eg.makeUser({ name: 'Zalix', email: '[email protected]' });
const users = deepFreeze([withDiacritics, withoutDiacritics, nonMatchingUser]);
const expectedUsers = [withDiacritics, withoutDiacritics];
expect(filterUserStartWith(users, 'Fro', eg.makeUser().user_id)).toEqual(expectedUsers);
});

const expectedUsers = [user4, user5];
expect(filterUserByInitials(users, 'ma', selfUser.user_id)).toEqual(expectedUsers);
test('returns users whose name contains diacritics and filter uses diacritics', () => {
const withDiacritics = eg.makeUser({ name: 'Frödö', email: '[email protected]' });
const withoutDiacritics = eg.makeUser({ name: 'Frodo', email: '[email protected]' });
const wrongDiacritics = eg.makeUser({ name: 'Frōdō', email: '[email protected]' });
const notIncludedDiactritic = eg.makeUser({ name: 'Fřödo', email: '[email protected]' });
const nonMatchingUser = eg.makeUser({ name: 'Zalix', email: '[email protected]' });
const users = deepFreeze([
withDiacritics,
withoutDiacritics,
wrongDiacritics,
notIncludedDiactritic,
nonMatchingUser,
]);
const expectedUsers = [withDiacritics];
expect(filterUserStartWith(users, 'Frö', eg.makeUser().user_id)).toEqual(expectedUsers);
});
});

Expand Down Expand Up @@ -331,6 +338,32 @@ describe('filterUserThatContains', () => {
const expectedUsers = [user2, user5];
expect(filterUserThatContains(users, 'ma', selfUser.user_id)).toEqual(expectedUsers);
});

test('returns users whose full_name has diacritics but otherwise contains filter', () => {
const withDiacritics = eg.makeUser({ name: 'Aärdvärk', email: '[email protected]' });
const withoutDiacritics = eg.makeUser({ name: 'Aardvark', email: '[email protected]' });
const nonMatchingUser = eg.makeUser({ name: 'Turtle', email: '[email protected]' });
const users = deepFreeze([withDiacritics, withoutDiacritics, nonMatchingUser]);
const expectedUsers = [withDiacritics, withoutDiacritics];
expect(filterUserThatContains(users, 'vark', eg.makeUser().user_id)).toEqual(expectedUsers);
});

test('returns users whose full_name has diacritics and filter uses diacritics', () => {
const withDiacritics = eg.makeUser({ name: 'Aärdvärk', email: '[email protected]' });
const withoutDiacritics = eg.makeUser({ name: 'Aardvark', email: '[email protected]' });
const wrongDiacritics = eg.makeUser({ name: 'Aärdvãrk', email: '[email protected]' });
const notIncludedDiactritic = eg.makeUser({ name: 'Aärdväŕk', email: '[email protected]' });
const nonMatchingUser = eg.makeUser({ name: 'Turtle', email: '[email protected]' });
const users = deepFreeze([
withDiacritics,
withoutDiacritics,
wrongDiacritics,
notIncludedDiactritic,
nonMatchingUser,
]);
const expectedUsers = [withDiacritics];
expect(filterUserThatContains(users, 'värk', eg.makeUser().user_id)).toEqual(expectedUsers);
});
});

describe('filterUserMatchesEmail', () => {
Expand Down
44 changes: 18 additions & 26 deletions src/users/userHelpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */
// $FlowFixMe[untyped-import]
import uniqby from 'lodash.uniqby';
import * as typeahead from '@zulip/shared/js/typeahead';
Copy link
Member

Choose a reason for hiding this comment

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

Build fails because this doesn't have types.

Have you sent a zulip/zulip PR with the .js.flow file? Then once merged I can make a shared release, and this PR can use that.

Copy link
Member

Choose a reason for hiding this comment

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

(Probably makes sense to add types for all the exports in that file while you're at it, though on the call yesterday we only did the one.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR here: https://github.com/zulip/zulip/pull/21397/files

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/zulip/zulip/pull/21397/files

Hmm, that didn't cause GitHub to show a link from that PR back to this one!

That is… a trick I may use in the future 😉 . Sometimes those back-links are unwanted.

Here, though, I think it's helpful when looking at that PR to see its context from this one. So here's a link that will cause a back-link: zulip/zulip#21397


import type {
MutedUsersState,
Expand Down Expand Up @@ -86,35 +87,27 @@ export const filterUserStartWith = (
users: $ReadOnlyArray<AutocompleteOption>,
filter: string = '',
ownUserId: UserId,
): $ReadOnlyArray<AutocompleteOption> =>
users.filter(
user =>
user.user_id !== ownUserId && user.full_name.toLowerCase().startsWith(filter.toLowerCase()),
);

export const filterUserByInitials = (
users: $ReadOnlyArray<AutocompleteOption>,
filter: string = '',
ownUserId: UserId,
): $ReadOnlyArray<AutocompleteOption> =>
users.filter(
user =>
user.user_id !== ownUserId
&& user.full_name
.replace(/(\s|[a-z])/g, '')
.toLowerCase()
.startsWith(filter.toLowerCase()),
);
): $ReadOnlyArray<AutocompleteOption> => {
const loweredFilter = filter.toLowerCase();
const isAscii = /^[a-z]+$/.test(loweredFilter);
return users.filter(user => {
const full_name = isAscii ? typeahead.remove_diacritics(user.full_name) : user.full_name;
Copy link
Member

Choose a reason for hiding this comment

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

Let's have a test to cover the "else" case of this too -- the fact that if you do type a diacritic, it won't match a name that has that letter without the diacritic; or has a different diacritic; or on some other letter where the query is just plain ASCII, does have a diacritic.

return user.user_id !== ownUserId && full_name.toLowerCase().startsWith(loweredFilter);
});
};

export const filterUserThatContains = (
users: $ReadOnlyArray<AutocompleteOption>,
filter: string = '',
ownUserId: UserId,
): $ReadOnlyArray<AutocompleteOption> =>
users.filter(
user =>
user.user_id !== ownUserId && user.full_name.toLowerCase().includes(filter.toLowerCase()),
);
): $ReadOnlyArray<AutocompleteOption> => {
const loweredFilter = filter.toLowerCase();
const isAscii = /^[a-z]+$/.test(loweredFilter);
return users.filter(user => {
const full_name = isAscii ? typeahead.remove_diacritics(user.full_name) : user.full_name;
return user.user_id !== ownUserId && full_name.toLowerCase().includes(filter.toLowerCase());
});
};

export const filterUserMatchesEmail = (
users: $ReadOnlyArray<AutocompleteOption>,
Expand Down Expand Up @@ -150,10 +143,9 @@ export const getAutocompleteSuggestion = (
}
const allAutocompleteOptions = getUsersAndWildcards(users);
const startWith = filterUserStartWith(allAutocompleteOptions, filter, ownUserId);
const initials = filterUserByInitials(allAutocompleteOptions, filter, ownUserId);
const contains = filterUserThatContains(allAutocompleteOptions, filter, ownUserId);
const matchesEmail = filterUserMatchesEmail(users, filter, ownUserId);
const candidates = getUniqueUsers([...startWith, ...initials, ...contains, ...matchesEmail]);
const candidates = getUniqueUsers([...startWith, ...contains, ...matchesEmail]);
return candidates.filter(user => !mutedUsers.has(user.user_id));
};

Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2586,10 +2586,10 @@
resolved "https://registry.yarnpkg.com/@yarnpkg/lockfile/-/lockfile-1.1.0.tgz#e77a97fbd345b76d83245edcd17d393b1b41fb31"
integrity sha512-GpSwvyXOcOOlV70vbnzjj4fW5xW/FdUF6nQEt1ENy7m4ZCczi1+/buVUPAqmGfqznsORNFzUMjctTIp8a9tuCQ==

"@zulip/shared@^0.0.8":
version "0.0.8"
resolved "https://registry.yarnpkg.com/@zulip/shared/-/shared-0.0.8.tgz#c0a786eb09c30cae3da364dee4bd49a09529129e"
integrity sha512-SmFzU2a2gqt3+XDdu7UF1/5iS/1t6Gvh/Qc9gWdN/pcVzX4qr4l0di6toORSeq/rX2FiHBANrOdozmRXLgRpHw==
"@zulip/shared@^0.0.9":
version "0.0.9"
resolved "https://registry.yarnpkg.com/@zulip/shared/-/shared-0.0.9.tgz#a7bb13eb22097e0c0f9e1c96008a007860b495e3"
integrity sha512-DYQ2pEUcDgLkMvoltnO4EHI1G/tKuuHpKk11gual6Nwih5ZRfcHLOzb40wBfkwwrj3tgEDdniWcggmQnyEseNw==
dependencies:
katex "^0.12.0"
lodash "^4.17.19"
Expand Down