Adding user groups to PM recipients typeahead #9978
Conversation
|
Hello @zulip/server-compose members, this pull request was labeled with the "area: compose" label, so you may want to check it out! |
static/js/composebox_typeahead.js
Outdated
| @@ -97,6 +97,14 @@ function query_matches_user_group_or_stream(query, user_group_or_stream) { | |||
| return query_matches_source_attrs(query, user_group_or_stream, ["name", "description"], " "); | |||
| } | |||
|
|
|||
| function query_matches_person_or_data(query, item) { | |||
There was a problem hiding this comment.
Why not name it query_matches_person_or_user_group?
static/js/composebox_typeahead.js
Outdated
| }, | ||
| updater: function (item) { | ||
| compose_pm_pill.set_from_typeahead(item); | ||
| if (user_groups.is_user_group(item)) { | ||
| item.members.keys().forEach(function (user_id) { |
There was a problem hiding this comment.
This can be changed to
item.members.keys()
.map(toUser)
.filter(notCurrentUser)
.forEach(set_from_typeahead)
synicalsyntax
force-pushed
the
user-groups-pms
branch
from
1406497
to
d92cae3
Compare
|
@gioragutt Thanks for your feedback! I've updated the PR accordingly. |
synicalsyntax
force-pushed
the
user-groups-pms
branch
2 times, most recently
from
99b79eb
to
55a66e7
Compare
|
I tested this pretty thoroughly and it works great! I'm not sure if all the commits here are completely atomic; we might want to squash them when we push. I'm also not sure if we prefer |
|
@showell Would you prefer that I squish them all into one commit, or are there specific commits that you'd like squashed together? EDIT: I've went ahead and squished them all into one commit. If there's specific changes you'd like to be separate, let me know please! |
synicalsyntax
force-pushed
the
user-groups-pms
branch
from
55a66e7
to
bd8b356
Compare
| appended_names.push(item.full_name); | ||
| }; | ||
|
|
||
| var cleared = false; |
There was a problem hiding this comment.
The CI fails due to the cleared variable never been used.
If the cleared value is really not to be used, replace clear_text below with function(){}
There was a problem hiding this comment.
Thanks for catching this (don't know why I forgot to commit that line), tests are now passing
synicalsyntax
force-pushed
the
user-groups-pms
branch
2 times, most recently
from
47e289e
to
6cd743d
Compare
|
@synicalsyntax this is a nice feature, but the commit history needs some cleanup. Can you split the commits to have a series of preparatory refactoring commits (e.g. extracting the functions to I think we will want to do the deduplication, as well, but we can talk about strategy for that a bit later, after we merge the preparatory refactors. |
|
@timabbott I used to have 5 commits for this PR before Steve asked me to squish my commits. Is it possible to restore a previous version of a pull request? |
|
You should be able to find it locally with I think Steve's comment was that the 5 commits were not split up properly, which would be an important issue (since reviewing PRs with a commit structure that's kinda broken takes a lot more time to review). |
synicalsyntax
force-pushed
the
user-groups-pms
branch
2 times, most recently
from
8a06642
to
1bab817
Compare
We use these new functions in the message compose typeahead so that they can also be used in a PM recipients typeahead with both people and user groups.
When a user group is selected, we add PM pills for each user in the group instead of creating a PM pill for the user group. Fixes zulip#9971.
synicalsyntax
force-pushed
the
user-groups-pms
branch
from
1bab817
to
78924e0
Compare
|
Sorry, I did say "squash" although there was a "might" in there, in my defense. I could have been a bit more clear, hope you're able to resurrect stuff without too much trouble. |
|
Merged, thanks @synicalsyntax! This new commit structure was great. As a quick follow-up, I think it might be worth having a test for the specific corner case of already having one user in a group in the typeahead and adding a second; I feel like that's one of the more important corner cases here, and I don't see an obvious place where we're testing that. |
|
@synicalsyntax, or anyone else here for that matter, if you have any git related question, I'm more than willing to answer. |
This PR has a series of commits refactoring the compose box typeahead to support user groups in the PM recipient typeahead.
One possible followup is excluding already inserted user groups from the compose box typeahead. Not sure how this would be accomplished though, given we don't insert actual groups but members of the user groups instead, but measures have been taken to exclude inserted users.
Fixes #9971.