Migrate navigateToAccountDetails and getUserIsActive to user IDs #3968

agrawal-d · 2020-03-12T13:54:09Z

This is a rough initial PR. Feedback is appreciated. This was done to make pull #3878 mergeable, as per #3878 (comment).

This PR will partially address #3764.

agrawal-d · 2020-03-12T14:29:24Z

@ray-kraesig @gnprice, please review.

rk-for-zulip

A few things, but looks good.

(The second commit's message and summary use JavaScript-style "Id" rather than English-style "ID" in a couple of places.)

src/account-info/AccountDetailsScreen.js

rk-for-zulip · 2020-03-16T20:09:09Z

src/title-buttons/InfoNavButtonPrivate.js

+    const user = getAllUsersByEmail(state).get(props.narrow[0].operand);
+    if (user === undefined) {
+      return -1;
+    }
+    return user.user_id;


This could be shortened to getAllUsersByEmail(state).get(props.narrow[0].operand)?.user_id ?? -1.

Preferably, though, this should be number | void rather than using a sentinel value. (Perhaps AccountDetailsScreen's prop should even be number | void? We've been quietly passing bad email-strings to it on failure for so long... 😞)

I've used this in the latest push, but do not understand the syntax. Can you please explain?
?? looks like the nullish coalescing operator, and ? looks like the ternary operator.
But then,

Ternary operator requires three operands. And here only 2 have been passed.

What is the object that ?? is checking for null ?

I also do not understand ?. - what does it do?

Why is this getAllUsersByEmail(state).get(props.narrow[0].operand)?.user_id :undefined) incorrect syntax?

?. is not part of the ternary operator; it's the optional chaining operator.

(EDIT: It's a bit difficult to Google, but surprisingly, not impossible.)

Thanks. I had tried '?.' in quotes, but didn't get any (immediately) useful results.

src/webview/html/messageAsHtml.js

rk-for-zulip · 2020-03-16T20:37:53Z

src/webview/webViewEventHandlers.js

@@ -200,7 +200,7 @@ export const handleMessageListEvent = (props: Props, _: GetText, event: MessageL
      break;

    case 'avatar':
-      props.dispatch(navigateToAccountDetails(event.fromEmail));
+      props.dispatch(navigateToAccountDetails(parseInt(event.fromUserId, 10)));


This leaks NaN. (And should probably log an error on parse-failure, including event.fromUserId as data in extras.)

src/webview/html/messageTypingAsHtml.js

agrawal-d · 2020-03-19T16:45:36Z

@ray-kraesig I've pushed some changes. Please re-review.

rk-for-zulip · 2020-03-19T17:26:52Z

src/account-info/AccountDetailsScreen.js

+  componentDidMount() {
+    const { user } = this.props;
+    if (user === undefined) {
+      logging.error('User object passed is undefined.');


Unfortunately, at some point in the last few releases we lost the ability to reconstruct the source file/line information from these calls. 😞 This will need to identify the error site: e.g., 'AccountDetailsScreen created with invalid user', or something similar.

Unfortunately, at some point in the last few releases we lost the ability to reconstruct the source file/line information from these calls. 😞

Hmm! It looks like we don't have an issue for this -- @ray-kraesig , would you file one with whatever information you have? It'd be good to fix, and that will help us at least track what we know about it.

Ah, perhaps you're thinking of #3809? Just happened across that thread by chance.

rk-for-zulip · 2020-03-19T17:32:29Z

src/webview/webViewEventHandlers.js

+      try {
+        userId = parseInt(event.fromUserId, 10);
+      } catch (e) {


NaNs don't raise exceptions. A try-catch will have no effect.

(See parseInt and NaN for more information.)

src/webview/webViewEventHandlers.js

rk-for-zulip · 2020-03-19T18:02:25Z

src/account-info/AccountDetailsScreen.js

+  user: (() => {
+    const { userId } = props.navigation.state.params;
+    return userId === undefined ? userId : getAllUsersById(state).get(userId);
+  })(),
+  isActive: (() => {
+    const { userId } = props.navigation.state.params;
+    if (userId === undefined) {
+      return false;
+    }
+    const user = getAllUsersById(state).get(userId);
+    if (!user) {
+      return false;
+    }
+    return getUserIsActive(state, user.email);
+  })(),


For future reference – isActive effectively needs the value of user, so you might as well make that explicit (and also avoid recomputing it). For example:

... connect((state, props) => { const { userId } = props.navigation.state.params; const user: number | void = ... const isActive: boolean = ... return { user, isActive }; }))(...

(I say "for future reference" since I'm aware most of this goes away in the next commit. 🙃 No need to take action here.)

rk-for-zulip · 2020-03-19T18:34:58Z

src/account-info/AccountDetailsScreen.js

+  isActive:
+    props.navigation.state.params.userId === undefined
+      ? false
+      : getUserIsActive(state, props.navigation.state.params.userId),


This is essentially the same computational structure as for user, above; the two should probably be structured similarly. (And, if it's simple enough to do so, partially coalesced.)

gnprice

Thanks @agrawal-d, and thanks @ray-kraesig for the reviews!

Comments below.

gnprice · 2020-03-20T03:15:27Z

src/users/userSelectors.js

+export const getUserIsActive = (state: GlobalState, userId: number): boolean =>
+  [...getUsers(state), ...getCrossRealmBots(state)].some(user => user.user_id === userId);


This creates a case of #3339. :-)

Instead, this should use a cached selector getActiveUsersById, in order to make a nice Map to efficiently look this up in. Adding that can be its own commit coming before this one.

See also this previous chat discussion:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Migrate.20from.20emails.20to.20userIds/near/829640

The new selector can go right next to getActiveUsersByEmail. See also getAllUsersById and getAllUsersByEmail for other examples to compare to.

gnprice · 2020-03-20T03:27:01Z

static/translations/messages_en.json

+  "Profile not found": "Profile not found",
+  "The profile for this user could not be found.": "The profile for this user could not be found.",


This definitely doesn't belong in an NFC commit. :-)

If the error case truly can't happen, then there's no need for a user-facing error message to translate. If it can happen, then this new form of error-handling for it is a user-visible change and therefore definitely not "no functional change".

In general I'm concerned that there's kind of a lot of complexity happening in this commit. Some of it, I think needn't be added at all; other parts may be best split into multiple commits. The change we're ultimately making here is of a kind that should be possible to do in a series where each commit is quite simple and straightforward.

gnprice · 2020-03-20T03:32:39Z

src/account-info/AccountDetailsScreen.js

@@ -71,6 +95,12 @@ class AccountDetailsScreen extends PureComponent<Props> {
 }

 export default connect<SelectorProps, _, _>((state, props) => ({
-  user: getAccountDetailsUserForEmail(state, props.navigation.state.params.email),


So in the chat discussion we had about this, I recommended:

Then I'd also replace the use of getAccountDetailsUserForEmail (in AccountDetailsScreen) with getAllUsersByEmail, as recommended in the former function's jsdoc.

One quick thing that would help simplify this series is: this change should be its own commit. I didn't specifically mention a separate commit, sorry -- but that's what I meant by "Then I'd also ...". In general when I think of some code changes in terms like "do A; then do B; then do C", what I mean is that A and B and C are each separate commits. This is basically part of how we write clear and coherent commits 🙂

Okay. The latest push creates some new selectors which are used here - these selectors are in their own, separate commits.

Cool. The change that actually replaces this use of getAccountDetailsUserForEmail should still be its own commit. 🙂

To see why, take a look at the implementation of that function -- there are some funny wrinkles in its behavior, involving the NULL_USER object. Replacing it will change the behavior in those edge cases, if those edge cases are possible. So I want that change as its own small commit, in order to keep a small circle drawn around the task of reading it and thinking through what changed and making sure all loose threads are taken care of.

I think that commit will come before the rest of the big "migrate from email to user ID" change. Probably it should use getUserForEmail.

Yeah, this makes sense - a migration commit should be a NFC. If it's not, then it's not strictly a migration.

Yeah, I'd say that at least for the term "refactor" -- a change that isn't NFC isn't purely a refactor, and "pure refactor" is a close synonym of "NFC".

I'd actually use "migration" somewhat more broadly: e.g. I might talk about "migrating" to a new major version of some dependency (especially a big dependency, like RN itself), and that's typically not NFC. But in this case, the kind of migration we're going for is pretty much a pure refactor.

... Oh, actually, here's another example of "migrate" -- for the commit that replaces the use of getAccountDetailsUserForEmail with getUserForEmail, I would actually describe that as migrating from getAccountDetailsUserForEmail to getUserForEmail. (I noticed this example as I found myself typing exactly that in the comment below.) Even though, in edge cases, it may not behave quite the same, and so it isn't NFC.

So, I think this sequence of code changes is now good!

There's some work I have to do to get to that conclusion, in the case of the change that migrates from getAccountDetailsUserForEmail to getUserForEmail: ask the questions, "what are the cases where this can behave differently? what's the old behavior in those cases, and what's the new behavior? could the new behavior be worse?", and study the code to answer them.

So the commit message should reflect that work. 🙂 I'll edit it to add that.

gnprice · 2020-03-20T03:39:17Z

src/account-info/AccountDetailsScreen.js

+  componentDidMount() {
+    const { user } = this.props;
+    if (user === undefined) {
+      logging.error('User object passed is undefined.');


Ah, perhaps you're thinking of #3809? Just happened across that thread by chance.

gnprice · 2020-03-20T04:13:06Z

src/account-info/AccountDetailsScreen.js

 class AccountDetailsScreen extends PureComponent<Props> {
-  handleChatPress = () => {
-    const { user, dispatch } = this.props;
-    dispatch(doNarrow(privateNarrow(user.email)));
+  componentDidMount() {
+    const { user } = this.props;
+    if (user === undefined) {
+      logging.error('User object passed is undefined.');
+    }
+  }
+
+  handleChatPress = (email: string) => {
+    const { dispatch } = this.props;
+    dispatch(doNarrow(privateNarrow(email)));
  };

  render() {
    const { user, isActive } = this.props;
+
+    if (user === undefined) {
+      return (
+        <Screen title="Profile not found" style={styles.userNotFoundScreen}>
+          <Label text="The profile for this user could not be found." />
+        </Screen>
+      );
+    }
+


So, one thing that's contributing quite a bit of the complexity of the code in this PR is the way it's handling the possibility of being asked to show a user that doesn't actually exist. Specifically, there's about four different spots in this component's code that deal with props.user potentially being undefined instead of a real User object, in addition to the places here and elsewhere dealing with potentially even the userId navigation parameter being undefined.

When lots of different bits of code all try to respond to the same error condition, it makes it hard to understand what the code is doing either in that error case or in the normal case, and hard to edit or refactor the code. So let's try to make sure this case gets taken care of once, as early as possible, and then all the code downstream of that can just deal with a real User object.

This principle is basically a particular form of the "crunchy shell, soft center" principle.

In particular this means props.user should go on being just UserOrBot, not UserOrBot | void, so that all the code inside the component just deals with a real, valid user object.

Specifically, what I'd like to do right now in this code is:

If userId (or email, if applicable in the earlier parts of the branch) doesn't actually find us a user object in our data structures, just throw an error.

See getStreamForId for an example of how to do this conveniently. We can add a similar selector getUserForId.

Similarly, over in InfoNavButtonPrivate, if the given user doesn't actually exist in our data then just throw an error.

A selector getUserForEmail, on the same lines as above, may be helpful for this.

Throwing an error is appropriate because I believe it's the case that if we actually have an invalid user ID or email here, that's a bug in the app.

If it later turns out that there is a legitimate case where that can happen -- or if such a bug does exist, and we don't simply debug and fix it, and it's happening with some frequency -- then we can add a bit more complexity to give a nice user-facing error like this "user not found" screen. If we do that, the key principle above will still be an important one: only one spot of code should have to handle the error case, letting everything else deal only with real, valid data.

One likely way to do that would be to have a little wrapper component, which shows either the "not found" screen or the actual account-details screen. The wrapper's user prop would be potentially undefined, but the inner component's user prop would not be.

Thanks for the detailed explanations! I too felt the changes were getting messy.

agrawal-d · 2020-03-23T17:26:49Z

Thanks for the review!
@ray-kraesig @gnprice I've pushed some changes. Please review.

gnprice

Thanks for the revision! This is getting close.

A few more comments below. With those changes, I think the main "migrate from email to user ID" commit will be a very clean NFC change -- it'll still touch about 12 different files, but all in nice boring regular ways.

gnprice · 2020-03-23T23:57:38Z

src/account-info/AccountDetailsScreen.js

@@ -71,6 +95,12 @@ class AccountDetailsScreen extends PureComponent<Props> {
 }

 export default connect<SelectorProps, _, _>((state, props) => ({
-  user: getAccountDetailsUserForEmail(state, props.navigation.state.params.email),


Cool. The change that actually replaces this use of getAccountDetailsUserForEmail should still be its own commit. 🙂

To see why, take a look at the implementation of that function -- there are some funny wrinkles in its behavior, involving the NULL_USER object. Replacing it will change the behavior in those edge cases, if those edge cases are possible. So I want that change as its own small commit, in order to keep a small circle drawn around the task of reading it and thinking through what changed and making sure all loose threads are taken care of.

I think that commit will come before the rest of the big "migrate from email to user ID" change. Probably it should use getUserForEmail.

gnprice · 2020-03-23T23:58:17Z

src/title-buttons/InfoNavButtonPrivate.js

+|}>;
 type Props = $ReadOnly<{|


nit: blank line

I use an extension called 'Git Lens' that shows the Git blame. Here, it showed the authors between line 13 and 14, so it appeared as if I had inserted a new line. :/

gnprice · 2020-03-24T00:01:37Z

src/webview/html/messageAsHtml.js

@@ -116,7 +116,8 @@ $!${divOpenHtml}
 `;
  }

-  const { sender_full_name, sender_email } = message;
+  const { sender_full_name } = message;
+  const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id;


Huh, is there no sender_id in Outbox?

... Huh, indeed there isn't. Well, that's our own code's fault. 😝 We should fix that.

This code is a good workaround for this PR's purposes, because fixing that omission is made complex by the fact we store outbox messages between runs. I'll file an issue for it.

gnprice · 2020-03-24T00:17:05Z

src/webview/webViewEventHandlers.js

+      const userId = parseInt(event.fromUserId, 10);
+      if (Number.isNaN(userId)) {
+        logging.error("WebViewEventHandlers: Failed to parse 'fromUserId' string to integer", {
+          fromUserId: event.fromUserId,
+        });
+        showToast(_('The profile for this user could not be found.'));
+      } else {


If this error condition ever happens, it's a bug purely internal to our code, and it's not one that even relies on any subtle invariants of our big data structures -- there's a handful of steps from our templates, via the click handler in js.js, to here, and this bug would mean one of those somehow fumbled the data.

The usual way we handle that kind of potential bug is by just throwing an exception. I think that's the most appropriate thing to do here, too. As you can see, trying to do this style of more-verbose error handling makes for quite a bit more code to read 😉

(This style is exactly the right kind of approach when parsing data we get from the network! But here we're inside the "soft center" of the app, and so we should take advantage of that.)

agrawal-d · 2020-03-24T09:45:06Z

@gnprice thanks for the review.

I've pushed some changes. I've also reworded the big commit 'migrate from email to user ID' as NFC again, I hope that it qualifies now.

Please re-review.

gnprice

Thanks @agrawal-d for the revisions!

This looks great. A few comments below; I plan to fix those remaining things up and merge.

gnprice · 2020-03-25T22:56:05Z

src/webview/webViewEventHandlers.js

+      } else {
+        props.dispatch(navigateToAccountDetails(userId));
+      }


nit: delete else { and }, to keep the main codepath in a single linear narrative without added indentation

gnprice · 2020-03-25T22:58:41Z

src/webview/webViewEventHandlers.js

+      const userId = parseInt(event.fromUserId, 10);
+      if (Number.isNaN(userId)) {
+        throw new Error(`Failed to parse fromUserId string '${event.fromUserId}' to integer`);


A further small improvement here would be to push this a step further back: if fromUserId is a string that doesn't parse, that's already invalid, so make it fromUserId: number instead, and do the parsing (and throwing) in js.js when this object is constructed.

See also requireAttribute, and dd03939 which added it. Cleanest might be to make a helper named like requireAttributeNumber, which calls requireAttribute and then parseInt, and returns a number or throws.

Will do this in the prep commits of #3878.

gnprice · 2020-03-25T23:04:24Z

src/users/userSelectors.js

+ * In particular, returns the user for:
+ * * i) cross-realm bots
+ * * ii) deactivated users (`is_active` false; see `User` and the linked docs)


I want to word this part a bit differently; I'll do that as an added commit on top.

Two concrete things I notice:

"returns the user for cross-realm bots [and] deactivated users" is kind of confusing; it sounds like other users (the common case) aren't included, or maybe like it returns a user that could be called "the user for" a cross-realm bot.

"See also" is a great technique to provide more context 🙂 See e.g. some mentions in the Flutter style guide (which in general is great, and especially great about docs style), here and here.

I saw a01decc, it's more clear now.

The Flutter guide is useful - thanks! This may be off-topic, but how did you end up reading the Flutter style guide in such depth?

how did you end up reading the Flutter style guide in such depth?

A couple of years ago when I first started doing mobile development, Flutter had just entered beta and looked pretty cool, so I spent some time working with it. One of the things I was impressed by was the quality of the docs, and relatedly of the code and APIs. Here's a past chat thread with a few thoughts on Flutter: thread.

gnprice · 2020-03-25T23:07:45Z

src/account-info/AccountDetailsScreen.js

@@ -31,7 +30,7 @@ type SelectorProps = $ReadOnly<{|
 |}>;

 type Props = $ReadOnly<{|
-  navigation: NavigationScreenProp<{ params: {| email: string |} }>,
+  navigation: NavigationScreenProp<{ params: {| userId: number |} }>,


nit in commit message: rather than navigateToAccountDetails as prefix, I think account details screen or AccountDetailsScreen is closer to the heart of it. The navigateToAccountDetails function is a helper to get to this screen.

gnprice · 2020-03-25T23:15:10Z

src/account-info/AccountDetailsScreen.js

@@ -71,6 +95,12 @@ class AccountDetailsScreen extends PureComponent<Props> {
 }

 export default connect<SelectorProps, _, _>((state, props) => ({
-  user: getAccountDetailsUserForEmail(state, props.navigation.state.params.email),


Yeah, I'd say that at least for the term "refactor" -- a change that isn't NFC isn't purely a refactor, and "pure refactor" is a close synonym of "NFC".

I'd actually use "migration" somewhat more broadly: e.g. I might talk about "migrating" to a new major version of some dependency (especially a big dependency, like RN itself), and that's typically not NFC. But in this case, the kind of migration we're going for is pretty much a pure refactor.

... Oh, actually, here's another example of "migrate" -- for the commit that replaces the use of getAccountDetailsUserForEmail with getUserForEmail, I would actually describe that as migrating from getAccountDetailsUserForEmail to getUserForEmail. (I noticed this example as I found myself typing exactly that in the comment below.) Even though, in edge cases, it may not behave quite the same, and so it isn't NFC.

gnprice · 2020-03-25T23:21:01Z

src/account-info/AccountDetailsScreen.js

@@ -71,6 +95,12 @@ class AccountDetailsScreen extends PureComponent<Props> {
 }

 export default connect<SelectorProps, _, _>((state, props) => ({
-  user: getAccountDetailsUserForEmail(state, props.navigation.state.params.email),


So, I think this sequence of code changes is now good!

There's some work I have to do to get to that conclusion, in the case of the change that migrates from getAccountDetailsUserForEmail to getUserForEmail: ask the questions, "what are the cases where this can behave differently? what's the old behavior in those cases, and what's the new behavior? could the new behavior be worse?", and study the code to answer them.

So the commit message should reflect that work. 🙂 I'll edit it to add that.

We are not caching any real work here. Part of zulip#3927.

Create a selector that returns a user object for a given user ID. Throws if no user exists with for the given ID.

Create a selector that returns a user object for a given email. Throws if no user exists with for the given email.

Most notably, use "see also getAllUsers" to point to details on the quirks of who counts as a user.

Currently, AccountDetailsScreen uses 'getAccountDetailsUserForEmail' to find the user object for the given email. But this selector is deprecated - it involves the use of NULL_USER. Replace it with the new selector 'getUserForEmail' instead. This can behave differently in the case where the given email doesn't belong to any known user. The existing behavior is we'll show the screen with some dummy data -- like showing the email as the user's name; trying to use the `realm_uri` as their avatar URL; and skipping presence information. That's a pretty broken-looking experience if it can happen; and if it can happen, it's a bug in the app. (There's one known case that can cause it, which is if the user in question changes their email address. This is basically part of why zulip#3764 is a bug.) Instead of showing some broken-looking bogus data, better to just show nothing. The simplest version of that is to throw an error; we could do a softer version of "show nothing", but because this case should be rare the simple thing is fine for now. [greg: expanded commit message]

The previous commit eliminated the one call site of this selector. Now we get to delete it!

We are moving towards user IDs to identify users. This commit refactors AccountDetailsScreen to take a user ID in its nav params, all call sites of navigateToAccoutDetails to pass a user ID, and makes other small changes required for that. Part of zulip#3764.

'getActiveUsersById' returns all users except those which have been deactivated as map with user ID as the key. This slector will be used in 'getUserIsActive',as a map offers a better performance than linear scans over arrays, and these values will also be cached. See zulip#3339 for discussion.

We are migrating the codebase from emails to user IDs to identify users, because user IDs are more reliable. This commit also modifies the selector to use the new selector 'getActiveUsersById'.

This will hopefully help us use user IDs instead of emails everywhere we can easily choose, as part of using user IDs everywhere, i.e. zulip#3764.

gnprice · 2020-03-26T00:31:32Z

OK -- merged! Thanks again @agrawal-d .

I made the adjustments mentioned in my last round of comments above, and added a couple of extra commits -- take a look:

3750bc5 user [nfc]: Drop caching for 'getUserIsActive'.
95d5e82 user [nfc]: Create selector 'getUserForId'.
03016a9 user [nfc]: Create selector 'getUserForEmail'.
a01decc user [nfc]: Tweak jsdoc on new selectors getUserFor{Id,Email}.
12a9935 account details: Use 'getUserForEmail' to retrieve user.
98b1b06 user [nfc]: Delete deprecated, disused getAccountDetailsUserForEmail.
239c75c account details [nfc]: Migrate from email to user ID.
28dbb99 user [nfc]: Create selector 'getActiveUsersById'.
7a44328 user [nfc]: Refactor 'getUserIsActive' to accept user ID.
680489f user [nfc]: Point to user-ID versions in docs of email-using selectors.

One other small style change, in commit messages: I shortened the prefixes on the summary lines. For example the word "selector" is right there in the later part of most of the summary lines 🙂 But also more broadly the prefix doesn't have to pinpoint the file involved, only the area or subsystem of the code. Pinpointing the file is what git log --stat (or equivalent) is for 😉

gnprice · 2020-03-26T00:41:00Z

I made the adjustments mentioned in my last round of comments above

Oh, except for this one: #3968 (comment)

That would be a nice (and small/quick) followup to do, I think.

gnprice · 2020-03-26T00:44:03Z

#3968 (comment)

That would be a nice (and small/quick) followup to do, I think.

In fact, looking over at #3878 which originally led to this PR, I think this followup would be helpful there too! You can use the same requireAttributeNumber (maybe requireNumberAttribute? requireNumericAttribute? my original suggestion kind of sounds like it's about something called an "attribute number") helper there, too, in the same way.

agrawal-d · 2020-03-26T15:19:07Z

In fact, looking over at #3878 which originally led to this PR, I think this followup would be helpful there too! You can use the same requireAttributeNumber (maybe requireNumberAttribute? requireNumericAttribute? my original suggestion kind of sounds like it's about something called an "attribute number") helper there, too, in the same way.

Will do this in the prep commits of #3878. requireNumericAttribute seems better.

This property is required (since 60847c9 / #4667, earlier this year), so we can rely on it to simplify away this workaround. This workaround was in fact the original prompt that caused me to notice we didn't have sender_id (or stream_id) on Outbox, and file #3968 (comment)

This property is required (since 60847c9 / zulip#4667, earlier this year), so we can rely on it to simplify away this workaround. This workaround was in fact the original prompt that caused me to notice we didn't have sender_id (or stream_id) on Outbox, and file zulip#3968 (comment)

agrawal-d force-pushed the userid branch 2 times, most recently from b99f4cf to b4bdc0d Compare March 12, 2020 14:26

agrawal-d force-pushed the userid branch from b4bdc0d to c3a975a Compare March 12, 2020 15:35

agrawal-d requested review from gnprice and rk-for-zulip March 12, 2020 19:05

rk-for-zulip reviewed Mar 16, 2020

View reviewed changes

agrawal-d force-pushed the userid branch from c3a975a to 702d15b Compare March 19, 2020 16:38

agrawal-d requested a review from rk-for-zulip March 19, 2020 16:45

rk-for-zulip reviewed Mar 19, 2020

View reviewed changes

gnprice reviewed Mar 20, 2020

View reviewed changes

agrawal-d force-pushed the userid branch from 702d15b to fc679e6 Compare March 23, 2020 17:18

agrawal-d requested review from gnprice and rk-for-zulip March 23, 2020 17:26

agrawal-d force-pushed the userid branch 4 times, most recently from 9fc8875 to 31d2e67 Compare March 23, 2020 18:57

gnprice reviewed Mar 24, 2020

View reviewed changes

gnprice mentioned this pull request Mar 24, 2020

Add sender_id and stream_id properties to Outbox #3998

Closed

agrawal-d force-pushed the userid branch from 31d2e67 to adc8b31 Compare March 24, 2020 09:24

agrawal-d requested a review from gnprice March 24, 2020 09:45

agrawal-d added the api migrations label Mar 24, 2020

gnprice reviewed Mar 25, 2020

View reviewed changes

agrawal-d added 3 commits March 25, 2020 17:26

user [nfc]: Drop caching for 'getUserIsActive'.

3750bc5

We are not caching any real work here. Part of zulip#3927.

user [nfc]: Create selector 'getUserForId'.

95d5e82

Create a selector that returns a user object for a given user ID. Throws if no user exists with for the given ID.

user [nfc]: Create selector 'getUserForEmail'.

03016a9

Create a selector that returns a user object for a given email. Throws if no user exists with for the given email.

gnprice and others added 7 commits March 25, 2020 17:26

user [nfc]: Tweak jsdoc on new selectors getUserFor{Id,Email}.

a01decc

Most notably, use "see also getAllUsers" to point to details on the quirks of who counts as a user.

user [nfc]: Delete deprecated, disused getAccountDetailsUserForEmail.

98b1b06

The previous commit eliminated the one call site of this selector. Now we get to delete it!

user [nfc]: Refactor 'getUserIsActive' to accept user ID.

7a44328

We are migrating the codebase from emails to user IDs to identify users, because user IDs are more reliable. This commit also modifies the selector to use the new selector 'getActiveUsersById'.

user [nfc]: Point to user-ID versions in docs of email-using selectors.

680489f

This will hopefully help us use user IDs instead of emails everywhere we can easily choose, as part of using user IDs everywhere, i.e. zulip#3764.

gnprice force-pushed the userid branch from adc8b31 to 680489f Compare March 26, 2020 00:28

gnprice merged commit 680489f into zulip:master Mar 26, 2020

agrawal-d deleted the userid branch March 26, 2020 02:38

agrawal-d mentioned this pull request Mar 26, 2020

Add feature to view profile on tapping @mention for a user #3878

Merged

		export const getUserIsActive = (state: GlobalState, userId: number): boolean =>
		[...getUsers(state), ...getCrossRealmBots(state)].some(user => user.user_id === userId);

		"Profile not found": "Profile not found",
		"The profile for this user could not be found.": "The profile for this user could not be found.",

Uh oh!

Migrate navigateToAccountDetails and getUserIsActive to user IDs #3968

Migrate navigateToAccountDetails and getUserIsActive to user IDs #3968

Uh oh!

Conversation

agrawal-d commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agrawal-d commented Mar 12, 2020

Uh oh!

rk-for-zulip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawal-d Mar 19, 2020 • 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!

rk-for-zulip Mar 19, 2020 • 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!

agrawal-d Mar 20, 2020 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agrawal-d commented Mar 19, 2020

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

agrawal-d Mar 23, 2020 • 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!

agrawal-d Mar 24, 2020 • 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!

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!

agrawal-d commented Mar 23, 2020

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

agrawal-d commented Mar 12, 2020 •

edited

Loading

agrawal-d Mar 19, 2020 •

edited

Loading

rk-for-zulip Mar 19, 2020 •

edited

Loading

agrawal-d Mar 20, 2020 •

edited

Loading

agrawal-d Mar 23, 2020 •

edited

Loading

agrawal-d Mar 24, 2020 •

edited

Loading

agrawal-d commented Mar 24, 2020 •

edited

Loading

agrawal-d Mar 26, 2020 •

edited

Loading