Skip to content
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.

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

Add support for muted users #4655

Open
timabbott opened this issue Apr 13, 2021 · 12 comments
Open

Add support for muted users #4655

timabbott opened this issue Apr 13, 2021 · 12 comments
Assignees
Labels
api migrations P1 high-priority webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity.

Comments

@timabbott
Copy link
Member

We've finished merging the backend for muting users in zulip/zulip#16915.

This is an important feature for large open communities.

https://chat.zulip.org/api/mute-user is relevant for when mobile wants to implement muting, but the part that is likely to be more important is having the mobile apps hide messages from muted users. We should update this issue with a link to the Help Center article explaining the behavior once that's complete, but in short:

  • Messages sent by muted users are automatically marked as read (implemented in server).
  • All prior messages sent by muted users are marked read when they are muted (implemented in server).
  • Clients should arrange to avoid displaying messages sent by muted users in stream/huddle views, and hide such messages in some way. (Details not fully settled and will likely evolve on the server).
  • Muted users should not appear in lists of PM conversations / presence lists.
@gnprice
Copy link
Member

gnprice commented Apr 16, 2021

See chat discussion.

In particular here's my description of what'll be involved in the implementation:

Basically the plumbing work will all look like sort of a zulip-mobile version of the server/webapp's new-feature tutorial:

  • There's some new data in the /register response. Add it in src/api/initialDataTypes.js.
  • The new data needs a new key to be mentioned in fetch_event_types. Add that.
  • We'll need to keep the new data in Redux. In this case, it can go in state.realm -- that's not a good name, but it's generally where we put miscellaneous small pieces of data that don't live as part of some other object like a particular user or stream or message.
    • Add an appropriate type for the data to the relevant part of the GlobalState type, in src/reduxTypes.js.
      • Often the appropriate type will be different from the wire format found in the server API. Here, probably Immutable.Map<UserId, number> is good, where the values are the timestamp fields from the API's format. That one is good because we can efficiently look a user up in it and efficiently make changes.
    • Add code to the reducer to give an initial empty value, and to read the /register response data from a REALM_INIT action and put that in the new data structure in the Redux state.
    • Add a migration in src/boot/store.js to update existing stored data to match the new type.
  • We'll need to handle the events.
    • Add a type in src/api/eventTypes.js, following the API docs.
    • Add an event action type in src/actionTypes.js and src/actionConstants.js, following the example of EventUserStatusUpdateAction.
    • Add it to eventToAction.js following the examples in actionTypeOfEventType.
    • Now add code to the reducer to handle that action type by updating the data structure.
  • Write tests for the new reducer code.
  • Now we're maintaining the data in Redux! We can go on to use the data for whatever we need to use it for.

That looks like a lot of steps but a lot of them are trivial, like adding one line that's just like lots of other lines next to it. I think in total it doesn't add up to very much work.

Then for this feature, the main places involved in the UI will probably be

  • src/webview/html/messageAsHtml.js, where we'll want to produce some HTML that contains the actual name/avatar/content (in case the user decides to unhide it) but hidden under the "Hidden because it's from a user you blocked" UI
  • src/webview/static/base.css, for styling that UI
  • src/webview/js/js.js, in the click listener toward the end of the file, for responding to that UI
  • the pieces needed to hide muted users from lists of PM conversations etc. This will include at least
    • the plumbing behind PmConversationList
    • people autocomplete
    • ReactionUserList
    • probably when you search for a user to start a (1:1) PM thread with? And the similar UIs for a group-PM thread, and for inviting to a stream.
    • quite possibly others I'm not thinking of right now

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Apr 16, 2021

The new data needs a new key to be mentioned in fetch_event_types. Add that.

Should we mention the new key only on servers that support the feature? If so, I'd like to have eyes on the first four commits of #4666 (they're pretty small) as they're relevant for that. (Though empirically it seems servers disregard nonsense values in fetch_event_types that they don't recognize.)

@gnprice
Copy link
Member

gnprice commented Apr 19, 2021

Should we mention the new key only on servers that support the feature?

Good question. I believe the server will ignore a key there that it doesn't recognize, so we can freely just send it.

Partly I think that because we've added such keys before without making an effort to condition it on server version, and I'm not aware of problems it's caused. 😉

But it'd be good to fact-check that and then make sure the answer is written down somewhere helpful.

@gnprice
Copy link
Member

gnprice commented Apr 19, 2021

OK, I confirmed that it sure looks that way in the implementation -- i.e. that we can freely send the new key and old servers will just ignore it -- but also found that the API docs are silent on the matter. I've asked in chat:
https://chat.zulip.org/#narrow/stream/19-documentation/topic/unrecognized.20event.20types/near/1166037
and hopefully we can get it written into the docs.

@WesleyAC
Copy link
Contributor

All of the UI stuff this issue involves, off the top of my head:

  • Hiding messages from muted users
  • Blocking notifications from muted users
  • Hiding muted users in the PMs list

We might also want to:

  • Hide muted users in the typeahead list
  • Hide muted users in the "all users" list

@gnprice
Copy link
Member

gnprice commented Apr 19, 2021

  • Blocking notifications from muted users

This is a good point to bring up, because it will need to be implemented by the server. That's because:

  • On iOS, none of our client-side code runs before the notification appears; the system (or some SDK code built into the app? I'm not actually sure) just interprets what the server sent in a generic way.
  • On Android, we do have our own code involved there but it doesn't see the full app data we keep in Redux; it only sees the data that's in the notifications themselves.

It looks like the server issue zulip/zulip#8040 already mentions this prominently, so that should be all set when the issue is completed. But it'd be a key thing to do a manual smoketest of at the end.

@timabbott
Copy link
Member Author

Notifications I believe are already blocked via the fact that we clear all unread messages sent by the muted user on-muting (which will send remove-push-notifications). Let me write a little mini design document in the API docs for this.

@timabbott
Copy link
Member Author

I opened zulip/zulip#18222 to write down the design theory for this feature, which is about the same as what i wrote in the original issue description, but spells out the implications for notifications of marking things as read when sent.

@WesleyAC WesleyAC added the webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. label May 1, 2021
@chrisbobbe
Copy link
Contributor

I see that zulip/zulip#18222 has been merged! 🎉

@WesleyAC, are we ready to declare victory on this issue?

@WesleyAC
Copy link
Contributor

The one remaining bit of this issue is to hide usernames in PM headers (when in a Search or All Messages narrow, for instance). I'm going to leave this open to track that.

@timabbott
Copy link
Member Author

It could also be reasonable to open a dedicated issue for that remaining work -- I find that sort of remapping to a focused issue can save time in re-reading a long issue for adding a feature that we actually did add (with one detail not right), and make it easier to track and for contributors to work on what remains.

gnprice added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 17, 2022
In the previous commit, we learned we weren't updating an existing
message list on an event saying that a user was muted or unmuted.

This had gone unnoticed in part because we don't have good
systematic tests for this logic in the existing code; we
discovered it on writing tests for the new message-list-diffing
logic.  (Thanks, tests!)

The other reason this had gone unnoticed is that it actually takes
a bit of a coincidence for you to have a message list open while
muting or unmuting someone -- because we don't actually have any
UI for doing so in the mobile app!  There's one existing TODO
comment related to that; give it a reference to the issue for this
feature, zulip#4655.
@gnprice
Copy link
Member

gnprice commented Feb 17, 2022

Over in #5225 (comment) we recently discovered one gap in our muted-users support:

  • When you mute or unmute a user, we don't update an existing message list, if there is one. (We do show it correctly if you load the message list after doing so, or if the message list updates for some other reason.)

That caused me to notice another area that AFAICT we just didn't think about when implementing this issue (I don't see it in any of our discussion above, for example), but would be good to have in order to really say that the mobile app supports user muting:

  • We don't have any UI for muting or unmuting a user. Some good places to have it might be:
    • In an action sheet / bottom sheet upon long-pressing a user in the PM conversations list. (We don't currently have such a thing; a long-press behaves just like a plain press.)
    • In an action sheet / bottom sheet upon long-pressing a sender's name or avatar in the message list. (Ditto.)
    • In a button on the profile screen (which you get when you press a sender's name or avatar in the message list, or from the app bar in a PM conversation with them.)
    • In an action sheet / bottom sheet upon long-pressing a recipient header for a 1:1 PM thread. (We do have such a sheet, and there's a TODO(Add support for muted users #4655) comment calling for adding this functionality there. That seems pretty tucked away, though; I spent several minutes looking around the app to see if I could find a way to mute users, and didn't think of this one.)

The second and third of those four (long-pressing a sender name or avatar, and a button in the profile screen) would provide good ways to unmute a user as well as to mute them, because they're all pretty accessible even after you've muted a user, if you have any messages from them other than 1:1 PMs.

There's also the one Wesley mentioned in a comment a bit above here:

  • to hide usernames in PM headers (when in a Search or All Messages narrow, for instance).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api migrations P1 high-priority webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity.
Projects
None yet
Development

No branches or pull requests

4 participants