Skip to content

outbox: Make stream_id required on StreamOutbox #5056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 19, 2021
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
1 change: 1 addition & 0 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ export const streamOutbox = (data: $Rest<StreamOutbox, { id: mixed, ... }>): Str
...outboxMessageBase,
type: 'stream',
display_recipient: stream.name,
stream_id: stream.stream_id,
subject: 'test topic',

...data,
Expand Down
6 changes: 6 additions & 0 deletions src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,12 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = {
// invariant that if there's server data, then the active Account has userId.
'34': dropCache,

// Make `stream_id` on `StreamOutbox` required.
'35': state => ({
...state,
outbox: state.outbox.filter(o => o.type === 'private' || o.stream_id !== undefined),
}),

// TIP: When adding a migration, consider just using `dropCache`.
};

Expand Down
1 change: 0 additions & 1 deletion src/outbox/outboxActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ const trySendMessages = (dispatch, getState): boolean => {
// TODO(server-2.0): switch to numeric user IDs, not emails.
? recipientsOfPrivateMessage(item).map(r => r.email).join(',')
// TODO(server-2.0): switch to numeric stream IDs, not names.
// (This will require wiring the stream ID through to here.)
// HACK: the server attempts to interpret this argument as JSON, then
// CSV, then a literal. To avoid misparsing, always use JSON.
: JSON.stringify([streamNameOfStreamMessage(item)]);
Expand Down
8 changes: 2 additions & 6 deletions src/topics/topicActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ import type { Narrow, Topic, Action, ThunkAction, Outbox } from '../types';
import * as api from '../api';
import { INIT_TOPICS } from '../actionConstants';
import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow';
import { getAuth, getStreams, getStreamsById } from '../selectors';
import { getAuth, getStreams } from '../selectors';
import { deleteOutboxMessage } from '../actions';
import { getOutbox } from '../directSelectors';
import { streamNameOfStreamMessage } from '../utils/recipient';

export const initTopics = (topics: Topic[], streamId: number): Action => ({
type: INIT_TOPICS,
Expand Down Expand Up @@ -49,14 +48,11 @@ export const deleteMessagesForTopic = (
): ThunkAction<Promise<void>> => async (dispatch, getState) => {
const state = getState();
const outbox = getOutbox(state);
const stream = getStreamsById(state).get(streamId);

outbox.forEach((outboxMessage: Outbox) => {
if (
outboxMessage.type === 'stream'
// TODO: Use StreamId here (see #M3918) when `Outbox` starts
// having that property (see #M3998).
&& streamNameOfStreamMessage(outboxMessage) === stream?.name
&& outboxMessage.stream_id === streamId
&& outboxMessage.subject === topic
) {
dispatch(deleteOutboxMessage(outboxMessage.id));
Expand Down
10 changes: 1 addition & 9 deletions src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,19 +228,12 @@ export type PmOutbox = $ReadOnly<{|
export type StreamOutbox = $ReadOnly<{|
...OutboxBase,

// TODO(#3764): Make stream_id required. First need to start supplying it
// in the Outbox values we create; compare a1fad7ca9, for sender_id.
//
// Once it is required, it should move from here to the second type
// argument passed to `SubsetProperties` of `StreamMessage`, below;
// and we can also drop the hack line about it in `MessageLike`.
stream_id?: number,

...SubsetProperties<
StreamMessage,
{|
type: mixed,
display_recipient: mixed,
stream_id: mixed,
subject: mixed,
|},
>,
Expand Down Expand Up @@ -302,7 +295,6 @@ export type MessageLike =
| $ReadOnly<{|
// $Shape<T> is unsound, per Flow docs, but $ReadOnly<$Shape<T>> is not
...$Shape<{| [$Keys<Message>]: void |}>,
stream_id?: number, // TODO: Drop this once required in StreamOutbox.
...Outbox,
|}>;

Expand Down
3 changes: 1 addition & 2 deletions src/utils/recipient.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,7 @@ export const isSameRecipient = (
}

return (
streamNameOfStreamMessage(message1).toLowerCase()
=== streamNameOfStreamMessage(message2).toLowerCase()
message1.stream_id === message2.stream_id
&& message1.subject.toLowerCase() === message2.subject.toLowerCase()
);
default:
Expand Down
8 changes: 2 additions & 6 deletions src/webview/handleOutboundEvents.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/* @flow strict-local */
import { Clipboard, Alert } from 'react-native';

import invariant from 'invariant';
import * as NavigationService from '../nav/NavigationService';
import * as api from '../api';
import config from '../config';
Expand All @@ -10,7 +9,7 @@ import type { BackgroundData } from './MessageList';
import type { ShowActionSheetWithOptions } from '../action-sheets';
import type { JSONableDict } from '../utils/jsonable';
import { showToast } from '../utils/info';
import { streamNameOfStreamMessage, pmUiRecipientsFromMessage } from '../utils/recipient';
import { pmUiRecipientsFromMessage } from '../utils/recipient';
import { isUrlAnImage } from '../utils/url';
import * as logging from '../utils/logging';
import { filterUnreadMessagesInRange } from '../utils/unread';
Expand Down Expand Up @@ -221,14 +220,11 @@ const handleLongPress = (
const { dispatch, showActionSheetWithOptions, backgroundData, narrow, startEditMessage } = props;
if (target === 'header') {
if (message.type === 'stream') {
const streamName = streamNameOfStreamMessage(message);
const stream = backgroundData.streamsByName.get(streamName);
invariant(stream !== undefined, 'No stream with provided stream name was found.');
showTopicActionSheet({
showActionSheetWithOptions,
callbacks: { dispatch, _ },
backgroundData,
streamId: stream.stream_id,
streamId: message.stream_id,
topic: message.subject,
});
} else if (message.type === 'private') {
Expand Down
12 changes: 2 additions & 10 deletions src/webview/html/header.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,9 @@ export default (
`;
} else if (headerStyle === 'full') {
const streamName = streamNameOfStreamMessage(message);
const subscription = subscriptions.get(message.stream_id);

// TODO(#3339): like `subscriptions.find(…)`, but for the values from …ById
let stream = null;
for (const sub of subscriptions.values()) {
if (sub.name === streamName) {
stream = sub;
break;
}
}

const backgroundColor = stream ? stream.color : 'hsl(0, 0%, 80%)';
const backgroundColor = subscription ? subscription.color : 'hsl(0, 0%, 80%)';
const textColor = foregroundColorFromBackground(backgroundColor);
const streamNarrowStr = keyFromNarrow(streamNarrow(streamName));
const topicNarrowStr = keyFromNarrow(topicNarrow(streamName, message.subject));
Expand Down
3 changes: 1 addition & 2 deletions src/webview/html/message.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,7 @@ $!${divOpenHtml}
`;
}

const { sender_full_name } = message;
const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id;
const { sender_full_name, sender_id } = message;
const avatarUrl = message.avatar_url
.get(
// 48 logical pixels; see `.avatar` and `.avatar img` in
Expand Down