Display presence status taking away status into account #3326

borisyankov · 2019-02-04T22:45:54Z

Update all presence UI to correctly, taking into account away status.

gnprice · 2019-02-06T06:51:51Z

#3263 is now merged, which I believe unblocks this one!

borisyankov · 2019-02-07T13:57:27Z

This is ready for review.

gnprice

Thanks @borisyankov !

This looks generally good. I quite like the simplification sweeping through in the second commit!

Specific comments below.

gnprice · 2019-02-07T20:07:44Z

src/users/userSelectors.js

+  users.reduce((usersById, user) => {
+    usersById[user.email] = user;
+    return usersById;
+  }, ({ ...undefined }: UserEmailMap)),


Huh interesting -- why this instead of {}?

This fixes: "Cannot cast object literal to UserEmailMap because inexact object literal [1] is incompatible with exact UserEmailMap [2]."

Because {} is considered 'unsealed object'
https://flow.org/en/docs/types/objects/#toc-unsealed-objects

A concept in Flow that might be removed as not very intuitive.

Huh fascinating!

Ah, I see you've made UserEmailMap exact. I've shied away from making many types with "indexer properties" exact, because I keep getting strange errors from them. This may be a very useful workaround for that problem!

Let's be sure to mention that either in a comment and/or the commit message. Also you mentioned in audio chat that you found this workaround in an issue thread on Flow. That link will be super useful to have written down.

bump:

Also you mentioned in audio chat that you found this workaround in an issue thread on Flow. That link will be super useful to have written down.

The exact issue I found was:
facebook/flow#7343
This does not have any extra information that is worth linking to, besides the fact it named the problem (unsealed objects) which made me read the docs.

There are other GitHub issues that discuss sealed/unsealed objects, like:
facebook/flow#2977

I don't think linking them directly is of any specific benefit since the main benefit is identifying the cause and knowing what term to search for.

gnprice · 2019-02-07T20:39:57Z

src/users/userSelectors.js

@@ -41,6 +41,13 @@ export const getAllUsersByEmail = createSelector(getAllUsers, allUsers =>
  }, {}),
 );

+export const getUsersByEmail: Selector<UserEmailMap> = createSelector(getUsers, (users = []) =>
+  users.reduce((usersById, user) => {


I think I've mentioned before that I'm not a fan of this pattern where reduce is used but the callback actually just mutates the object -- I think they're almost always clearer as a straightforward loop. Because it's just like the code around it, though, I won't press that here; maybe I'll follow up separately with a sweep through to change a lot of these.

gnprice · 2019-02-07T20:40:40Z

src/users/userSelectors.js

@@ -41,6 +41,13 @@ export const getAllUsersByEmail = createSelector(getAllUsers, allUsers =>
  }, {}),
 );

+export const getUsersByEmail: Selector<UserEmailMap> = createSelector(getUsers, (users = []) =>
+  users.reduce((usersById, user) => {
+    usersById[user.email] = user;


This name definitely needs to be made accurate, though 😉

gnprice · 2019-02-07T20:54:08Z

src/users/userSelectors.js

@@ -41,6 +41,13 @@ export const getAllUsersByEmail = createSelector(getAllUsers, allUsers =>
  }, {}),
 );

+export const getUsersByEmail: Selector<UserEmailMap> = createSelector(getUsers, (users = []) =>


The commit message here says:

We should not use the existing `getAllUsersByEmail` since it returns all users and realm bots and messes up our strict typing.

Two questions:

We should not use that selector where? From context I think you probably mean in the upcoming change to PresenceStatusIndicator; that's fine but should be explicit, like "In an upcoming commit, we'll want to use this instead of the existing getAllUsersByEmail because the latter [...]".

Remember the context of the next few commits isn't always handy or obvious, when this is found in the history in the future.

Without that, it sounds like you're saying we should never use getAllUsersByEmail. Maybe that is what you're saying? I'm not actually 100% sure.

Why not use getAllUsersByEmail in that upcoming commit? This new one amounts to a filtered view of that one, with a subset of the entries. Sure, it seems plausible that the difference will never matter... but if it ever does (we're showing presence status for a "user" that doesn't show up in getUsers), it seems like we want to go ahead and use the data. And that's the simpler thing, too.

More broadly, I definitely agree that these should be made clearer. I think the most important bit will be to work out the differences in what they actually mean... and write that down as a sentence or two each, in jsdoc. I'm not entirely sure what the contrasts are myself. Then we can find names that reflect those contrasts.

There are some clues in the commit messages in this file's history -- take a look at git log --stat -p src/users/userSelectors.js.

gnprice · 2019-02-07T20:57:16Z

src/common/PresenceStatusIndicator.js

@@ -2,9 +2,12 @@
 /* eslint-disable react-native/no-unused-styles */
 import React, { PureComponent } from 'react';
 import { StyleSheet, View } from 'react-native';
+import { connect } from 'react-redux';


This refactoring makes sense to me! Thanks also for the clear commit messages explaining why ❤️

gnprice · 2019-02-07T20:58:36Z

src/common/PresenceStatusIndicator.js

@@ -25,9 +28,16 @@ const styles = StyleSheet.create({
  },
 });

+type PropsFromState = {|


Ah, interesting idea to split this out! I can think of some other components that would likely benefit from a similar change.

I'm not keen on using the word "state" here -- the thing it first makes me think of in this context (juxtaposed so closely with Props) is the React component's state, which is not at all what you mean. How about PropsFromRedux?

Or perhaps PropsFromConnect?

I was playing with ConnectedProps but sounded too cutesy and not clear.
PropsFromConnect sounds good. Did rename.

Makes sense!

I think I've sometimes said "connected props" and spelled it "connected props"... but without the punctuation it does become unclear.

gnprice · 2019-02-07T21:00:04Z

src/common/PresenceStatusIndicator.js

@@ -38,18 +48,28 @@ type Props = {|
 * * gray if 'offline'
 *
 * @prop [style] - Style object for additional customization.
- * @prop [presence] - UserPresence object used to determine the status from.
+ * @prop [email] - email of the user whose status we are showing.


This is no longer optional, right?

Right. Fixed.

gnprice · 2019-02-07T21:07:17Z

src/common/PresenceStatusIndicator.js

+
+    const awayStatus = userStatus[user.user_id];
+
+    const status = awayStatus && awayStatus.away ? 'offline' : statusFromPresence(userPresence);


Hmm. I think I'd rather this logic be encapsulated in its own function or method somewhere.

A minimal change would be to put it in a method on the component -- so like

render() { const status = this.getStatus(); if (status === null) { return null; } const { style, hideIfOffline } = this.props; // if (hideIfOffline ... etc., etc.

and that would I think help a lot in separating the logic for the sake of reading and understanding this code.

Ultimately I'd want the substance of that method, especially the awayStatus.away ? 'offline' : statusFromPresence(...) part, to be in a selector or similar function somewhere, outside of display code. But I'm OK merging without that.

src/users/UserList.js

borisyankov · 2019-02-08T17:20:39Z

Did the discussed changes. Ready for a merge?
A rename of the other selector is here #3326

gnprice

Thanks @borisyankov for these updates!

Several of my comments seem to still be open, mainly on the first commit adding getUsersByEmail.

I think I can merge the refactor, though -- that will take a lot of the complexity out of the branch.

gnprice · 2019-02-08T23:13:57Z

src/users/userSelectors.js

@@ -41,6 +41,13 @@ export const getAllUsersByEmail = createSelector(getAllUsers, allUsers =>
  }, {}),
 );

+export const getUsersByEmail: Selector<UserEmailMap> = createSelector(getUsers, (users = []) =>


gnprice · 2019-02-08T23:14:05Z

src/users/userSelectors.js

@@ -41,6 +41,13 @@ export const getAllUsersByEmail = createSelector(getAllUsers, allUsers =>
  }, {}),
 );

+export const getUsersByEmail: Selector<UserEmailMap> = createSelector(getUsers, (users = []) =>
+  users.reduce((usersById, user) => {
+    usersById[user.email] = user;


gnprice · 2019-02-08T23:14:26Z

src/users/userSelectors.js

+  users.reduce((usersById, user) => {
+    usersById[user.email] = user;
+    return usersById;
+  }, ({ ...undefined }: UserEmailMap)),


bump:

Also you mentioned in audio chat that you found this workaround in an issue thread on Flow. That link will be super useful to have written down.

gnprice · 2019-02-08T23:24:52Z

src/common/PresenceStatusIndicator.js

      return null;
    }

-    const status = statusFromPresence(presence);
+    const status = statusFromPresenceAndUserStatus(userPresence, userStatus[user.user_id]);


Cool, this nicely encapsulates the business logic (such an odd term here; "product logic"?) at work here.

A further improvement would be to also stuff the next few lines above it, with user = usersByEmail[email] etc., inside. I think the ideal interface for that is probably to put this all inside a selector... e.g. the connect call could say

presenceStatus: getPresenceStatusForEmail(state, props.email),

That's a pattern we don't currently use much, if at all, but there are a lot of places in our code that would benefit. Detailed discussion at #3015 .

I'm fine merging with this interface as it is, though.

gnprice · 2019-02-08T23:30:25Z

Pushed the refactor and also the helper function, as 01b33ad..7faa1fe .

Now `connect`s to the `userStatus` state and if `away` for the given user exists and is set to `true`, we override the calculated value and make it `offline` instead. It is unfortunate that there is a (temporary) discrepancy between our `presence` and `userStatus` key used for mapping (`email` vs `user_id`) thus needing the `usersByEmail` state too. We should be moving `presence` to 'id to presence mapping' and then we would replace the `email` prop with `userId` prop. Later.

borisyankov · 2019-02-12T20:53:18Z

Rebased and now using the Map.

gnprice · 2019-02-12T22:51:35Z

Merged -- thanks again!

I'd still be glad to see the followup I mentioned in this comment:
#3326 (comment)

A further improvement would be to also stuff the next few lines above it, with user = usersByEmail[email] etc., inside.

gnprice added the review label Feb 4, 2019

borisyankov added P0 critical Highest priority blocked on other work To come back to after another related PR, or some other task. labels Feb 5, 2019

gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Feb 6, 2019

borisyankov force-pushed the presence-avatar branch 7 times, most recently from ac0ae35 to fe6d841 Compare February 7, 2019 13:56

borisyankov changed the title ~~WIP: Display presence status taking away status into account~~ Display presence status taking away status into account Feb 7, 2019

gnprice reviewed Feb 7, 2019

View reviewed changes

borisyankov force-pushed the presence-avatar branch 3 times, most recently from cabd599 to c76f7d2 Compare February 8, 2019 17:12

borisyankov force-pushed the presence-avatar branch 2 times, most recently from 7ef7abe to 9734141 Compare February 8, 2019 21:01

gnprice reviewed Feb 8, 2019

View reviewed changes

borisyankov force-pushed the presence-avatar branch from 9734141 to 6037f7c Compare February 11, 2019 19:19

gnprice mentioned this pull request Feb 11, 2019

WIP: Rename 'getAllUsersByEmail' to 'getUsersAndBotsByEmail' and type it #3332

Closed

borisyankov force-pushed the presence-avatar branch from 6037f7c to db719ac Compare February 12, 2019 20:37

gnprice merged commit db719ac into zulip:master Feb 12, 2019

gnprice removed the review label Feb 12, 2019

gnprice added the a-presence/status Presence, and "away status" label Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display presence status taking away status into account #3326

Display presence status taking away status into account #3326

borisyankov commented Feb 4, 2019 •

edited

Loading

gnprice commented Feb 6, 2019

borisyankov commented Feb 7, 2019

gnprice left a comment

gnprice Feb 7, 2019

borisyankov Feb 7, 2019

gnprice Feb 7, 2019

gnprice Feb 8, 2019

borisyankov Feb 11, 2019

borisyankov Feb 11, 2019

gnprice Feb 7, 2019

gnprice Feb 7, 2019

gnprice Feb 8, 2019

gnprice Feb 8, 2019

borisyankov Feb 11, 2019

gnprice Feb 7, 2019

gnprice Feb 8, 2019

gnprice Feb 7, 2019

gnprice Feb 7, 2019

gnprice Feb 7, 2019

borisyankov Feb 7, 2019

gnprice Feb 8, 2019

gnprice Feb 7, 2019

borisyankov Feb 7, 2019

gnprice Feb 7, 2019

borisyankov commented Feb 8, 2019

gnprice left a comment

gnprice Feb 8, 2019

gnprice Feb 8, 2019

gnprice Feb 8, 2019

gnprice Feb 8, 2019

gnprice commented Feb 8, 2019

borisyankov commented Feb 12, 2019

gnprice commented Feb 12, 2019


		const awayStatus = userStatus[user.user_id];

		const status = awayStatus && awayStatus.away ? 'offline' : statusFromPresence(userPresence);

Display presence status taking away status into account #3326

Display presence status taking away status into account #3326

Conversation

borisyankov commented Feb 4, 2019 • edited Loading

gnprice commented Feb 6, 2019

borisyankov commented Feb 7, 2019

gnprice left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borisyankov commented Feb 8, 2019

gnprice left a 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

gnprice commented Feb 8, 2019

borisyankov commented Feb 12, 2019

gnprice commented Feb 12, 2019

borisyankov commented Feb 4, 2019 •

edited

Loading