Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions src/streams/CreateStreamScreen.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
/* @flow strict-local */
import React, { useCallback } from 'react';
import React, { useCallback, useContext } from 'react';
import type { Node } from 'react';

import { TranslationContext } from '../boot/TranslationProvider';
import type { RouteProp } from '../react-navigation';
import type { AppNavigationProp } from '../nav/AppNavigator';
import * as NavigationService from '../nav/NavigationService';
import { useSelector } from '../react-redux';
import { navigateBack } from '../actions';
import { getAuth } from '../selectors';
import { getStreamsByName } from '../subscriptions/subscriptionSelectors';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import Screen from '../common/Screen';
import EditStreamCard from './EditStreamCard';
import { showErrorAlert } from '../utils/info';
import { ApiError } from '../api/apiErrors';
import * as api from '../api';

type Props = $ReadOnly<{|
Expand All @@ -18,14 +22,41 @@ type Props = $ReadOnly<{|
|}>;

export default function CreateStreamScreen(props: Props): Node {
const _ = useContext(TranslationContext);

const auth = useSelector(getAuth);
const streamsByName = useSelector(getStreamsByName);

const handleComplete = useCallback(
(name: string, description: string, invite_only: boolean) => {
api.createStream(auth, { name, description, invite_only });
NavigationService.dispatch(navigateBack());
async (name: string, description: string, invite_only: boolean) => {
// This will miss existing streams that the client can't know about;
// for example, a private stream the user can't access. See comment
// where we catch an `ApiError`, below.
if (streamsByName.has(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

showErrorAlert(_('A stream with this name already exists.'));
return;
}

try {
await api.createStream(auth, { name, description, invite_only });
NavigationService.dispatch(navigateBack());
} catch (error) {
// 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 above 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)
if (error instanceof ApiError) {
showErrorAlert(error.message);
}
}
},
[auth],
[auth, streamsByName, _],
);

return (
Expand Down
2 changes: 1 addition & 1 deletion src/streams/EditStreamCard.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type Props = $ReadOnly<{|
description: string,
invite_only: boolean,
|},
onComplete: (name: string, description: string, invite_only: boolean) => void,
onComplete: (name: string, description: string, invite_only: boolean) => void | Promise<void>,
|}>;

type State = {|
Expand Down
3 changes: 2 additions & 1 deletion static/translations/messages_en.json
Original file line number Diff line number Diff line change
Expand Up @@ -300,5 +300,6 @@
"Copy link to topic": "Copy link to topic",
"Failed to copy topic link": "Failed to copy topic link",
"Copy link to stream": "Copy link to stream",
"Failed to copy stream link": "Failed to copy stream link"
"Failed to copy stream link": "Failed to copy stream link",
"A stream with this name already exists.": "A stream with this name already exists."
}