user_settings: Add settings to configure sending typing notifications and read receipts #19200

chdinesh1089 · 2021-07-09T19:59:51Z

Currently added only typing notifications settings. ~~Will add a commit for read receipts soon.~~(added)

Testing plan:
Verified through UI.

GIFs or screenshots:
image

chdinesh1089 · 2021-07-09T20:04:17Z

static/js/settings.js

+        send_stream_typing_notifications: $t({
+            defaultMessage: "Display whether I'm typing messages in streams to other users",
+        }),
+        send_private_typing_notifications: $t({
+            defaultMessage: "Display whether I'm typing private messages to recipients",
+        }),


It might be worthwhile to rephrase these but I'm unsure what would be nicer.

It’s easy to misparse “to” as “messages to” rather than “display to”. I propose

Let subscribers see when I’m typing messages in streams

Let recipients see when I’m typing private messages

Let senders see when I’ve read private messages

Those sound good to me. Changed.

Let senders see when I’ve read private messages

Made this 'Let other users see when I've read messages' as we are allowing everyone who has access to the message to see read receipts.

Did you mean to drop the “private”? And I think we need to be more specific than “other users”—maybe “participants”?

Yes, as we show read receipts in streams too. (#18935 is the PR for that, and there's some discussion at czo here)

Changed 'other users' to 'participants'. It is more specific but sounds a bit odd to me (probably it's just me. I think we can stick to it).

sahil839

Left one minor comment, otherwise LGTM.

static/js/settings_account.js

static/js/settings.js

chdinesh1089 · 2021-07-11T19:49:55Z

@timabbott I think we can make the send_private_typing_notifications setting functional here. Should we just ignore typing notifications from clients in the server with a json_success like we do with presence or should we also add js code to not send typing notification requests from the web app?

chdinesh1089 · 2021-07-20T19:06:04Z

Rebased as the settings endpoints merge is done. Asked the above question at https://chat.zulip.org/#narrow/stream/101-design/topic/privacy.2Fsecurity.20settings/near/1234961 to allow more discussion.

chdinesh1089 · 2021-07-25T19:56:50Z

@timabbott This is ready for review. Could you have a look?

Should we just ignore typing notifications from clients in the server with a json_success like we do with presence or should we also add js code to not send typing notification requests from the web app?

Did both. I think reducing the number of requests sent to the server would definitely help a bit.

static/js/typing.js

chdinesh1089 · 2021-07-25T20:03:47Z

zerver/views/typing.py

+        if not user_profile.send_stream_typing_notifications:
+            return json_success(
+                {
+                    "msg": "Sharing stream typing notifications turned off. This request will be ignored."


Does it make sense to do it this way? The other option of raising an error might break things in other clients.

Also, this string might benefit a bit from rephrasing it to be more concise.

Can you ask about this question in the "api design" stream?

I would probably prefer to raise an error if the client sent one of these notices in violation of the configuration; we should just make sure to check with client developers that doing so will not break anything.

Changed to raise errors.

(for later reference: link to the discussion: https://chat.zulip.org/#narrow/stream/378-api-design/topic/typing.20notifications.20endpoint

timabbott · 2021-07-28T23:00:04Z

zerver/openapi/zulip.yaml

+                        description: |
+                          Present if `update_display_settings` is present in `fetch_event_types`.
+
+                          Whether the user has chosen to send typing notifications in private messages.


These should have API changelog entries, **Changes** notes, etc.

Also we should have "typing notifications" link to the /help/ docs for the feature.

We don't seem to have a /help page for typing notifications. I think we only have API docs and developer docs (We'll probably have to tweak this once we merge stream typing notifications PR)

I guess we should add one soon?

Oh! we actually have /help page for typing notifications but it is hidden as a subsection in status-and-availability.

Linked "typing notifications" to those docs and rebased the PR to match to bump latest API_FEATURE_LEVEL.

I added links to that in all of the places we talk about them, for clarity.

timabbott · 2021-07-28T23:03:06Z

I think this will be mergeable with the API documentation changes I suggested, some copy-editing, and doing something to hide the settings UI checkboxes where the feature itself isn't merged yet when not in a development environment. E.g. an if development_only check in the template would probably be fine (and then we can add those to the PRs for the not yet finished features).

chdinesh1089 · 2021-07-30T20:05:00Z

Thanks for the review! Made the changes. Added an if page_params.development_environment in the template to hide unmerged features.

chdinesh1089 · 2021-08-04T14:25:36Z

zerver/openapi/zulip.yaml

+                      send_private_typing_notifications:
+                        type: boolean
+                        description: |
+                          Present if `update_display_settings` is present in `fetch_event_types`.
+
+                          Whether the user has chosen to send [typing notifications](/help/status-and-availability#typing-notifications)


Should these be moved to user_settings object above? Do we want to keep a copy here too with text like the following?

**Changes**: Deprecated in Zulip 5.0 (feature level 89). Clients connecting to newer servers should declare the `user_settings_object` client capability and access the `user_settings` object.

Good question, but I don't think it's necessary -- we already make clear in the top-level description that clients will need the user_settings_object capability.

This isn't necessary as `settings_checkbox.hbs` template used for presence enabled setting in `account_settings.hbs` takes care of checking/unchecking this checkbox.

From 430c5cb, we do not send events for settings added after introduction of `user_settings` event. This test wasn't considering that and started failing on adding new user_settings.

From 430c5cb, in `fetch_initial_state_data`, we only include legacy settings in the top level of `state` and the newer ones are stored in `state['user_settings']`. That should've had a corresponding change in apply_event(). Also, fixed a test related to this logic.

Note: These are not functional in enabling/disabling sending of typing notifications with this commit. Refactored the privacy settings update to keep the code less duplicated along with making the addition of new settings easier.

Changes the view code to skip sending typing events when these settings are disabled. Returns a json success response with a msg.

…ications.

This will be useful to let users enable/disable sharing read receipts once we add that feature. Note: Added "I've" to IGNORED_PHRASES in tools/lib/capitalization.py to avoid capitalization errors for the label text of this setting.

timabbott · 2021-10-08T00:45:19Z

Merged, after a somewhat extended pass of:

Renumbering migrations/feature levels.
Tweaking the OpenAPI documentation to be more consistent with our style (E.g. always ending sentences with a .).
Tweaking the error messages to have a more consistent style

I don't think I made any functional changes. Thanks for doing this! Now we just need to finish implementing the "Read receipts" and "stream typing notifications" features.

For the typing notifications one, I'd really appreciate your doing a bit of a writeup on the strategy for how you managed the issues around potential for duplicated code, since that's the thing I've been stuck on in thinking about that PR.

chdinesh1089 commented Jul 9, 2021

View reviewed changes

sahil839 reviewed Jul 10, 2021

View reviewed changes

static/js/settings_account.js Outdated Show resolved Hide resolved

chdinesh1089 force-pushed the privacy-settings branch 4 times, most recently from 61553e9 to 54f6814 Compare July 11, 2021 19:24

chdinesh1089 commented Jul 11, 2021

View reviewed changes

static/js/settings.js Outdated Show resolved Hide resolved

chdinesh1089 force-pushed the privacy-settings branch 4 times, most recently from f691f0d to 220b748 Compare July 13, 2021 12:33

chdinesh1089 force-pushed the privacy-settings branch from 220b748 to eac2c74 Compare July 20, 2021 17:31

zulipbot added the size: XL label Jul 20, 2021

chdinesh1089 force-pushed the privacy-settings branch from eac2c74 to 2e7930d Compare July 20, 2021 18:28

chdinesh1089 force-pushed the privacy-settings branch from 2e7930d to 6501673 Compare July 25, 2021 19:37

chdinesh1089 commented Jul 25, 2021

View reviewed changes

static/js/typing.js Outdated Show resolved Hide resolved

chdinesh1089 commented Jul 25, 2021

View reviewed changes

timabbott reviewed Jul 28, 2021

View reviewed changes

chdinesh1089 force-pushed the privacy-settings branch 2 times, most recently from 845f77c to a99190a Compare July 30, 2021 19:52

chdinesh1089 force-pushed the privacy-settings branch 2 times, most recently from cefbd1d to 4a82ce8 Compare August 3, 2021 17:21

chdinesh1089 commented Aug 4, 2021

View reviewed changes

chdinesh1089 mentioned this pull request Aug 13, 2021

user_settings: Create _legacy dicts for existing settings. #19561

Merged

zulipbot added the has conflicts label Sep 7, 2021

chdinesh1089 force-pushed the privacy-settings branch from 4a82ce8 to ad3b185 Compare October 3, 2021 18:48

zulipbot added has conflicts and removed has conflicts labels Oct 3, 2021

chdinesh1089 added 3 commits October 7, 2021 16:39

Remove $("#user_presence_enabled").val(page_params.presence_enabled);.

bb3953e

This isn't necessary as `settings_checkbox.hbs` template used for presence enabled setting in `account_settings.hbs` takes care of checking/unchecking this checkbox.

Fix change_user_settings_test to consider sending only one event.

47da353

From 430c5cb, we do not send events for settings added after introduction of `user_settings` event. This test wasn't considering that and started failing on adding new user_settings.

This was referenced Oct 8, 2021

Don't send typing notifications unless send_private_typing_notifications setting is enabled. zulip/zulip-mobile#5042

Open

Don't send typing notifications unless send_private_typing_notifications setting is enabled. zulip/zulip-terminal#1136

Closed

chdinesh1089 added 4 commits October 7, 2021 17:39

typing_notifications: Make privacy settings functional.

9cea88c

Changes the view code to skip sending typing events when these settings are disabled. Returns a json success response with a msg.

typing_notifications: Send requests only if send_private_typing_notif…

b41918e

…ications.

user_settings: Add send_read_receipts setting.

58184c2

This will be useful to let users enable/disable sharing read receipts once we add that feature. Note: Added "I've" to IGNORED_PHRASES in tools/lib/capitalization.py to avoid capitalization errors for the label text of this setting.

timabbott force-pushed the privacy-settings branch from ad3b185 to 58184c2 Compare October 8, 2021 00:42

zulipbot removed the has conflicts label Oct 8, 2021

timabbott merged commit 58184c2 into zulip:main Oct 8, 2021

chdinesh1089 mentioned this pull request Feb 8, 2022

Add typing indicators for streams. #17040

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user_settings: Add settings to configure sending typing notifications and read receipts #19200

user_settings: Add settings to configure sending typing notifications and read receipts #19200

chdinesh1089 commented Jul 9, 2021 •

edited

chdinesh1089 Jul 9, 2021

andersk Jul 11, 2021 •

edited

chdinesh1089 Jul 12, 2021

andersk Jul 13, 2021

chdinesh1089 Jul 13, 2021

sahil839 left a comment

chdinesh1089 commented Jul 11, 2021

chdinesh1089 commented Jul 20, 2021

chdinesh1089 commented Jul 25, 2021

chdinesh1089 Jul 25, 2021

timabbott Jul 28, 2021 •

edited

chdinesh1089 Jul 30, 2021

timabbott Jul 28, 2021

timabbott Jul 28, 2021

chdinesh1089 Jul 30, 2021

chdinesh1089 Aug 3, 2021

timabbott Oct 8, 2021

timabbott commented Jul 28, 2021

chdinesh1089 commented Jul 30, 2021

chdinesh1089 Aug 4, 2021 •

edited

timabbott Oct 8, 2021

timabbott commented Oct 8, 2021

user_settings: Add settings to configure sending typing notifications and read receipts #19200

user_settings: Add settings to configure sending typing notifications and read receipts #19200

Conversation

chdinesh1089 commented Jul 9, 2021 • edited

Choose a reason for hiding this comment

andersk Jul 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil839 left a comment

Choose a reason for hiding this comment

chdinesh1089 commented Jul 11, 2021

chdinesh1089 commented Jul 20, 2021

chdinesh1089 commented Jul 25, 2021

Choose a reason for hiding this comment

timabbott Jul 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Jul 28, 2021

chdinesh1089 commented Jul 30, 2021

chdinesh1089 Aug 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Oct 8, 2021

chdinesh1089 commented Jul 9, 2021 •

edited

andersk Jul 11, 2021 •

edited

timabbott Jul 28, 2021 •

edited

chdinesh1089 Aug 4, 2021 •

edited