streams: Show alert when stream name already exists. #5331

thisisnitish · 2022-04-09T09:07:24Z

Show Alert when when we try to create a stream with an existing name.

chrisbobbe

Thanks, @thisisnitish! See a few small comments below.

chrisbobbe · 2022-04-11T17:19:04Z

src/streams/CreateStreamScreen.js

+  const getStreams = useSelector(getStreamsByName);
+  const allStreams = Array.from(getStreams.keys());


The getStreamsByName selector gives a Map<string, Stream>, i.e., a Map where keys are strings and values are Streams. As the name suggests, the keys are stream names.

Map gives you a way to see if there's a value at a given key, so you don't need to make an Array to see if we know about a stream: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/has

Sure, @chrisbobbe!, Today evening, I got the idea of this i.e. has and kind of realised that, I need to change here 😉 and thankfully you pointed out.

chrisbobbe · 2022-04-11T17:27:24Z

src/streams/CreateStreamScreen.js

+      if (allStreams.includes(name)) {
+        showErrorAlert('A Stream with this name already exists');
+      } else {
+        api.createStream(auth, name, description, [ownEmail], isPrivate);
+        NavigationService.dispatch(navigateBack());
+      }


Instead of putting the normal, happy-case code in an else, let's use the "early return" pattern here. So, in the if, just after the showErrorAlert line, add a return line. Then pull what's currently in the else block outward. Like this:

if (allStreams.includes(name)) { showErrorAlert('A Stream with this name already exists'); return; } api.createStream(auth, name, description, [ownEmail], isPrivate); NavigationService.dispatch(navigateBack());

For Greg's reasoning about this preference, see #4980 (comment) .

chrisbobbe · 2022-04-11T17:29:24Z

src/streams/CreateStreamScreen.js

-      api.createStream(auth, name, description, [ownEmail], isPrivate);
-      NavigationService.dispatch(navigateBack());
+      if (allStreams.includes(name)) {
+        showErrorAlert('A Stream with this name already exists');


Lowercase "stream", and put a period at the end: "A stream with this name already exists."

Then, please add an entry to static/translations/messages_en.json, as mentioned in our translations doc (docs/howto/translations.md).

chrisbobbe · 2022-04-11T17:40:12Z

src/streams/CreateStreamScreen.js

    (name: string, description: string, isPrivate: boolean) => {
-      api.createStream(auth, name, description, [ownEmail], isPrivate);
-      NavigationService.dispatch(navigateBack());
+      if (allStreams.includes(name)) {


There's an edge case that I'm not sure how best to handle. I think it shouldn't block progress on the PR, but it'd be good to write it down in a code comment here, so at least the behavior is clear.

It's possible that a stream exists but the client doesn't know about it, for example, if it's a private stream that the user can't access.

Maybe something like

// This will miss existing streams that the client can't know about; // for example, a private stream the user can't access.

just above the if line.

I think it shouldn't block progress on the PR

Ah, I take that back; Greg has suggested something to do about this: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5199/near/1364442

Sure @chrisbobbe!, Thanks for thinking about this edge case. I think we forgot to discuss this edge case in our discussion. I'll add it.
Thanks 😊

chrisbobbe · 2022-04-11T17:44:56Z

src/streams/CreateStreamScreen.js

 import { useSelector } from '../react-redux';
 import { navigateBack } from '../actions';
 import { getAuth, getOwnEmail } from '../selectors';
+import { getStreamsByName } from '../subscriptions/subscriptionSelectors';


streams: Show alert when stream name already exists. Show Alert when when we try to create a stream with an existing name. Fixes: https://github.com/zulip/zulip-mobile/issues/5199

Commit message nit: Let's link to the discussion where we decided on this approach; perhaps here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5199/near/1363029 .

There were a few things that suggested this approach instead of others (one being the awkwardness of the server API); it could be helpful context in case we look into changing the design in the future.

thisisnitish · 2022-04-11T21:32:26Z

Thanks for review @chrisbobbe!, I've made changes to the PR. Please have a look. Thanks 😊

chrisbobbe

Thanks for the revision. Please see one small comment below. Also, did you see #5331 (comment) ?

chrisbobbe · 2022-04-11T22:01:22Z

src/streams/CreateStreamScreen.js

 export default function CreateStreamScreen(props: Props): Node {
  const auth = useSelector(getAuth);
  const ownEmail = useSelector(getOwnEmail);
+  const getStreams = useSelector(getStreamsByName);


getStreams isn't the right name for this variable, which holds a Map<string, Stream> value. It makes it sound like the value is a function that, when called, will "get streams".

Let's use streamsByName. That's a more natural name for the result of using the getStreamsByName selector.

Sure @chrisbobbe!, I'll change the name.

thisisnitish · 2022-04-12T20:07:01Z

Also, did you see #5331 (comment) ?

Yes @chrisbobbe!, I read that comment.
So, now we want to handle the edge case where, the stream already exists and user isn't aware of the stream because the stream itself is a private stream.
So as per the suggestion from https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5199/near/1363029, the server will always throw an error and we have handle that error from the server.

So, I've one more doubt @chrisbobbe!, I'm still not able to understand

we have handle that error from the server.

It would be a great help, if you could explain this line a little. Thanks 😊

chrisbobbe · 2022-04-13T23:41:34Z

I've responded in the chat thread: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5199/near/1366320

thisisnitish · 2022-04-30T14:45:47Z

Hey @chrisbobbe!, I made some changes please have a look.
Here is the preview

Simulator.Screen.Recording.-.iPhone.12.-.2022-04-30.at.19.18.43.mp4

I hope this will be fine 🤞🏻
Thanks 😊

Show Alert when when we try to create a stream with an existing name. Refer Discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5199/near/1363029 Fixes: zulip#5199 Co-authored-by: Chris Bobbe <[email protected]>

chrisbobbe

Thanks!

I've just rebased, resolving some rebase conflicts with some recent work I did. I'll go ahead and merge, with a few fixes:

I noticed that we weren't using _ for the 'A stream with this name already exists' text; I've done that.
I squashed the two commits together; see comments.

About the second point, here's a note about the benefits of Zulip's version-control philosophy:

Zulip's commit discipline makes review easier and it makes the project's history clearer. (You may have used Greg's "secret" to browsing history so many times that it's instinctive by now. If not, I recommend it!) Our style lets us confidently make complex changes. A clean record of the code's history is useful for everybody, especially new contributors and anyone reading commits years after they were written.

chrisbobbe · 2022-05-02T20:29:00Z

src/streams/CreateStreamScreen.js

+        // If the stream already exists but you can't access it (e.g., it's
+        // private), then it won't be in the client's data structures, so our
+        // client-side check won't stop the request from being made. In that case,
+        // we expect the server to always give an error, because you can't subscribe
+        // to a stream that you can't access. That error will have:
+        // - error.message: `Unable to access stream (${streamName})`, or a
+        //   translation of that into the user's own language
+        // - error.code: "BAD_REQUEST" (as of server feature level 126; possibly
+        //   the server should be more specific)


Hmm. Your second commit—

Added Comments before showing alert for accessing Private Stream. Refer dicussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M5199.20error.20for.20name.20collision.20on.20creating.20stream

—is best squashed into the first. Its behavior changes are part of showing an alert when a stream name is unavailable, which is what the first commit is about. I'll squash it myself, but please develop the habit of submitting coherent commits for review, and please ask questions, or find examples yourself, when you're not sure how best to do that.

Also (and this won't be relevant once I squash the commits together, but for the future): the commit message of this second commit is very misleading. It makes it sound like the added comment is the only thing that changes in the commit. Comments don't change how the app behaves, so it sounds like the app's behavior doesn't change in this commit. But that's not true: the commit adds handling of the error from the server. If a commit intentionally changes the app's behavior, that fact should be obvious in the commit message, preferably in the summary line. Otherwise, the behavior changes are easy to miss and will appear accidental or poorly planned, which this change is not. 🙂

Umm, Sorry @chrisbobbe! for this one. Actually, I forgot to squash the commits. From now onwards, I'll keep this in mind. Thanks 😊

chrisbobbe · 2022-05-02T22:52:13Z

src/streams/CreateStreamScreen.js

      // This will miss existing streams that the client can't know about;
      // for example, a private stream the user can't access.
-      if (getStreams.has(name)) {
+      if (streamsByName.has(name)) {


This rename should be squashed into the commit that added the wrong name, so there aren't any commits with the wrong name. I'll take care of that before merging.

thisisnitish mentioned this pull request Apr 9, 2022

Not Showing any alert if stream name already exist. #5199

Closed

chrisbobbe requested changes Apr 11, 2022

View reviewed changes

thisisnitish force-pushed the stream_name_exist branch from a1d6c7f to bd1a6b7 Compare April 11, 2022 21:19

chrisbobbe requested changes Apr 11, 2022

View reviewed changes

chrisbobbe force-pushed the stream_name_exist branch 2 times, most recently from 1c90b66 to 0200c1e Compare May 2, 2022 23:37

chrisbobbe reviewed May 2, 2022

View reviewed changes

chrisbobbe force-pushed the stream_name_exist branch from 0200c1e to 1c90b66 Compare May 2, 2022 23:45

chrisbobbe merged commit 1c90b66 into zulip:main May 2, 2022

		const getStreams = useSelector(getStreamsByName);
		const allStreams = Array.from(getStreams.keys());

Uh oh!

Uh oh!

streams: Show alert when stream name already exists. #5331

streams: Show alert when stream name already exists. #5331

Uh oh!

Conversation

thisisnitish commented Apr 9, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Apr 11, 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!

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!

thisisnitish commented Apr 11, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Apr 11, 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!

thisisnitish commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Apr 13, 2022

Uh oh!

thisisnitish commented Apr 30, 2022

Uh oh!

chrisbobbe left a comment • 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 May 2, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe Apr 11, 2022 •

edited

Loading

chrisbobbe Apr 11, 2022 •

edited

Loading

thisisnitish commented Apr 12, 2022 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe May 2, 2022 •

edited

Loading