presence: Replace "unavailable" status with "invisible mode" #5531

chrisbobbe · 2022-10-26T13:03:31Z

gnprice · 2022-10-27T00:16:56Z

src/api/modelTypes.js

+ * User settings in common between PATCH /settings and POST /register
+ *
+ * (In particular, `user_settings` in the /register response.)
+ *
+ * - PATCH /settings doc: https://zulip.com/api/update-settings
+ * - POST /register doc: https://zulip.com/api/register-queue


Hmm, do these really differ? That's awkward; I feel like that's probably unintentional, and something we'd want to fix in the API. Let's have an #api design thread about it.

That may in turn shape how we'll want to organize these types -- if the current state is indeed unintended, we'll want the presentation to be focused on the way it's supposed to work conceptually, and then any discrepancies between that and what the server currently does send can be patched up with things like spreads and $Rest<…> or (depending on the nature of the discrepancy) even outright ignored.

Sure, makes sense: https://chat.zulip.org/#narrow/stream/378-api-design/topic/User.20settings.20discrepancies/near/1456337

gnprice · 2022-10-27T00:23:09Z

src/api/initialDataTypes.js

+    // The PATCH /settings doc doesn't mention these, but that's OK; mobile
+    // doesn't need to update them.
+    +available_notification_sounds: $ReadOnlyArray<string>,
+    +emojiset_choices: $ReadOnlyArray<{| +key: string, +text: string |}>,


Hmm, I see.

Yeah, looking at those, my reaction is that they really aren't user settings at all -- they're not something a user can set. So it's quite right that /api/update-settings doesn't accept these, and really they shouldn't appear in the /api/register-queue user_settings object either.

What these are is realm- or server-level information, and specifically information which is useful when presenting a user-settings UI -- it's information that tells you what choices should appear in that UI. So that gives a hint of how the mistake came about of putting them in the user_settings object.

Let's have that #api design thread to confirm, but my suspicion is that we'll all agree that these shouldn't be in user_settings.

In that case, the types in this version are good, but the comments should be adjusted -- UserSettings simply is the list of user settings, and then over here in the initial data there's this quirk on the user_settings object.

gnprice · 2022-10-27T00:59:00Z

src/api/eventTypes.js

-  property: $Keys<UserSettings>,
-  value: $Values<UserSettings>,
+  property: $Keys<$NonMaybeType<InitialData['user_settings']>>,
+  value: $Values<$NonMaybeType<InitialData['user_settings']>>,


And on that hypothesis about what's going on with this mismatch, probably these should stay UserSettings -- probably we never get an event updating the two rogue "user settings".

gnprice · 2022-10-27T01:00:44Z

src/api/modelTypes.js

+// ===================================================================
+// Data attached to the current user on the realm.


Cool, seems like a good home for this type.

gnprice · 2022-10-27T01:06:47Z

src/api/users/updateUserSettings.js

+  // These aren't in UserSettings because the doc on POST /register doesn't
+  // mention them. (That's OK: we get the name and email elsewhere, and we
+  // don't need the user's password from the server at all.)


This endpoint isn't about getting these anyway, right? It's about setting them.

gnprice · 2022-10-27T01:17:06Z

src/realm/realmReducer.js

+            if (data.mandatory_topics !== undefined) {
+              result.mandatoryTopics = data.mandatory_topics;
+            }


realm state: Fill in some live-updating gaps; mark another with a TODO

Hmm, good catch.

How did you identify what gaps existed? It'd be good to arrange things so that it's straightforward to verify whether any gaps exist, and so to bring the list up to date.

The first thing I looked at is the list of properties on RealmState. We have the REGISTER_COMPLETE case above kept in sync with that (in the exact same order), which is handy. This list is mostly in sync with it too, but it looks like there are a few gaps beyond the ones filled in this commit: nonActiveUsers, filters, emoji, defaultExternalAccounts, pushNotificationsEnabled, webPublicStreamsEnabled.

(Actually fixing those needn't be in this PR, to the extent they're nontrivial or require some investigation -- just want to understand what you did, and we can add TODO comments.)

I looked at the RealmUpdateDictEvent type to see what that event contains, and that led me to the RealmDataForUpdate type, which goes like this:

add_custom_emoji_policy: InitialDataRealm['realm_add_custom_emoji_policy'], allow_community_topic_editing: // deprecated InitialDataRealm['realm_allow_community_topic_editing'], allow_edit_history: InitialDataRealm['realm_allow_edit_history'], // etc.

I went down that list, and for each item that we were storing in the realm state, I checked if we were also handling the event. For example, for that third item, realm_allow_edit_history appears in realmReducer's REGISTER_COMPLETE handler, and allow_edit_history appears in realmReducer's realm-event op-update handler, so that one is taken care of.

None of realm_non_active_users, realm_filters, realm_emoji, realm_default_external_accounts, realm_push_notifications_enabled, and server_web_public_streams_enabled appear in RealmUpdateDictEvent, so we shouldn't expect them to be updated with this event.

I don't know if we can get updates to nonActiveUsers.

It looks like we get updates to filters with our handling of the realm_filters event (via our Redux action with type EVENT_REALM_FILTERS).

It looks like we get updates to emoji with our handling of the realm_emoji event (via our Redux action with type EVENT_REALM_EMOJI_UPDATE).

There's no event we'd look to for defaultExternalAccounts; see ee74a94 and discussion.

I suspect there's likewise no event for pushNotificationsEnabled but I'm not sure.

I don't think there's an event for webPublicStreamsEnabled. The initial-data property is server_web_public_streams_enabled, and since it's prefixed with server_ I'd imagine it can only be changed with a server restart, and we should handle restarts by reregistering. (Looks like Refetch server data from scratch after server upgrade #4793 is for reregistering on restarts where the server version changed. For this, we'd want to reregister on all restarts).

Very helpful, thanks!

None of realm_non_active_users, […], and server_web_public_streams_enabled appear in RealmUpdateDictEvent, so we shouldn't expect them to be updated with this event.

Looking through just now, I think I find one more: realm_enable_read_receipts. (Unless it should appear in RealmUpdateDictEvent after all? Seems like the sort of thing I'd expect to be there.)

I don't know if we can get updates to nonActiveUsers.

I think the natural way for that to be updated is as a user-related event. Like user/update:
https://zulip.com/api/get-events#realm_user-update
That doesn't mention updates to is_active, though. I wonder if those do happen and it's just undocumented, or if there isn't an event for that happening (i.e., for a user getting deactivated or reactivated), or what. Hmm.

I suspect there's likewise no event for pushNotificationsEnabled but I'm not sure.

I agree. I think realm_push_notifications_enabled is really a server-level setting, which can only change on server restart.

Because the bulk of the RealmState properties -- or specifically, those that we've grouped together as coming from InitialDataRealm -- are found in RealmDataForUpdate and are handled in this spot, I think a good way to make this auditable would be to get them all listed here: when they aren't handled here, to have a line or two of comments saying why not, e.g. where they're handled instead.

So let's remember to do that as a followup.

Looking through just now, I think I find one more: realm_enable_read_receipts. (Unless it should appear in RealmUpdateDictEvent after all? Seems like the sort of thing I'd expect to be there.)

I do see this in RealmDataForUpdate:

enable_read_receipts: InitialDataRealm['realm_enable_read_receipts'],

and I do see handling in realmReducer for the initial data and for the event. Have I misunderstood something? 🙂

Hmm, yeah.

That one was added relatively recently, in 03f7a98 a few weeks ago (in all the different places it appears.) Perhaps I was somehow looking at an out-of-date version of that file? I remember thinking of that hypothesis and specifically checking for it, though. 🤷 Anyway, that one looks good.

gnprice · 2022-10-27T01:25:23Z

(Review ongoing -- not actually sure why GitHub decided to post it.)

gnprice

Thanks @chrisbobbe! Generally this looks great. Comments below and above.

gnprice · 2022-10-27T01:25:48Z

src/lightbox/LightboxHeader.js

-        <UserAvatarWithPresence size={36} avatarUrl={avatarUrl} email={senderEmail} />
+        <UserAvatarWithPresence size={36} userId={senderId} />


gnprice · 2022-10-27T01:52:10Z

src/common/UserAvatarWithPresence.js

+    // Callers don't make sure that the user is one we actually have data
+    // for: e.g., we're helping identify a PM narrow by showing one of the
+    // PM recipients, but that recipient's user record doesn't exist because
+    // it was deleted after the conversation started.


Hmm. Have you observed something like this actually happen? If so I think that's a bug.

In general, we assume that the data we have is coherent and not corrupted or truncated -- so if there's a user ID somewhere, there's a user record for that user ID. For example if a user wants their data deleted, that should result in the data in the user record that's about them (the human) getting scribbled over with placeholder data, but the server should still have a record in the user table at their former user ID and should still provide to clients a user record with that ID.

It's still reasonable to return null here rather than crash, just out of caution in case this can indeed happen. But on that theory that it's a bug if it does happen, we should be logging an error (and shouldn't worry much about presenting a nice UI if it does happen.)

Huh, interesting. No, I haven't seen something like that happen, but I didn't know that we expect user records to stay around forever. I guess that's true of bots too, not just humans?

I plan to remove this comment change from this PR, and come back to it in #5508. I think we'll get some noise in the logs if we add logging before #5508.

gnprice · 2022-10-27T01:57:06Z

src/common/UserAvatarWithPresence.js

  size: number,
  onPress?: () => void,
-  email: string,
-  isMuted?: boolean,


This actually fixes a few spots where we were failing to show an avatar as muted: - The lightbox header, where it shows the message sender - The ChatScreen header for a 1:1 PM - The list of selected users in the user picker (UserPickerCard)

Oof. Yeah, this prop never should have been optional, oops -- that's a wide-open invitation for exactly this bug.

(Or potentially it could be optional temporarily in the course of adding the feature. But only with a plan to finish supplying this prop at each of this component's use sites as part of finishing the implementation of the feature.)

gnprice · 2022-10-27T02:06:42Z

src/utils/presence.js

+  const userPresence = (presence[user.email]: UserPresence | void);
+  const userStatus = getUserStatus(state, userId);
+  if (!userPresence || !userPresence.aggregated) {
+    return null;
+  }


nit: swap order so the userPresence checks and early return are immediately after defining userPresence

(The old code didn't do this because it was a series of useSelector calls, and so the control flow had to be a bit contorted to put the conditional after all of those. But here, we can.)

gnprice · 2022-10-27T02:18:11Z

src/utils/__tests__/presence-test.js

      presenceToHumanTime(
        { aggregated: { client: 'website', status: 'active', timestamp: currentTimestamp - 100 } },
        { away: true, status_text: null, status_emoji: null },
+        eg.recentZulipFeatureLevel,


test('if less than a day, and the user is "away" the user is ...', () => {

This test is all about exercising the case that no longer applies to new servers -- so it should use an appropriate old feature level, rather than eg.recentZulipFeatureLevel.

In this revision, when in the future someone bumps the definition of eg.recentZulipFeatureLevel, this test would break.

As long as eg.recentZulipFeatureLevel is coming to our attention, that's probably a good prompt to bump it to the latest. Can do that in a tiny prep commit -- it looks like nothing breaks, happily.

Meanwhile, should add a case that does use a new feature level (probably cleanest is eg.recentZulipFeatureLevel, after bumping it) and tests that the old logic here no longer applies.

chrisbobbe · 2022-11-01T02:28:55Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Looks great -- remaining comments are all only about comments.

That includes one open subthread above, #5531 (comment) . That one also doesn't need to be within this PR; can be a followup.

gnprice · 2022-11-01T23:14:15Z

src/utils/__tests__/presence-test.js

+  // TODO(server-6.0): Remove; this is the natural behavior and won't need
+  //   testing when the 148 condition is gone.


Hmm, "this won't need testing, it's the natural behavior" sounds like famous last words 😉 I'd be hesitant to drop a test case on those grounds.

I think this test will end up being removable, but the reason is: when we delete that 148 condition, it will also let us stop passing a UserStatus to this function at all. When we do that, this test case will be testing the exact same input as one of the cases above.

Hmm, that fact we can stop passing a UserStatus is probably worth a reminder in that other TODO-server comment, though.

gnprice · 2022-11-01T23:15:18Z

src/utils/presence.js

+  // TODO(server-6.0): Remove this `if` block.
+  if (zulipFeatureLevel < 148 && status.away && differenceInDays(Date.now(), lastTimeActive) < 1) {


Suggested change

// TODO(server-6.0): Remove this `if` block.

if (zulipFeatureLevel < 148 && status.away && differenceInDays(Date.now(), lastTimeActive) < 1) {

// TODO(server-6.0): Remove this `if` block and the `status` parameter.

if (zulipFeatureLevel < 148 && status.away && differenceInDays(Date.now(), lastTimeActive) < 1) {

gnprice · 2022-11-01T23:25:00Z

src/account-info/ProfileScreen.js

      <ScrollView>
        <AccountDetails user={ownUser} showEmail={false} />
-        <AwayStatusSwitch />
+        {zulipFeatureLevel >= 148 ? (


Should get a TODO-server comment.

To prepare this type to be reused for a new PATCH /settings binding, coming next. This makes one functional change to the types: since `available_notification_sounds` and `emojiset_choices` no longer appear in `UserSettings`, the `UserSettingsUpdateEvent` type gets narrower by excluding those too. That's correct; we don't expect an event that updates those two rogue "user settings".

By unexporting the old UserAvatarWithPresence and giving its name to UserAvatarWithPresenceById.

Instead, grab that information from Redux inside this component. This actually fixes a few spots where we were failing to show an avatar as muted: - The lightbox header, where it shows the message sender - The ChatScreen header for a 1:1 PM - The list of selected users in the user picker (UserPickerCard)

These are from CZO, accessed just now.

chrisbobbe · 2022-11-03T22:37:29Z

Thanks for the review! Revision pushed, and I've added this followup to my list: #5531 (comment)

Fixes: zulip#5445

gnprice · 2022-11-03T23:21:47Z

Thanks! Looks good; merging, with one edit to the "famous last words" comment from above:
#5531 (comment)
so that it instead specifies the condition that really does mean we can delete the test case:

-  // TODO(server-6.0): Remove; this is the natural behavior and won't need
-  //   testing when the 148 condition is gone.
+  // TODO(server-6.0): Remove, once this test case is redundant with those
+  //   above after the status parameter is gone.

chrisbobbe · 2022-11-03T23:33:55Z

Thanks!

chrisbobbe added 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. server release goal Things we should try to coordinate with a major Zulip Server release. labels Oct 26, 2022

chrisbobbe requested a review from gnprice October 26, 2022 13:03

chrisbobbe mentioned this pull request Oct 26, 2022

api: Add updateUserSettings binding, for PATCH /settings #5530

Closed

gnprice reviewed Oct 27, 2022

View reviewed changes

gnprice mentioned this pull request Oct 28, 2022

UserItem: Remove UserItemRaw hack #5508

Merged

chrisbobbe force-pushed the pr-invisible-mode branch from 1fbb25c to 6b54f29 Compare November 1, 2022 02:28

chrisbobbe force-pushed the pr-invisible-mode branch from 6b54f29 to 0eecac3 Compare November 1, 2022 02:34

gnprice reviewed Nov 1, 2022

View reviewed changes

chrisbobbe force-pushed the pr-invisible-mode branch from 0eecac3 to a285a52 Compare November 3, 2022 22:31

chrisbobbe added 14 commits November 3, 2022 15:32

initial data types: Sync InitialDataUserSettings with the doc at FL 152

89397f0

api types [nfc]: Move UserSettings to modelTypes.js

f551a53

api: Add updateUserSettings binding, for PATCH /settings

63861dc

realm state: Fill in some live-updating gaps; mark another with a TODO

b8b74e6

realm state: Store presenceEnabled

662b122

UserAvatarWithPresence: Convert all callers to use …ById instead

b31750c

UserAvatarWithPresence [nfc]: Finish migration to UserId-based interface

5b04f58

By unexporting the old UserAvatarWithPresence and giving its name to UserAvatarWithPresenceById.

PresenceStatusIndicator: Take a user ID instead of email

1cd86f8

PresenceStatusIndicator [nfc]: Call getUserStatus with userId from props

41f347c

presence [nfc]: Make a Redux selector for getting presence status

1a0d9c0

ProfileScreen [nfc]: Inline AwayStatusSwitch

f8a638c

exampleData: Update recentZulip{Version,FeatureLevel}

7d19edf

These are from CZO, accessed just now.

chrisbobbe force-pushed the pr-invisible-mode branch from a285a52 to 69c330f Compare November 3, 2022 22:37

presence: Replace "unavailable" status with "invisible mode"

71fe5fa

Fixes: zulip#5445

gnprice force-pushed the pr-invisible-mode branch from 69c330f to 71fe5fa Compare November 3, 2022 23:21

gnprice merged commit 71fe5fa into zulip:main Nov 3, 2022

chrisbobbe deleted the pr-invisible-mode branch November 10, 2022 23:47

chrisbobbe mentioned this pull request Jun 13, 2025

Support toggling "Invisible mode" zulip/zulip-flutter#1578

Closed

		// ===================================================================
		// Data attached to the current user on the realm.

		<UserAvatarWithPresence size={36} avatarUrl={avatarUrl} email={senderEmail} />
		<UserAvatarWithPresence size={36} userId={senderId} />

		// TODO(server-6.0): Remove; this is the natural behavior and won't need
		// testing when the 148 condition is gone.

		// TODO(server-6.0): Remove this `if` block.
		if (zulipFeatureLevel < 148 && status.away && differenceInDays(Date.now(), lastTimeActive) < 1) {

Uh oh!

presence: Replace "unavailable" status with "invisible mode" #5531

presence: Replace "unavailable" status with "invisible mode" #5531

Uh oh!

Conversation

chrisbobbe commented Oct 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Oct 27, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 1, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 3, 2022

Uh oh!

gnprice commented Nov 3, 2022

Uh oh!

chrisbobbe commented Nov 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

gnprice Oct 27, 2022 •

edited

Loading

chrisbobbe Oct 28, 2022 •

edited

Loading

chrisbobbe Nov 3, 2022 •

edited

Loading