Skip to content

Commit

Permalink
accounts: Make Auth.realm a URL object, including in state.accounts.
Browse files Browse the repository at this point in the history
Changing the `Auth` type implicitly changes the `Account` and
`Identity` types, too.

Done with an effort to minimize unnecessary follow-on changes in
this commit. Here, we don't propagate the realm in its URL form as
far toward the eventual consumers of the realm as we'll eventually
want to; we'll do that in upcoming commits.

Wherever two realm values are compared for equality with `===`, we
make sure they are both stringified first. Flow didn't warn about
these cases, so we caught them in test failures and by scanning
through a full-project search for the term 'realm'.
  • Loading branch information
chrisbobbe committed Sep 21, 2020
1 parent 1dfec57 commit 8c0cc3f
Show file tree
Hide file tree
Showing 35 changed files with 96 additions and 67 deletions.
8 changes: 4 additions & 4 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export const makeCrossRealmBot = (args: { name?: string } = {}): CrossRealmBot =
is_bot: true,
});

export const realm = 'https://zulip.example.org';
export const realm = new URL('https://zulip.example.org');

export const zulipVersion = new ZulipVersion('2.1.0-234-g7c3acf4');

Expand All @@ -133,7 +133,7 @@ export const makeAccount = (
args: {
user?: User,
email?: string,
realm?: string,
realm?: URL,
apiKey?: string,
zulipFeatureLevel?: number | null,
zulipVersion?: ZulipVersion | null,
Expand Down Expand Up @@ -423,7 +423,7 @@ export const action = deepFreeze({
},
login_success: {
type: LOGIN_SUCCESS,
realm: selfAccount.realm,
realm: selfAccount.realm.toString(),
email: selfAccount.email,
apiKey: selfAccount.apiKey,
},
Expand Down Expand Up @@ -475,7 +475,7 @@ export const action = deepFreeze({
realm_send_welcome_emails: true,
realm_show_digest_email: true,
realm_signup_notifications_stream_id: 3,
realm_uri: selfAccount.realm,
realm_uri: selfAccount.realm.toString(),
realm_video_chat_provider: 1,
realm_waiting_period_threshold: 3,
zulip_feature_level: 1,
Expand Down
2 changes: 1 addition & 1 deletion src/account-info/AccountDetails.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,6 @@ class AccountDetails extends PureComponent<Props> {
}

export default connect<SelectorProps, _, _>((state, props) => ({
realm: getCurrentRealm(state),
realm: getCurrentRealm(state).toString(),
userStatusText: getUserStatusTextForUser(state, props.user.user_id),
}))(AccountDetails);
4 changes: 2 additions & 2 deletions src/account/AccountList.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ export default class AccountList extends PureComponent<Props> {
<View>
<FlatList
data={accounts}
keyExtractor={item => `${item.email}${item.realm}`}
keyExtractor={item => `${item.email}${item.realm.toString()}`}
ItemSeparatorComponent={() => <ViewPlaceholder height={8} />}
renderItem={({ item, index }) => (
<AccountItem
index={index}
showDoneIcon={index === 0 && item.isLoggedIn}
email={item.email}
realm={item.realm}
realm={item.realm.toString()}
onSelect={onAccountSelect}
onRemove={onAccountRemove}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/account/AccountPickScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class AccountPickScreen extends PureComponent<Props> {
dispatch(switchAccount(index));
});
} else {
dispatch(navigateToRealmScreen(realm));
dispatch(navigateToRealmScreen(realm.toString()));
}
};

Expand Down
24 changes: 12 additions & 12 deletions src/account/__tests__/accountsReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import * as eg from '../../__tests__/lib/exampleData';
describe('accountsReducer', () => {
describe('REALM_ADD', () => {
describe('on list of identities', () => {
const account1 = eg.makeAccount({ realm: 'https://realm.one.org', apiKey: '' });
const account2 = eg.makeAccount({ realm: 'https://realm.two.org', apiKey: '' });
const account1 = eg.makeAccount({ realm: new URL('https://realm.one.org'), apiKey: '' });
const account2 = eg.makeAccount({ realm: new URL('https://realm.two.org'), apiKey: '' });
const prevState = deepFreeze([account1, account2]);
const baseAction = deepFreeze({
type: REALM_ADD,
Expand All @@ -27,8 +27,8 @@ describe('accountsReducer', () => {
});

test('if no account with this realm exists, prepend new one, with empty email/apiKey', () => {
const newRealm = 'https://new.realm.org';
const action = deepFreeze({ ...baseAction, realm: newRealm });
const newRealm = new URL('https://new.realm.org');
const action = deepFreeze({ ...baseAction, realm: newRealm.toString() });
expect(accountsReducer(prevState, action)).toEqual([
eg.makeAccount({ realm: newRealm, email: '', apiKey: '' }),
account1,
Expand All @@ -37,7 +37,7 @@ describe('accountsReducer', () => {
});

test('if account with this realm exists, move to front of list', () => {
const action = deepFreeze({ ...baseAction, realm: account2.realm });
const action = deepFreeze({ ...baseAction, realm: account2.realm.toString() });
expect(accountsReducer(prevState, action)).toEqual([account2, account1]);
});
});
Expand All @@ -46,7 +46,7 @@ describe('accountsReducer', () => {
const existingAccountBase = eg.makeAccount({});
const baseAction = deepFreeze({
type: REALM_ADD,
realm: existingAccountBase.realm,
realm: existingAccountBase.realm.toString(),
zulipFeatureLevel: eg.zulipFeatureLevel,
zulipVersion: eg.zulipVersion,
});
Expand Down Expand Up @@ -153,8 +153,8 @@ describe('accountsReducer', () => {
});

describe('LOGIN_SUCCESS', () => {
const account1 = eg.makeAccount({ email: '', realm: 'https://one.example.org' });
const account2 = eg.makeAccount({ realm: 'https://two.example.org' });
const account1 = eg.makeAccount({ email: '', realm: new URL('https://one.example.org') });
const account2 = eg.makeAccount({ realm: new URL('https://two.example.org') });

const prevState = deepFreeze([account1, account2]);

Expand All @@ -168,7 +168,7 @@ describe('accountsReducer', () => {
type: LOGIN_SUCCESS,
apiKey: newAccount.apiKey,
email: newAccount.email,
realm: newAccount.realm,
realm: newAccount.realm.toString(),
});

const expectedState = [{ ...newAccount, zulipVersion: account1.zulipVersion }, account2];
Expand All @@ -181,7 +181,7 @@ describe('accountsReducer', () => {
test('on login, if account does not exist, add as first item', () => {
const newAccount = eg.makeAccount({
email: '[email protected]',
realm: 'https://new.realm.org',
realm: new URL('https://new.realm.org'),
zulipVersion: null,
zulipFeatureLevel: null,
});
Expand All @@ -190,7 +190,7 @@ describe('accountsReducer', () => {
type: LOGIN_SUCCESS,
apiKey: newAccount.apiKey,
email: newAccount.email,
realm: newAccount.realm,
realm: newAccount.realm.toString(),
});

const expectedState = [newAccount, account1, account2];
Expand All @@ -209,7 +209,7 @@ describe('accountsReducer', () => {
const action = deepFreeze({
type: LOGIN_SUCCESS,
apiKey: newAccount.apiKey,
realm: newAccount.realm,
realm: newAccount.realm.toString(),
email: newAccount.email,
});

Expand Down
3 changes: 2 additions & 1 deletion src/account/accountMisc.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ export const identityOfAuth: Auth => Identity = identitySlice;
export const identityOfAccount: Account => Identity = identitySlice;

/** A string corresponding uniquely to an identity, for use in `Map`s. */
export const keyOfIdentity = ({ realm, email }: Identity): string => `${realm}\0${email}`;
export const keyOfIdentity = ({ realm, email }: Identity): string =>
`${realm.toString()}\0${email}`;

export const authOfAccount = (account: Account): Auth => {
const { realm, email, apiKey } = account;
Expand Down
18 changes: 13 additions & 5 deletions src/account/accountsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { NULL_ARRAY } from '../nullObjects';
const initialState = NULL_ARRAY;

const realmAdd = (state, action) => {
const accountIndex = state.findIndex(account => account.realm === action.realm);
const accountIndex = state.findIndex(account => account.realm.toString() === action.realm);

if (accountIndex !== -1) {
const newAccount = {
Expand All @@ -29,7 +29,7 @@ const realmAdd = (state, action) => {

return [
{
realm: action.realm,
realm: new URL(action.realm),
apiKey: '',
email: '',
ackedPushToken: null,
Expand Down Expand Up @@ -60,16 +60,24 @@ const accountSwitch = (state, action) => {
const findAccount = (state: AccountsState, identity: Identity): number => {
const { realm, email } = identity;
return state.findIndex(
account => account.realm === realm && (!account.email || account.email === email),
account =>
account.realm.toString() === realm.toString() && (!account.email || account.email === email),
);
};

const loginSuccess = (state, action) => {
const { realm, email, apiKey } = action;
const accountIndex = findAccount(state, { realm, email });
const accountIndex = findAccount(state, { realm: new URL(realm), email });
if (accountIndex === -1) {
return [
{ realm, email, apiKey, ackedPushToken: null, zulipVersion: null, zulipFeatureLevel: null },
{
realm: new URL(realm),
email,
apiKey,
ackedPushToken: null,
zulipVersion: null,
zulipFeatureLevel: null,
},
...state,
];
}
Expand Down
2 changes: 1 addition & 1 deletion src/api/apiFetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const apiFetch = async (
auth: Auth,
route: string,
params: $Diff<$Exact<RequestOptions>, {| headers: mixed |}>,
) => fetchWithAuth(auth, `${auth.realm}/${apiVersion}/${route}`, params);
) => fetchWithAuth(auth, `${auth.realm.toString()}/${apiVersion}/${route}`, params);

export const apiCall = async (
auth: Auth,
Expand Down
2 changes: 1 addition & 1 deletion src/api/settings/getServerSettings.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,4 @@ export type ApiResponseServerSettings = {|

/** See https://zulip.com/api/server-settings */
export default async (realm: string): Promise<ApiResponseServerSettings> =>
apiGet({ apiKey: '', email: '', realm }, 'server_settings');
apiGet({ apiKey: '', email: '', realm: new URL(realm) }, 'server_settings');
2 changes: 1 addition & 1 deletion src/api/transportTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* an `Auth`.
*/
export type Auth = {|
realm: string,
realm: URL,
apiKey: string,
email: string,
|};
Expand Down
12 changes: 12 additions & 0 deletions src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ const migrations: { [string]: (GlobalState) => GlobalState } = {
...state,
accounts: state.accounts.map(a => ({
...a,
// `a.realm` is a string until migration 15
// $FlowMigrationFudge
realm: a.realm.replace(/\/+$/, ''),
})),
}),
Expand Down Expand Up @@ -187,6 +189,16 @@ const migrations: { [string]: (GlobalState) => GlobalState } = {
})),
}),

// Convert Accounts[].realm from `string` to `URL`
'15': state => ({
...state,
accounts: state.accounts.map(a => ({
...a,
// $FlowMigrationFudge - `a.realm` will be a string here
realm: new URL(a.realm),
})),
}),

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

Expand Down
2 changes: 1 addition & 1 deletion src/common/OwnAvatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ class OwnAvatar extends PureComponent<Props> {
}

export default connect(state => ({
realm: getCurrentRealm(state),
realm: getCurrentRealm(state).toString(),
user: getSelfUserDetail(state),
}))(OwnAvatar);
2 changes: 1 addition & 1 deletion src/common/UserAvatarWithPresence.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,5 @@ class UserAvatarWithPresence extends PureComponent<Props> {
}

export default connect(state => ({
realm: getCurrentRealm(state),
realm: getCurrentRealm(state).toString(),
}))(UserAvatarWithPresence);
2 changes: 1 addition & 1 deletion src/common/WebLink.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@ class WebLink extends PureComponent<Props> {
}

export default connect(state => ({
realm: getCurrentRealm(state),
realm: getCurrentRealm(state).toString(),
}))(WebLink);
8 changes: 4 additions & 4 deletions src/emoji/__tests__/emojiSelectors-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe('getActiveImageEmojiById', () => {
const state = {
accounts: [
{
realm: 'https://example.com',
realm: new URL('https://example.com'),
},
],
realm: {
Expand Down Expand Up @@ -56,7 +56,7 @@ describe('getActiveImageEmojiById', () => {
describe('getAllImageEmojiById', () => {
test('get realm emojis with absolute url', () => {
const state = {
accounts: [{ realm: 'https://example.com' }],
accounts: [{ realm: new URL('https://example.com') }],
realm: {
emoji: {
1: {
Expand Down Expand Up @@ -87,7 +87,7 @@ describe('getAllImageEmojiById', () => {
describe('getAllImageEmojiByCode', () => {
test('get realm emoji object with emoji names as the keys', () => {
const state = {
accounts: [{ realm: 'https://example.com' }],
accounts: [{ realm: new URL('https://example.com') }],
realm: {
emoji: {
1: {
Expand Down Expand Up @@ -129,7 +129,7 @@ describe('getAllImageEmojiByCode', () => {
describe('getActiveImageEmojiByName', () => {
test('get realm emoji object with emoji names as the keys', () => {
const state = {
accounts: [{ realm: 'https://example.com' }],
accounts: [{ realm: new URL('https://example.com') }],
realm: {
emoji: {
1: {
Expand Down
2 changes: 1 addition & 1 deletion src/lightbox/Lightbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class Lightbox extends PureComponent<Props, State> {
<LightboxHeader
onPressBack={this.handlePressBack}
timestamp={message.timestamp}
avatarUrl={getAvatarFromMessage(message, auth.realm)}
avatarUrl={getAvatarFromMessage(message, auth.realm.toString())}
senderName={message.sender_full_name}
/>
</SlideAnimationView>
Expand Down
2 changes: 1 addition & 1 deletion src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat
},
}),
),
tryFetch(() => api.getServerSettings(auth.realm)),
tryFetch(() => api.getServerSettings(auth.realm.toString())),
]);
} catch (e) {
// This should only happen on a 4xx HTTP status, which should only
Expand Down
6 changes: 3 additions & 3 deletions src/message/messagesActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ export const messageLinkPress = (href: string) => async (
const auth = getAuth(state);
const usersById = getUsersById(state);
const streamsById = getStreamsById(state);
const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById);
const narrow = getNarrowFromLink(href, auth.realm.toString(), usersById, streamsById);
if (narrow) {
const anchor = getMessageIdFromLink(href, auth.realm);
const anchor = getMessageIdFromLink(href, auth.realm.toString());
dispatch(doNarrow(narrow, anchor));
} else if (!isUrlOnRealm(href, auth.realm)) {
} else if (!isUrlOnRealm(href, auth.realm.toString())) {
openLink(href);
} else {
const url =
Expand Down
10 changes: 6 additions & 4 deletions src/nav/__tests__/navReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('navReducer', () => {
const action = deepFreeze({
type: REHYDRATE,
payload: {
accounts: [{ apiKey: '', realm: 'https://example.com' }],
accounts: [{ apiKey: '', realm: new URL('https://example.com') }],
users: [],
realm: {},
},
Expand All @@ -147,8 +147,8 @@ describe('navReducer', () => {
type: REHYDRATE,
payload: {
accounts: [
{ apiKey: '', realm: 'https://example.com', email: '[email protected]' },
{ apiKey: '', realm: 'https://example.com', email: '[email protected]' },
{ apiKey: '', realm: new URL('https://example.com'), email: '[email protected]' },
{ apiKey: '', realm: new URL('https://example.com'), email: '[email protected]' },
],
users: [],
realm: {},
Expand All @@ -165,7 +165,9 @@ describe('navReducer', () => {
const action = deepFreeze({
type: REHYDRATE,
payload: {
accounts: [{ apiKey: '', realm: 'https://example.com', email: '[email protected]' }],
accounts: [
{ apiKey: '', realm: new URL('https://example.com'), email: '[email protected]' },
],
users: [],
realm: {},
},
Expand Down
Loading

0 comments on commit 8c0cc3f

Please sign in to comment.