Skip to content

autocomplete: Add and test MentionAutocompleteQuery #86

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

Jump to bottom
Merged
merged 2 commits into from
May 6, 2023
Merged

autocomplete: Add and test MentionAutocompleteQuery #86

merged 2 commits into from
May 6, 2023

Conversation

chrisbobbe
Copy link
Collaborator

This is atop my current revision of #84. It excludes much of the work we did in the office together, which I still have in a draft branch (and which I have an open question about: czo)—but this seems like a self-contained piece of logic that we'll need, and that we'll want to get right.

Related: #49

@chrisbobbe chrisbobbe requested a review from gnprice May 1, 2023 20:00
gnprice
gnprice reviewed May 3, 2023
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments.

test/example_data.dart Outdated
Comment on lines 75 to 81
// The use of JSON here is convenient in order to delegate parts of the data
// to helper functions. The main downside is that it loses static typing
// of the properties as we're constructing the data. That's probably OK
// because (a) this is only for tests; (b) the types do get checked
// dynamically in the constructor, so any ill-typing won't propagate further.
return User.fromJson({
'user_id': userId ?? 123, // TODO generate example IDs
Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't actually using such a helper function, better to leave it as a direct call to the generative constructor User.

Looking at zulip-mobile's example data, we do have such a helper function there but it's only for abstracting between User and CrossRealmBot. Since we've successfully avoided introducing that distinction here in zulip-flutter, it's likely we'll never need such a helper function.

(For streamMessage, by contrast, we're going to want to abstract between stream messages and DMs, much like we do in zulip-mobile.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah, I see I copied from streamMessage too hastily. 🙂

test/example_data.dart Outdated
@@ -71,6 +71,30 @@ StreamMessage streamMessage(
});
}

User user({int? userId, String? fullName}) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's order these similarly to api/model/model.dart, or when that doesn't suggest a workable ordering then like zulip-mobile's exampleData.js.

That means users come next to accounts; streams and subscriptions will be after those; then messages.

test/example_data.dart Outdated
return User.fromJson({
'user_id': userId ?? 123, // TODO generate example IDs
'email': '[email protected]',
'full_name': fullName ?? '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.

Bit more realistic, in making the name a name for the user rather than a self-reference:

Suggested change
'full_name': fullName ?? 'Full Name',
'full_name': fullName ?? 'A User',

But I think better yet: default to a newly-generated random name. That will help avoid confusion when debugging a failing test, if there are several users involved who had automatic names.

(Random names can be out of scope for this PR, though — just bundle them into the same TODO as generating IDs. Same for emails.)

lib/model/autocomplete.dart Outdated
Comment on lines 8 to 9


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

test/model/autocomplete_test.dart
Comment on lines +31 to +38
doCheck('Full Full', eg.user(fullName: 'Full Name'), false);
doCheck('Name Name', eg.user(fullName: 'Full Name'), false);
Copy link
Member

Choose a reason for hiding this comment

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

Also test that "full full" does match "Full Full Name" and "Full Name Full".

test/model/autocomplete_test.dart
doCheck('Full', eg.user(fullName: 'Full Name'), true);
doCheck('Name', eg.user(fullName: 'Full Name'), true);
doCheck('Full Name', eg.user(fullName: 'Fully Named'), true);
doCheck('Full Four', eg.user(fullName: 'Full Name Four Words'), true);
Copy link
Member

Choose a reason for hiding this comment

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

Also test that "F Full" and "Four F" don't match this name, while "Full F" and "F Four" do.

test/model/autocomplete_test.dart
expected ? check(result).isTrue() : check(result).isFalse();
}

doCheck('', eg.user(fullName: 'Full Name'), true);
Copy link
Member

Choose a reason for hiding this comment

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

Also include tests with empty fullName ­— one where query is also empty, one where nonempty.

That isn't a super likely case, but it wouldn't be hard to have a bug where the search crashed on it.

@chrisbobbe chrisbobbe force-pushed the pr-mention-autocomplete-query-test-user branch from 4712aa7 to 511959d Compare May 4, 2023 18:07
@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed (with the commits from #84 refreshed with my current revision of that).

gnprice
gnprice reviewed May 4, 2023
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

One nit; otherwise lgtm once #84 is ready.

test/example_data.dart Outdated
Comment on lines 24 to 26
}

final Uri realmUrl = Uri.parse('https://chat.example/');
Copy link
Member

Choose a reason for hiding this comment

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

nit: user data after realm-wide / server-wide data

@chrisbobbe chrisbobbe force-pushed the pr-mention-autocomplete-query-test-user branch from 511959d to fa4434c Compare May 4, 2023 21:05
@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! Revision pushed.

chrisbobbe added 2 commits May 5, 2023 12:01
@chrisbobbe
test: Add example data for User
b1c0641
@chrisbobbe
autocomplete: Add and test MentionAutocompleteQuery
8895ce8 
This is a small step toward zulip#49, without changing any UI. Here we
have a simple function `testUser` to test whether a given User
matches a query.

Related: zulip#49
@chrisbobbe chrisbobbe force-pushed the pr-mention-autocomplete-query-test-user branch from fa4434c to 8895ce8 Compare May 5, 2023 16:02
@chrisbobbe
Copy link
Collaborator Author

(Rebased atop main now that #84 is in.)

@gnprice gnprice merged commit 8895ce8 into zulip:main May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisbobbe @gnprice