Display and autocomplete PM recipients name along with their emails

[prah23]

Modified the display to use names along with delivery emails, and enabled the autocomplete for the to_write_box.
The commits do the following (in order):

• Type the emails parameter of the private_box_view() as List[str].
• Display the name of the user along with the email to make it more user-friendly.
• Add a helper function to match users, considering matches in name, email, or a combination of both in the name <email> format.
• Enable autocomplete for the users' names.

[neiljp]

@prah23 I just gave this a go and it works fairly well for autocomplete, but I've noticed a few important issues:

• x causes a crash pretty reliably
• validation appears to be less effective than currently; if a user deletes some of the email address, then tab just moves to the content, whereas before it complained it wasn't valid

I was thinking of merging the first commit, as it's a good standalone refactor in a direction I know we want to move, but I noticed that you've not changed the calling location in that commit?

[prah23]

@neiljp The refactor commit has been modified to update the calling function as well.
I think the crash on x is because of an empty email parameter. I'll add the necessary checks and update it.

[prah23]

@neiljp I've modified the to_write_box and the validation.

• x shouldn't cause a crash anymore (or any empty email for that matter).
• recipients_emails is updated both before a message is sent and before cycling to the compose box (i.e., validation) using a helper function I've added, that uses re.findall() to recognize the emails.

[prah23]

@zulipbot remove "PR needs update"

[prah23]

@zulipbot add "PR needs review"

[neiljp]

@prah23 I've cherry-picked the first commit now, and the x issue appears to be resolved 👍

For the remaining commits I have some practical feedback from manual testing, separate to the code right now:

• I just noticed this crashes when resuming a draft. Given this and the previous issue with x, can we tidy up the code somehow to improve upon finding other similar issues? (this occurs at least from the first remaining commit)
• It seems OK that the name and angle brackets (<>) around the email are not validated in the first remaining commit, as we're just showing extra information. However, users can edit around the email and it still look OK; ideally it would be good to autocomplete "around" the email text after a good validation, replacing the recipient text with a newly polished version, eg. an 1 <[email protected] -> Human 1 <[email protected]>
• For the autocomplete, it's confusing to have the first email work and not later - it actually stops users typing in names? So, I'd prefer that to be in the last commit too.

The show-name commit is already an improvement, and particularly if we can get the 'validation polishes to-field' (which could be a separate commit) then we can merge that incrementally before the full autocomplete.

Other possible improvements you could include here or follow-up work, that I don't think we have noted in issues yet:

• Place the To: in a column, like @Ezio-Sarthak did with the stream markers - that makes a good clean separation between user details and the title/caption, particularly with multiple/long users/emails on narrower terminals
• Try to keep parts of user full names together, and possibly on the same line as the email addresses; this needs to be 'soft' to allow names to wrap in the long-name/email case, but has the potential to make the list of user names tidier.
• style the user name and email differently? Style the 'To:' differently, maybe?

[prah23]

We could definitely refine the design further. I'll open a separate PR for that once this is done, since that seems to be a design change, and this work is majorly on a structural change.

[prah23]

@zulipbot remove "PR needs update"

[prah23]

@zulipbot add "PR needs review"

[neiljp]

@prah23 Thanks for working on this step by step 👍

I've given this a try and it looks good, but it asserts the text is wrong with anything but the full original text, which is a regression from this step to what we had before - there's text (ie. not the raw email) that is now important only for display (not delivery) which I can delete and be unable to use the UI. I know you said you'd skipped autocomplete at this stage, but can we validate just on the email for now? That is, if I pressed R and ended up with some user <a@b>, I could delete parts of the text including up to everything except a@b and have it still work?

I know this is now one commit, but you have multiple things happening in it and it would be clearer if split. In particular you have

This commit also tidies up the private_box_view() method by typing emails as List[str], and updating the corresponding calls.

That would be a good first separate refactor commit, then followed by a commit which does the 'feature' part:

Currently, PM narrows only display the delivery emails of the users. This
commit modifies the to_write_box to also display the names of the users
along with their emails.

That split allows you to focus each commit title and text on one change, and makes it easier to review :)
I'll take a closer look at each part again, ideally after splitting, and such splitting may make it easier to eg. merge the refactor separately to the second commit (though that's not always the case, as refactors can be just to make adding a feature easier, so may be merged at the same time)

[prah23]

@neiljp Thank you for your patience and quick response!
I may have misunderstood what you said for the autocomplete - I was of the impression that you wanted me to conduct a more severe validation check as well. I've updated it again and have also split it into two commits.

[prah23]

@zulipbot remove "PR needs update"

[prah23]

@zulipbot add "PR needs review"

[neiljp]

@prah23 Thanks for working on this - I've just merged this and it will be great to have this feature! It's present in the repo as the 4 commits ending with b03d371 🎉

I've merged it manually to make some minor changes, mainly just layout changes, using list comprehensions, and removing an unnecessary fixture. Plus this fixes #587 so I indicated that in the text of the commit that fixes it 🎉