Typeahead remove diacritics #5292

Fingel · 2022-03-11T19:13:53Z

When invoking the typeahead filter for users it is desirable to be able
to see results for users with diacritics in their names even if the
filter does not contain the matching diacritics. This PR leverages
@zulip/shared typeahead module in order to strip diacritics out of the
filtering process, unless the user explicitly uses them in the filter.

Fixes: #3710

chrisbobbe · 2022-03-11T20:43:19Z

Hmm, looks like the CI failure is a Flow error:

Error ------------------------------------------------------------------------------------- src/users/userHelpers.js:4:1

Importing from an untyped module makes it `any` and is not safe! Did you mean to add `// @flow` to the top of
`@zulip/shared/js/typeahead`? [untyped-import]

   4| import * as typeahead from '@zulip/shared/js/typeahead';
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^



Found 1 error

If it's reasonably easy, I think we'd want to add a js/typeahead.js.flow to @zulip/shared, like we have an internal_url.js.flow and typing_status.js.flow.

Otherwise, I think we'd add a // $FlowFixMe[untyped-import] line above the import.

Greg, is that right?

Fingel · 2022-03-11T20:45:09Z

Hmm, looks like the CI failure is a Flow error:
Error ------------------------------------------------------------------------------------- src/users/userHelpers.js:4:1

Importing from an untyped module makes it `any` and is not safe! Did you mean to add `// @flow` to the top of
`@zulip/shared/js/typeahead`? [untyped-import]

   4| import * as typeahead from '@zulip/shared/js/typeahead';
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^



Found 1 error
If it's reasonably easy, I think we'd want to add a js/typeahead.js.flow to @zulip/shared, like we have an internal_url.js.flow and typing_status.js.flow.

Otherwise, I think we'd add a // $FlowFixMe[untyped-import] line above the import.

Greg, is that right?

This is correct, there will be a PR for zulip/zulip with a .flow file for typeahead.js shortly, I'll make this a draft PR for now.

gnprice

Thanks @Fingel! This will be a good change. Comments below.

gnprice · 2022-03-11T20:37:26Z

src/users/__tests__/userHelpers-test.js

      user2, // name contains in 'ma'
      user11, // have priority because of 'ma' contains in name
      user4, // email contains 'ma'
+      user10, // have priority because of initials condition


nit: this comment needs updating

gnprice · 2022-03-11T20:39:23Z

src/users/userHelpers.js

@@ -1,6 +1,7 @@
 /* @flow strict-local */
 // $FlowFixMe[untyped-import]
 import uniqby from 'lodash.uniqby';
+import * as typeahead from '@zulip/shared/js/typeahead';


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.

(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.)

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

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

gnprice · 2022-03-11T20:40:58Z

src/users/__tests__/userHelpers-test.js

-    const expectedUsers = [user4, user5];
-    expect(filterUserByInitials(users, 'ma', selfUser.user_id)).toEqual(expectedUsers);
+  test('returns users whose name contains diacritics but otherwise starts with filter', () => {
+    const with_diacritis = eg.makeUser({ name: 'Frödö', email: '[email protected]' });


nit: spelling, "diacritics" (here and another test)

gnprice · 2022-03-11T20:41:12Z

src/users/__tests__/userHelpers-test.js

  });
+  test('returns users whose full_name has diacritics but otherwise contains filter', () => {


nit: blank line between these

I don't have a great bright-line rule I can articulate for when a blank line is called for in general. But when there's two multi-line blocks next to each other, like these two test cases, those almost always call for a blank line to separate them.

The Google style guide describes rules I agree with:
https://google.github.io/styleguide/jsguide.html#formatting-vertical-whitespace
Applying those literally to this case, it's a question of whether these tests belong together as a "logical grouping". But also, the tests are a lot like separate class method definitions themselves, and they benefit from a space for much the same reasons.

gnprice · 2022-03-11T20:47:49Z

src/users/__tests__/userHelpers-test.js

+    const with_diacritis = eg.makeUser({ name: 'Frödö', email: '[email protected]' });
+    const without_diacritics = eg.makeUser({ name: 'Frodo', email: '[email protected]' });
+    const non_matching_user = eg.makeUser({ name: 'Zalix', email: '[email protected]' });


nit: camelCase for these names -- they're locals that don't come from the server API

gnprice · 2022-03-11T20:56:42Z

src/users/__tests__/userHelpers-test.js

+    const with_diacritis = eg.makeUser({ name: 'Frödö', email: '[email protected]' });
+    const without_diacritics = eg.makeUser({ name: 'Frodo', email: '[email protected]' });
+    const non_matching_user = eg.makeUser({ name: 'Zalix', email: '[email protected]' });
+    const selfUser = eg.makeUser({ name: 'Bob', email: '[email protected]' });


The name and email of this self-user aren't playing any role in the test -- it'd come out the same regardless of what they were.

In general, we try to minimize the amount of detail that a test specifies beyond what's relevant to it. That's helpful for reading and understanding the tests, and also just for making it less tedious to write the tests. So when there is such detail, we try to find ways to lean on the eg module to provide it instead.

Here, this could just be eg.makeUser(). Then a step further: instead of saying const selfUser at all, we could use eg.selfUser in the spot where this is used below.

An alternate direction would be to make the details of the self-user relevant: if they had a name that did match, then we'd be exercising the user.user_id !== ownUserId in the function. But I think actually best here is to leave that out -- that's covered already in a separate test, the one just above this.

gnprice · 2022-03-11T20:59:47Z

src/users/userHelpers.js

+): $ReadOnlyArray<AutocompleteOption> => {
+  const loweredFilter = filter.toLowerCase();
+  const isAscii = /^[a-z]+$/.test(loweredFilter);

-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()),
-  );
+  return users.filter(user => {


nit: I think this is slightly cleaner without the blank line.

Partly that's because this function is so short -- as that Google style item puts it, use "sparingly" within a function body, to create logical groupings. This short function doesn't need separate logical groupings.

It's also helpful in visually parsing this file in general, as this is one of a string of short functions -- it's helpful that each of them is one stanza.

gnprice · 2022-03-11T21:01:16Z

src/users/userHelpers.js

-        .startsWith(filter.toLowerCase()),
-  );
+  return users.filter(user => {
+    const full_name = isAscii ? typeahead.remove_diacritics(user.full_name) : user.full_name;


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.

gnprice · 2022-03-12T01:31:12Z

Merged zulip/zulip#21397 , and released shared-0.0.9 with that change. So this PR should be able to use that now.

Fingel · 2022-03-14T22:39:42Z

OK, I think this should be ready for review, for realsies this time. Sorry about that!

gnprice

Thanks @Fingel for the revisions! Just a couple of small comments remaining.

gnprice · 2022-03-15T01:02:33Z

src/users/__tests__/userHelpers-test.js

-    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


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.

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.

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.

gnprice · 2022-03-15T01:05:14Z

src/users/__tests__/userHelpers-test.js

+    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];


Let's exercise a couple more cases as mentioned here: #5292 (comment)

(done) if you do type a diacritic, it won't match a name that has that letter without the diacritic.

if you do type a diacritic, it won't match a name that has that letter with a different diacritic.

if you do type a diacritic, it won't match a name that, on some other letter where the query is just plain ASCII, does have a diacritic.

I believe these cases should be covered now.

Thanks. This one isn't, though:

if you do type a diacritic, it won't match a name that, on some other letter where the query is just plain ASCII, does have a diacritic.

For example, searching for "Frödo" won't find "Frödö". This is true even though the individual pieces of the query would match: "Fröd" (without the last "o") would match, because it matches exactly, and "do" would match (because it matches the "dö" part with diacritics removed.)

Let's also test the corresponding cases on filterUserThatContains as well as filterUserStartWith.

OK I think I follow you, check the latest revision.

This commit removes the unintuitive behavior of filtering users in a typeahead by their initials; for example 'az' would match Abba Zee. This is more likely to cause confusion than to be of real practical value.

This release added flow types for the shared `typeahead` module.

When invoking the typeahead filter for users it is desirable to be able to see results for users with diacritics in their names even if the filter does not contain the matching diacritics. This PR leverages @zulip/shared typeahead module in order to strip diacritics out of the filtering process, unless the user explicitly uses them in the filter. Fixes: zulip#3710

gnprice · 2022-03-16T17:49:56Z

Thanks @Fingel for the revisions! This looks good -- merging.

Fingel marked this pull request as draft March 11, 2022 20:46

gnprice reviewed Mar 11, 2022

View reviewed changes

Fingel force-pushed the typeahead-remove-diacritics branch from 933f107 to 0b3802e Compare March 11, 2022 22:35

gnprice mentioned this pull request Mar 12, 2022

shared: Add flow types for typeahead module. zulip/zulip#21397

Merged

Fingel force-pushed the typeahead-remove-diacritics branch from 0b3802e to 7bb3486 Compare March 14, 2022 22:28

Fingel marked this pull request as ready for review March 14, 2022 22:39

gnprice reviewed Mar 15, 2022

View reviewed changes

Fingel force-pushed the typeahead-remove-diacritics branch 3 times, most recently from 5b20a46 to 35872ea Compare March 16, 2022 17:44

Fingel added 3 commits March 16, 2022 10:48

typeahead: Remove filtering by user initials.

ba68387

This commit removes the unintuitive behavior of filtering users in a typeahead by their initials; for example 'az' would match Abba Zee. This is more likely to cause confusion than to be of real practical value.

shared: Upgrade from 0.0.8 to 0.0.9.

8dfcde1

This release added flow types for the shared `typeahead` module.

gnprice force-pushed the typeahead-remove-diacritics branch from 35872ea to d73e764 Compare March 16, 2022 17:49

gnprice merged commit d73e764 into zulip:main Mar 16, 2022

gnprice mentioned this pull request Mar 17, 2022

Untangle eg.makeUser names; simplify some tests #5302

Merged

		});
		test('returns users whose full_name has diacritics but otherwise contains filter', () => {

		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

Uh oh!

Typeahead remove diacritics #5292

Typeahead remove diacritics #5292

Uh oh!

Conversation

Fingel commented Mar 11, 2022

Uh oh!

chrisbobbe commented Mar 11, 2022

Uh oh!

Fingel commented Mar 11, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 12, 2022

Uh oh!

Fingel commented Mar 14, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Mar 16, 2022

Uh oh!

Uh oh!