Read receipts #18935

chdinesh1089 · 2021-06-21T20:28:20Z

timabbott · 2021-06-21T23:44:42Z

zerver/models.py

+        # Use this for Django ORM queries to access read messages.
+        # This custom SQL plays nice with our partial indexes.  Grep
+        # the code for example usage.
+        return "flags & 1 <> 0"


How did you confirm that this works with the partial indexes? It seems very possible to me that it does not and we need to create an index for this purpose.

Oh, I see this comment is copied. I would expect this to not have a good index.

That said, I don't think we need an index for this operation -- we're fetching with a given message_id, which does have a nice index, and it's fine to scan all the matches. So I think we should just replace the comment with one warning that this operation likely does not have an index, so should only be used for queries where another good index is already used.

Oh, I'm not very familiar with partial indices. I thought this would be the same as where_unread, and got the comment from it. Will change the comment and spend a bit of time reading about partial indices. Thanks! 🙂

Changed the comment to

# It is likely that this operation doesn't have an index. So, # use this only for queries where another good index is present.

timabbott · 2021-06-21T23:47:10Z

zerver/views/read_indicators.py

+        .values_list("user_profile_id", flat=True)
+    )
+
+    return json_success({"user_ids": list(user_ids)})


This looks about right to me! I think we might need additional filtering to exclude deactivated users; I think that a UX question I don't understand clearly.

I'm confused about excluding deactivated users. Since their read activity was before their deactivation, we may have to show their read receipts too. Like we presumably show deactivated users' mentions in messages before their account deactivation.

Did this in a different commit to make it easier to remove in case we decide we don't want to filter.

timabbott · 2021-06-21T23:48:36Z

zproject/urls.py

@@ -354,6 +355,8 @@
    # typing -> zerver.views.typing
    # POST sends a typing notification event to recipients
    rest_path("typing", POST=send_notification_backend),
+    # read_by -> zerver.views.read_indicators
+    rest_path("read_by/<int:message_id>", GET=read_by_users),


This should be something like GET /message/{message_id}/read_receipts, to follow REST conventions.

Ok, changed. Thanks, didn't know.

timabbott · 2021-06-21T23:51:28Z

static/js/click_handlers.js

@@ -205,6 +206,11 @@ export function initialize() {
        message_flags.toggle_starred_and_update_server(message);
    });

+    $("#main_div").on("click", ".show_readby_users", (e) => {


I think show_read_indicators is probably a better name.

I'd also consider clarifying the interface between this and read_indicators.js, to try to do the rows.js logic here only.

Changed to show_read_receipts and moved the rows.js logic to this.

timabbott · 2021-06-21T23:52:41Z

This seems plausible as a direction; posted a few quick comments on details and naming. I think it'd be best to pick a single variable name for the feature, e.g. read_indicators or read_receipts and stick to it; might be worth your starting a conversation about which to pick in chat.zulip.org.

chdinesh1089 · 2021-06-22T19:34:49Z

Thanks for the review, @timabbott! Will continue with the rest of the work after making those changes.

chdinesh1089 · 2021-06-28T18:07:54Z

As an update, I made the suggested changes, added backend tests, made slight changes to js code like hiding the author of the message in the tooltip. I'm going to write API docs and examples and then add a few js tests.

chdinesh1089 · 2021-07-04T19:50:09Z

I think the API endpoint part of this is done. I might have to merge the first two commits. Kept them separate for now to keep it easier to review.

tools/test-js-with-node

timabbott · 2022-04-26T00:32:22Z

static/templates/actions_popover_content.hbs

+        <a class="view_read_receipts" data-message-id="{{message_id}}" tabindex="0">
+            <i class="zulip-icon zulip-icon-readreceipts" aria-label="{{#tr}}View read receipts{{/tr}}"></i> {{t "View read receipts" }}
+        </a>
+    </li>


This should be hidden for spectators, who have no access to the relevant API endpoint. I think this can be done by just adding class="hidden-for-spectators" to the li element.

Added this class.

alya · 2022-04-26T22:26:52Z

The hover effects look nice! Playing with the PR, I came across an error when clicking on "View full profile" from the profile modal:

I think the right solution is to close the read receipts modal when any of the links in the profile menu are clicked, whether or not they would result in an error. E.g. if you just clicked on "Send private message", you're pretty clearly moving on to the next task in any case.

Once this is fixed, I think we should be good to go!

timabbott · 2022-04-28T00:05:06Z

I concur with that assessment. (There's also another API changelog renumbering, but those are quick).

timabbott · 2022-07-31T20:04:16Z

Updated with a handful of important changes:

I've addressed a few mobile size responsive bugs in the CSS, and documented those decisions in comments.
I added a new commit commit to solve the "Show full profile" modal problem that this had been blocked on -- I think it is a much nicer experience for not close the UI when clicking on a user, and I had the time to try to make that happen. I went through a few iterations of this plan, in part because the "Manage user" modal had the same issue.
I squashed the frontend changes into the API commit, since we're merging them at the same time and doing so supports a better commit message.
I resolved merge conflicts and some less visible things (like micromodal: true no longer being necessary, and the API documentation description format changing slightly to no longer hand-entering the URLs).
I reworked a bunch of the tests to avoid unnecessary .login steps, by having mark_message_read use api_post instead of client_post. This, in my view, makes it a lot more readable / less fragile.
I added a block comment in the view code discussing the ~6 corner cases that are important here, and our reasoning around their choices.
I added a dozen or additional asserts to the tests, generally motivated by things I thought about in writing that big comment

With those changes, I think this is good to merge to main. I'll post a few comments on immediate follow-ups, like documentation, that might block deploying this to Zulip Cloud, but those probably will be done by different folks, and thus can be a separate PR.

The previous version with e.target would give the element that was clicked lying inside an element with '.view_user_profile'. One would usually expect "data-user-id" to be attached to the same element with ".view_user_profile" instead of any of its children. So, to just look for "data-user-id" in the element with that class, instead of any of its chidren, this commit changes e.target to e.currentTarget.

Previously, our modal system prevented opening a modal when one was already open. It appears this was implemented to work around the fact that we're using Micromodal selectors to determine if a modal is open (and those don't update until after an animation frame). We'd like to support opening the full user profile and manage user modals while read receipts is open. While we could work around this in that place, it feels like one needs a lot of documentation in order to add a setTimeout in those code paths. So we instead make open_modal support this, with a guard to prevent infinite recursion in case of future bugs. Note that dialog_widget was already closing modals before opening the next one, so this is a behavior change only for our 3 modals that do not use dialog_widget. (I'm not sure why the `dialog_widget` modals did not already require a delay, but likely there's some CSS difference). We likely will want to redo this to instead use a better state tracking system. See https://chat.zulip.org/#narrow/stream/49-development-help/topic/close.20and.20open.20another.20modal.20immediately for discussion.

Now that we've split this out from the enormous actions.py, it makes sense to include this in the set of inputs for generating the database.

timabbott · 2022-08-12T19:24:18Z

Following the integration of #22657, this needed a few tweaks:

I discovered that the read_receipts modal didn't handle errors from the server and fixed that, though the visuals aren't great as a result of a quirk of how it's implemented (there ends up being a minimum height for the red region). I think this is not important enough to block merging it, since we don't expect errors to happen in practice with any frequency, and likely will want to rewrite the modal as a dialog_widget extension to resolve it.
I added enforcement at the API layer of the new realm-level setting, with a new backend test.
I made the "Show read receipts" option visible in popover menus only if the new realm setting allows read receipts.
I made the organization creation logic initialize the read receipts setting the same way we do it for existing realms. In practice, since every realm starts out as invitation required, this makes it on by default for new organizations.
Renumbered the API feature level; currently going with 137 since I expect this to merge just after Add realm setting for enabling read receipts. #22694.

With those changes, I've marked this to merge once CI passes.

I think there are two major forms of finishing work that we'll want to do:

The feature needs a nice /help/ page before we deploy this to Zulip Cloud; @laurynmm @alya can you coordinate on that?
We may want to move the precise location for the settings checkbox; I think the section it's in may be fine, but maybe it should be next to the "message content in email notifications".

If necessary we can slow roll the feature in a few different ways; the simplest is just to delay the migration that enables the realm setting for existing closed organizations, such that only those few organizations that discover it and enable it would have it at first (but the privacy setting would be available to everyone).

Adds an API endpoint for accessing read receipts for other users, as well as a modal UI for displaying that information. Enables the previously merged privacy settings UI for managing whether a user makes read receipts data available to other users. Documentation is pending, and we'll likely want to link to the documentation with help_settings_link once it is complete. Fixes zulip#3618. Co-authored-by: Tim Abbott <tabbott@zulip.com>

zulipbot added the size: L label Jun 21, 2021

timabbott reviewed Jun 21, 2021

View reviewed changes

chdinesh1089 force-pushed the read-indicators branch from 092727e to 722de40 Compare June 24, 2021 18:59

zulipbot added size: XL and removed size: L labels Jun 24, 2021

chdinesh1089 force-pushed the read-indicators branch 3 times, most recently from a4d3ec9 to 72d38f6 Compare June 28, 2021 17:58

chdinesh1089 force-pushed the read-indicators branch 3 times, most recently from ee74091 to 0becb91 Compare July 4, 2021 19:15

chdinesh1089 commented Jul 4, 2021

View reviewed changes

tools/test-js-with-node Outdated Show resolved Hide resolved

chdinesh1089 force-pushed the read-indicators branch from 0becb91 to 9653b53 Compare July 8, 2021 19:29

chdinesh1089 mentioned this pull request Jul 13, 2021

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

Merged

chdinesh1089 force-pushed the read-indicators branch from 9653b53 to e89aed9 Compare July 13, 2021 20:09

chdinesh1089 force-pushed the read-indicators branch from e89aed9 to 5a5d58b Compare July 21, 2021 19:25

chdinesh1089 force-pushed the read-indicators branch from 5a5d58b to cb85c3a Compare August 4, 2021 19:05

chdinesh1089 changed the title ~~[WIP] Read indicators~~ Read receipts Aug 20, 2021

zulipbot added the has conflicts label Sep 7, 2021

chdinesh1089 force-pushed the read-indicators branch from 41f81a9 to 1172c52 Compare February 28, 2022 14:14

zulipbot removed the has conflicts label Feb 28, 2022

chdinesh1089 force-pushed the read-indicators branch from f9cf39c to fbee328 Compare March 4, 2022 12:20

zulipbot added the has conflicts label Mar 10, 2022

timabbott reviewed Apr 26, 2022

View reviewed changes

chdinesh1089 force-pushed the read-indicators branch from 12f1a09 to 53b09ea Compare April 26, 2022 06:10

zulipbot removed the has conflicts label Apr 26, 2022

chdinesh1089 force-pushed the read-indicators branch from 53b09ea to 0b0866c Compare April 26, 2022 18:30

zulipbot added the has conflicts label Apr 26, 2022

alya mentioned this pull request Apr 27, 2022

Make it possible to view read receipts zulip/zulip-mobile#5367

Closed

timabbott added the integration review Added by maintainers when a PR may be ready for integration. label Jul 28, 2022

zulipbot added has conflicts and removed has conflicts labels Jul 30, 2022

timabbott force-pushed the read-indicators branch from fb0274b to c0b2a5d Compare July 31, 2022 18:51

zulipbot removed the has conflicts label Jul 31, 2022

timabbott force-pushed the read-indicators branch 2 times, most recently from 9ea7425 to 131947a Compare July 31, 2022 19:57

timabbott force-pushed the read-indicators branch from 131947a to d559292 Compare July 31, 2022 20:05

alya mentioned this pull request Aug 3, 2022

Add organization setting for enabling/disabling read receipts #22657

Closed

zulipbot added the has conflicts label Aug 6, 2022

chdinesh1089 and others added 2 commits August 12, 2022 11:26

timabbott force-pushed the read-indicators branch from d559292 to 27e9ef5 Compare August 12, 2022 18:58

zulipbot removed the has conflicts label Aug 12, 2022

test_fixtures: Rebuild database when create_realm.py changes.

05cabc8

Now that we've split this out from the enormous actions.py, it makes sense to include this in the set of inputs for generating the database.

timabbott force-pushed the read-indicators branch from 27e9ef5 to 432019c Compare August 12, 2022 19:11

timabbott force-pushed the read-indicators branch from 432019c to 13041e0 Compare August 12, 2022 19:32

timabbott merged commit 48d2783 into zulip:main Aug 12, 2022

neiljp mentioned this pull request Aug 18, 2022

Make it possible to view read receipts zulip/zulip-terminal#1247

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read receipts #18935

Read receipts #18935

chdinesh1089 commented Jun 21, 2021

timabbott Jun 21, 2021 •

edited

timabbott Jun 21, 2021

chdinesh1089 Jun 22, 2021

chdinesh1089 Jul 4, 2021

timabbott Jun 21, 2021

chdinesh1089 Jun 22, 2021

chdinesh1089 Jul 4, 2021

timabbott Jun 21, 2021 •

edited

chdinesh1089 Jul 4, 2021

timabbott Jun 21, 2021

chdinesh1089 Jul 4, 2021

timabbott commented Jun 21, 2021

chdinesh1089 commented Jun 22, 2021

chdinesh1089 commented Jun 28, 2021

chdinesh1089 commented Jul 4, 2021

timabbott Apr 26, 2022

chdinesh1089 Apr 26, 2022

alya commented Apr 26, 2022

timabbott commented Apr 28, 2022

timabbott commented Jul 31, 2022

timabbott commented Aug 12, 2022 •

edited

Read receipts #18935

Read receipts #18935

Conversation

chdinesh1089 commented Jun 21, 2021

timabbott Jun 21, 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 Jun 21, 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

timabbott commented Jun 21, 2021

chdinesh1089 commented Jun 22, 2021

chdinesh1089 commented Jun 28, 2021

chdinesh1089 commented Jul 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alya commented Apr 26, 2022

timabbott commented Apr 28, 2022

timabbott commented Jul 31, 2022

timabbott commented Aug 12, 2022 • edited

timabbott Jun 21, 2021 •

edited

timabbott Jun 21, 2021 •

edited

timabbott commented Aug 12, 2022 •

edited