autocomplete: Exclude deactivated users from @-mention autocomplete popup

[Khader-1]

Logic Implemented

autocomplete: Exclude deactivated users from @-mention autocomplete popup

Deactivated users can not be mentioned, so this ensures that deactivated users are excluded from the list that gets rendered in autocomplete popup.

compose test: Check ID still included if same-name users deactivated

While autocomplete does not include deactivated users, there still is a possibility that same-name users with one or more of them deactivated gets mentioned, which will make the mentioned user ambiguous without the id, this is explained in #451.

Fixes #451

[sm-sayedi]

Better than before, but still needs some improvements.

It should pass tests (so test updates needed by a change should be in the same commit as the original change, not a separate “fix the tests that were broken by the last commit” commit).

As stated in the Zulip Commit Discipline page, the tests for a change should be in the same commit.

Also, instead of closing a PR and then opening a new one just for the sake of some changes, there is an easy way of doing it explained in this page.

[Khader-1]

It should pass tests (so test updates needed by a change should be in the same commit as the original change, not a separate “fix the tests that were broken by the last commit” commit).

@sm-sayedi You're right although the tests in the second commit do not test the same change that the first one introduces. they depend on some of the changes.

Regarding the rebase.. I learned it hard way!

Thanks for you helpful guidance☺️

[gnprice]

Thanks @Khader-1 for the contribution, and @sm-sayedi for the initial reviews! Comments below on this version.

[Khader-1]

This has gone through many iterations! hopefully each iteration is a step in the right direction.
I think it is ready to be reviewed now

[gnprice]

Thanks @Khader-1! This is quite close to merge.

A few small comments below. The one other area that needs a bit of revision before we merge this is the formatting of the commit messages.

• For the formatting around the "[nfc]" marker, see examples at https://github.com/zulip/zulip-mobile/blob/main/docs/glossary.md#nfc .
• For the "fixes" line, see https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#fixes-format .
• See Zulip's Git style guide: https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#formatting-guidelines
  on line-wrapping for the prose in the body of your commit message.
• The commit summary can go slightly longer than 70 columns when there's a lot to express, but this one is 95 columns and longer than it needs to be:
  autocomplete test[nfc]: Use a test group to handle MentionAutocompleteQuery.testUser test cases
  • Saying just testUser instead of MentionAutocompleteQuery.testUser is the main part of a fix.
  • You can also make the wording more concise; summary lines can be in a terse "headlinese" style. To see examples, try git log --oneline --author Greg or git log --oneline --author cbobbe.

To see example commit messages in general, here's a slightly broader version of that last pair of commands:

$ git log --stat --author '(greg|cbobbe)@zulip' -P

[gnprice]

Just to make this test case fully self-contained, let's give it a second line that looks exactly the same except the user is active, and does match.

That line will be identical to one of the lines in the existing test case, so in a sense it's redundant. But it makes a useful baseline for the reader to compare this other check to. It means that the reader can look at this test case and be confident that it really is testing what it's meant to test — that there isn't some accident where the user stopped matching because something changed about the name matching, or whatever — without having to look around at other tests, or to look back at the implementation that's being tested.

[Khader-1]

This is not tagged as [nfc] because it does actually add functionality or at least it tests it's existence. Besides the new test case is mentioned as a part of the issue.

[gnprice]

This is not tagged as [nfc] because it does actually add functionality or at least it tests it's existence. Besides the new test case is mentioned as a part of the issue.

Sounds good, yeah.

(I assume you're referring to the second commit in the series:
62015a2 compose test [nfc]: Make trailing whitespace explicit in test cases
9672a0a compose test: Check ID still included if same-name users deactivated
a88222a autocomplete [nfc]: factor out _testName filtering helper
f9f476f autocomplete test [nfc]: Use test group to handle testUser cases
854a18c autocomplete: Exclude user from autocomplete popup if deactivated

because that's the one that was marked NFC in the previous revision, and isn't in this one.)

[gnprice]

Thanks @Khader-1 for the revision! This all looks good except two small nits in commit messages. I'll fix those up and merge.

autocomplete [nfc]: factor out _testName filtering helper

The summary line should be in sentence case after the prefix: so "Factor out …".

autocomplete: Exclude user from autocomplete popup if deactivated

Deactivated users can not be mentioned, so this commit ensures that
deactivated users are excluded from the list that gets rendered in
autocomplete popup.

Fixes #451

The "fixes" line should have a colon: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#fixes-format