diff --git a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt index 760f51c9db1..d5fbc48a9c4 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt @@ -197,10 +197,12 @@ private fun extractIdentity(data: Map): Identity = // use this as a substitute for `user_id` when that's missing... // but it'd be inherently buggy, and the bug it'd introduce seems // likely to affect more users than the bug it'd fix. So just ignore. + // TODO(server-2.0): Delete this comment. // (data["user"] ignored) // `user_id` was added in server version 2.1.0 (released 2019-12-12; // commit 447a517e6, PR #12172.) + // TODO(server-2.1): Require this. userId = data["user_id"]?.parseInt("user_id") ) diff --git a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt index 91c5377353b..a875843ef84 100644 --- a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt +++ b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt @@ -198,6 +198,7 @@ class RemoveFcmMessageTest : FcmMessageTestBase() { )) /// The Zulip server before v2.0 sends this form (plus some irrelevant fields). + // TODO(server-2.0): Drop this, and the logic it tests. val singular = base.plus(sequenceOf( "zulip_message_id" to "123" )) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index e4688e2b060..1bb39e865e6 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -328,7 +328,6 @@ const messagePropertiesFromSender = (user: User) => { const { user_id: sender_id, email: sender_email, full_name: sender_full_name } = user; return deepFreeze({ - sender_domain: '', avatar_url: user.avatar_url, client: 'ExampleClient', gravatar_hash: 'd3adb33f', @@ -714,8 +713,8 @@ export const action = Object.freeze({ anchor: 0, numBefore: 50, numAfter: 50, - foundNewest: undefined, - foundOldest: undefined, + foundNewest: false, + foundOldest: false, ownUserId: selfUser.user_id, }): MessageFetchCompleteAction), // If a given action is only relevant to a single test file, no need to diff --git a/src/account/accountsSelectors.js b/src/account/accountsSelectors.js index ffa6f1295ec..6f817b8c7c8 100644 --- a/src/account/accountsSelectors.js +++ b/src/account/accountsSelectors.js @@ -89,8 +89,22 @@ export const getAccount = (state: PerAccountState): Account => { /** The realm URL for this account. */ export const getRealmUrl = (state: PerAccountState): URL => getAccount(state).realm; -/** The Zulip server version for this account. */ -export const getZulipVersion = (state: PerAccountState): ZulipVersion => { +/** + * The Zulip server version for this account. + * + * Prefer `getZulipFeatureLevel`, which is finer-grained, for most uses. + * This function is useful for logging, for user-facing information, and for + * old releases (pre-3.0) where the feature level is always 0. + * + * This function assumes we have server data for this account, and if not it + * may throw. If you want to call it from a context where we may not have + * server data, we can fix that; see implementation comment. + * + * If using this to condition behavior where we make a request to the + * server, note that there's fundamentally a race; for details, see + * `getZulipFeatureLevel`. + */ +export const getServerVersion = (state: PerAccountState): ZulipVersion => { const { zulipVersion } = getAccount(state); // This invariant will hold as long as we only call this function in a // context where we have server data. @@ -107,13 +121,29 @@ export const getZulipVersion = (state: PerAccountState): ZulipVersion => { return zulipVersion; }; -/** The Zulip server feature level for this account. */ +/** + * The Zulip server feature level for this account. + * + * This function assumes we have server data for this account, and if not it + * may throw. If you want to call it from a context where we may not have + * server data, we can fix that; see implementation comment. + * + * If using this to condition behavior where we make a request to the + * server, note that there's fundamentally a race: the server may have been + * upgraded since we last heard from it. Generally we don't worry about + * this because (a) usually our behavior for old servers is fine for new + * servers (by design in the new server, because it's the same behavior old + * clients will have), and then this can only be a problem on downgrade, + * which is rare; (b) with the exception of initial fetch, almost all our + * requests come while we already have a live event queue which would tell + * us about an upgrade, so the race is quite narrow. + */ export const getZulipFeatureLevel = (state: PerAccountState): number => { const { zulipFeatureLevel } = getAccount(state); // This invariant will hold as long as we only call this function in a // context where we have server data. // - // TODO(#5006): Much like getZulipVersion above. This property is just a + // TODO(#5006): Much like getServerVersion above. This property is just a // bit newer: b058fa266, from 2020-09. invariant(zulipFeatureLevel !== null, 'zulipFeatureLevel must be non-null in PerAccountState'); return zulipFeatureLevel; @@ -178,12 +208,3 @@ export const getAuth = (state: PerAccountState): Auth => { export const getIdentity: Selector = createSelector(getAuth, auth => identityOfAuth(auth), ); - -/** - * The Zulip server version for this account, or null if unknown. - * - * See the `zulipVersion` property of `Account` for details on how this - * information is kept up to date. - */ -export const getServerVersion = (state: PerAccountState): ZulipVersion | null => - getAccount(state).zulipVersion; diff --git a/src/actionTypes.js b/src/actionTypes.js index b9e9a934f62..50aafd18e5a 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -221,8 +221,8 @@ export type MessageFetchCompleteAction = {| anchor: number, numBefore: number, numAfter: number, - foundNewest: boolean | void, - foundOldest: boolean | void, + foundNewest: boolean, + foundOldest: boolean, ownUserId: UserId, |}; @@ -297,8 +297,8 @@ type EventSubscriptionUpdateAction = {| property: string, value: boolean | number | string, + // TODO(server-4.0): Delete these commented-out properties. // name: string, // exists pre-4.0, but expected to be removed soon - // email: string, // gone in 4.0; was the user's own email, so never useful |}; diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index 999a04400df..1a31d699cdd 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -145,6 +145,7 @@ export type UpdateMessageFlagsEvent = $ReadOnly<{| // Servers with feature level 32+ send `op`. Servers will eventually // stop sending `operation`; see #4238. + // TODO(server-4.0): Simplify to just `op`. operation?: 'add' | 'remove', op?: 'add' | 'remove', @@ -168,6 +169,7 @@ export type RestartEvent = $ReadOnly<{| // // They have the same shape and meaning as the same-named fields in // the /server_settings and /register responses. + // TODO(server-4.0): Mark these as required. zulip_version?: string, zulip_feature_level?: number, |}>; diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index d6fff130c5d..709edc62184 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -30,10 +30,11 @@ export type InitialDataBase = $ReadOnly<{| // `fetch_event_types` and remove this comment. /** - * Added in server version 2.2, feature level 1. + * Added in server version 3.0, feature level 1. * Same meaning as in the server_settings response: * https://zulip.com/api/get-server-settings. See also the comment above. */ + // TODO(server-3.0): Mark as required. zulip_feature_level?: number, /** @@ -59,8 +60,8 @@ export type InitialDataMutedTopics = $ReadOnly<{| muted_topics: $ReadOnlyArray, |}>; -/** Added in server version 4.0, feature level 48 */ export type InitialDataMutedUsers = $ReadOnly<{| + /** (When absent, treat as empty. Added in server version 4.0, feature level 48.) */ muted_users?: $ReadOnlyArray, |}>; @@ -124,6 +125,7 @@ export type InitialDataRealmEmoji = $ReadOnly<{| export type RawInitialDataRealmFilters = $ReadOnly<{| // We still request this, since not all servers can provide the // newer `realm_linkifiers` format. + // TODO(server-4.0): Drop this. realm_filters?: $ReadOnlyArray, |}>; @@ -173,14 +175,17 @@ export type InitialDataRealmUserGroups = $ReadOnly<{| /** * Absent in servers prior to v1.8.0-rc1~2711 (or thereabouts). */ + // TODO(server-1.8): Mark as required. realm_user_groups?: $ReadOnlyArray, |}>; export type InitialDataRecentPmConversations = $ReadOnly<{| // * Added in server commit 2.1-dev-384-g4c3c669b41. + // TODO(server-2.1): Mark this required. See MIN_RECENTPMS_SERVER_VERSION. // * `user_id` fields are sorted as of commit 2.2-dev-53-g405a529340, which // was backported to 2.1.1-50-gd452ad31e0 -- meaning that they are _not_ // sorted in either v2.1.0 or v2.1.1. + // TODO(server-3.0): Simply say these are sorted. ("2.2" became 3.0.) recent_private_conversations?: $ReadOnlyArray, |}>; diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index 712d201ec02..16631a9bb37 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -10,9 +10,9 @@ import { AvatarURL } from '../../utils/avatar'; type ApiResponseMessages = {| ...$Exact, anchor: number, - found_anchor?: boolean, - found_newest?: boolean, - found_oldest?: boolean, + found_anchor: boolean, + found_newest: boolean, + found_oldest: boolean, messages: Message[], |}; @@ -25,6 +25,7 @@ type ApiResponseMessages = {| // We shouldn't have to rely on this format on servers at feature // level 2+; those newer servers include a top-level `user_id` field // in addition to the `user` object. See #4072. +// TODO(server-3.0): Simplify this away. export type ServerReaction = $ReadOnly<{| ...$Diff, user: $ReadOnly<{| @@ -83,11 +84,6 @@ const migrateResponse = (response, identity: Identity) => { /** * See https://zulip.com/api/get-messages - * - * These values exist only in Zulip 1.8 or newer: - * * found_anchor - * * found_newest - * * found_oldest */ export default async ( auth: Auth, diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 6e0c3d13536..836666886da 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -37,6 +37,7 @@ export type RealmEmojiById = $ReadOnly<{| /** * The only way servers before feature level 54 represent linkifiers. */ +// TODO(server-4.0): Delete this. export type RealmFilter = [string, string, number]; /** @@ -52,8 +53,8 @@ export type RealmFilter = [string, string, number]; // `realm_linkifiers` format. (When doing so, also don't forget to // change various variable and type definition names to be like // `realm_linkifiers`.) -// - When we drop support for servers older than 54, we can remove all -// our code that knows about the `realm_filters` format. +// - TODO(server-4.0): When we drop support for servers older than 54, we +// can remove all our code that knows about the `realm_filters` format. export type RealmLinkifier = $ReadOnly<{| id: number, pattern: string, @@ -108,6 +109,7 @@ export type User = $ReadOnly<{| full_name: string, // date_joined included since commit 372e9740a (in 1.9.0) + // TODO(server-1.9): mark this as required date_joined?: string, // is_active doesn't appear in `/register` responses -- instead, @@ -155,7 +157,7 @@ export type User = $ReadOnly<{| // profile_data added in commit 02b845336 (in 1.8.0); // see also e3aed0f7b (in 2.0.0) // (This one doesn't appear in `/users` responses.) - profile_data?: empty, // TODO describe actual type + profile_data?: empty, // When we need this, describe its actual type. |}>; /** @@ -177,6 +179,7 @@ export type CrossRealmBot = $ReadOnly<{| avatar_url: AvatarURL, // date_joined included since commit 58ee3fa8c (in 1.9.0) + // TODO(server-1.9): mark this as required date_joined?: string, email: string, @@ -351,6 +354,7 @@ export type NarrowElement = // * `group-pm-with` since 2.1-dev-1813-gb338fd130 // * `sender` since 2.1-dev-1812-gc067c155a // * `pm-with` since 2.1-dev-1350-gd7b4de234 + // TODO(server-2.1): Stop sending stream names or user emails here. | {| +operator: 'stream', +operand: string | number |} // stream ID | {| +operator: 'pm-with', +operand: string | $ReadOnlyArray |} | {| +operator: 'sender', +operand: string | UserId |} @@ -539,9 +543,6 @@ type MessageBase = $ReadOnly<{| match_content?: string, match_subject?: string, - /** Obsolete? Gone in server commit 1.6.0~1758 . */ - sender_domain: string, - /** * The `flags` story is a bit complicated: * * Absent in `event.message` for a `message` event... but instead @@ -598,7 +599,7 @@ export type PmMessage = $ReadOnly<{| // Notes from studying the server code: // * Notes are primarily from the server as of 2020-04 at cb85763c7, but // this logic is very stable; confirmed all points about behavior as of - // 1.8.0, too. + // 1.8.0 (from 2018-04), too. // // * This field is ultimately computed (for both events and /messages // results) in MessageDict.hydrate_recipient_info, with most of the @@ -725,5 +726,6 @@ export type RecentPrivateConversation = $ReadOnly<{| max_message_id: number, // When received from the server, these are guaranteed to be sorted only after // 2.2-dev-53-g405a529340. To be safe, we always sort them on receipt. + // TODO(server-3.0): Stop sorting these ourselves. ("2.2" became 3.0.) user_ids: $ReadOnlyArray, |}>; diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index 8fc437dcb29..a158ec5e2d5 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -54,6 +54,8 @@ const transform = (rawInitialData: RawInitialData, auth: Auth): InitialData => ( // Transform the newer `realm_linkifiers` format, if present, to the // older `realm_filters` format. We do the same transformation on // 'realm_linkifiers' events. + // TODO(server-4.0): Switch to new format, if we haven't already; + // and drop conversion. realm_filters: rawInitialData.realm_linkifiers ? rawInitialData.realm_linkifiers.map(({ pattern, url_format, id }) => [ pattern, diff --git a/src/api/settings/getServerSettings.js b/src/api/settings/getServerSettings.js index f0b34c867f2..2c8bbb515ff 100644 --- a/src/api/settings/getServerSettings.js +++ b/src/api/settings/getServerSettings.js @@ -26,6 +26,7 @@ export type ApiResponseServerSettings = {| ...$Exact, authentication_methods: AuthenticationMethods, // external_authentication_methods added for server v2.1 + // TODO(server-2.1): Mark this as required; simplify downstream. external_authentication_methods?: ExternalAuthenticationMethod[], email_auth_enabled: boolean, push_notifications_enabled: boolean, @@ -36,8 +37,9 @@ export type ApiResponseServerSettings = {| require_email_format_usernames: boolean, zulip_version: string, - // zulip_feature_level added for server v2.2, feature level 1 + // zulip_feature_level added for server v3.0, feature level 1 // See https://zulip.com/api/get-server-settings + // When absent, equivalent to 0. zulip_feature_level?: number, |}; diff --git a/src/api/streams/updateStream.js b/src/api/streams/updateStream.js index ae9af9f7901..47d2a6bb66e 100644 --- a/src/api/streams/updateStream.js +++ b/src/api/streams/updateStream.js @@ -9,6 +9,7 @@ import { apiPatch } from '../apiFetch'; */ // TODO(#4659): Once we pass the feature level to API methods, this one // should encapsulate a switch at feature level 64. See its call sites. +// TODO(server-4.0): Simplify that away. export default ( auth: Auth, id: number, diff --git a/src/api/subscriptions/setSubscriptionProperty.js b/src/api/subscriptions/setSubscriptionProperty.js index b9e3639fdad..3539fced302 100644 --- a/src/api/subscriptions/setSubscriptionProperty.js +++ b/src/api/subscriptions/setSubscriptionProperty.js @@ -20,8 +20,8 @@ export default async ( // the server the older, confusingly named property // 'in_home_view' with the opposite value. // - // TODO: 'is_muted' is said to be new in Zulip 2.1, released - // 2019-12-12. Switch to sending that, once we can. + // TODO(server-2.1): 'is_muted' is said to be new in Zulip 2.1, + // released 2019-12-12. Switch to sending that, once we can. ...(property === 'is_muted' ? { property: 'in_home_view', value: !value } : { property, value }), diff --git a/src/api/transportTypes.js b/src/api/transportTypes.js index 72072f0b4b9..ccd7fee64d9 100644 --- a/src/api/transportTypes.js +++ b/src/api/transportTypes.js @@ -61,6 +61,8 @@ export type ApiResponseSuccess = $ReadOnly<{ * that value when connecting to old servers that don't provide an error * code. * + * TODO(server-1.7): Simplify this. + * * [1] Specifically at 1.7.0~2354 and ancestors, aka 9faa44af6^..709c3b50fc . * See: https://github.com/zulip/zulip/commit/709c3b50fc * diff --git a/src/caughtup/__tests__/caughtUpReducer-test.js b/src/caughtup/__tests__/caughtUpReducer-test.js index dd07f4ba47b..193a34bf952 100644 --- a/src/caughtup/__tests__/caughtUpReducer-test.js +++ b/src/caughtup/__tests__/caughtUpReducer-test.js @@ -4,14 +4,7 @@ import deepFreeze from 'deep-freeze'; import * as eg from '../../__tests__/lib/exampleData'; import caughtUpReducer from '../caughtUpReducer'; import { MESSAGE_FETCH_ERROR } from '../../actionConstants'; -import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../../anchor'; -import { - HOME_NARROW, - HOME_NARROW_STR, - ALL_PRIVATE_NARROW, - ALL_PRIVATE_NARROW_STR, - SEARCH_NARROW, -} from '../../utils/narrow'; +import { HOME_NARROW, HOME_NARROW_STR, SEARCH_NARROW } from '../../utils/narrow'; describe('caughtUpReducer', () => { describe('MESSAGE_FETCH_START', () => { @@ -85,24 +78,13 @@ describe('caughtUpReducer', () => { }); describe('MESSAGE_FETCH_COMPLETE', () => { - test('if messages received are less than requested then we are caught up', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }); + test('apply `foundNewest` and `foundOldest` when true', () => { + const initialState = deepFreeze({}); const action = deepFreeze({ ...eg.action.message_fetch_complete, - anchor: 1, - messages: [ - eg.streamMessage({ id: 1 }), - eg.streamMessage({ id: 2 }), - eg.streamMessage({ id: 3 }), - ], - numBefore: 5, - numAfter: 5, + foundNewest: true, + foundOldest: true, }); const expectedState = { @@ -118,16 +100,13 @@ describe('caughtUpReducer', () => { }); test('if fetched messages are from a search narrow, ignore them', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }); + const initialState = deepFreeze({}); const action = deepFreeze({ ...eg.action.message_fetch_complete, narrow: SEARCH_NARROW('some query'), + foundOldest: true, + foundNewest: true, }); const newState = caughtUpReducer(initialState, action); @@ -136,43 +115,7 @@ describe('caughtUpReducer', () => { }); }); - test('if messages received are requested amount we consider it not yet caught up', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: 3, - messages: [ - eg.streamMessage({ id: 1 }), - eg.streamMessage({ id: 2 }), - eg.streamMessage({ id: 3 }), - eg.streamMessage({ id: 4 }), - eg.streamMessage({ id: 5 }), - ], - numBefore: 2, - numAfter: 2, - foundNewest: undefined, - foundOldest: undefined, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('new results do not reset previous state', () => { + test('new false results do not reset previous true state', () => { const initialState = deepFreeze({ [HOME_NARROW_STR]: { older: true, @@ -182,266 +125,8 @@ describe('caughtUpReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_complete, - anchor: 3, - messages: [ - eg.streamMessage({ id: 1 }), - eg.streamMessage({ id: 2 }), - eg.streamMessage({ id: 3 }), - eg.streamMessage({ id: 4 }), - eg.streamMessage({ id: 5 }), - ], - numBefore: 2, - numAfter: 2, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: true, - newer: true, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('when at first unread and before and after messages are as many as requested not yet caught up', () => { - const initialState = deepFreeze({}); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: FIRST_UNREAD_ANCHOR, - messages: [ - eg.streamMessage({ id: 1, flags: ['read'] }), - eg.streamMessage({ id: 2, flags: ['read'] }), - eg.streamMessage({ id: 3, flags: ['read'] }), - eg.streamMessage({ id: 4, flags: [] }), - eg.streamMessage({ id: 5, flags: [] }), - eg.streamMessage({ id: 6, flags: [] }), - eg.streamMessage({ id: 7, flags: [] }), - ], - numBefore: 3, - numAfter: 3, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('when at first unread and before messages are less than requested older is caught up', () => { - const initialState = deepFreeze({}); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: FIRST_UNREAD_ANCHOR, - messages: [ - eg.streamMessage({ id: 1, flags: ['read'] }), - eg.streamMessage({ id: 2, flags: ['read'] }), - eg.streamMessage({ id: 3, flags: [] }), - eg.streamMessage({ id: 4, flags: [] }), - eg.streamMessage({ id: 5, flags: [] }), - eg.streamMessage({ id: 6, flags: [] }), - ], - numBefore: 3, - numAfter: 4, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: true, - newer: false, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('when at first unread and after messages are less than requested newer is caught up', () => { - const initialState = deepFreeze({}); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: FIRST_UNREAD_ANCHOR, - messages: [ - eg.streamMessage({ id: 1, flags: ['read'] }), - eg.streamMessage({ id: 2, flags: ['read'] }), - eg.streamMessage({ id: 3, flags: ['read'] }), - eg.streamMessage({ id: 4, flags: [] }), - eg.streamMessage({ id: 5, flags: [] }), - eg.streamMessage({ id: 6, flags: [] }), - ], - numBefore: 3, - numAfter: 4, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: false, - newer: true, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('when at first unread and both before and after messages are less than requested older and newer are caught up', () => { - const initialState = deepFreeze({}); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: FIRST_UNREAD_ANCHOR, - messages: [ - eg.streamMessage({ id: 1, flags: ['read'] }), - eg.streamMessage({ id: 2, flags: ['read'] }), - eg.streamMessage({ id: 3, flags: [] }), - eg.streamMessage({ id: 4, flags: [] }), - eg.streamMessage({ id: 5, flags: [] }), - ], - numBefore: 3, - numAfter: 4, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: true, - newer: true, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('if requesting latest messages always newer is caught up', () => { - const initialState = deepFreeze({}); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - narrow: ALL_PRIVATE_NARROW, - anchor: LAST_MESSAGE_ANCHOR, - messages: [ - eg.streamMessage({ id: 1 }), - eg.streamMessage({ id: 2 }), - eg.streamMessage({ id: 3 }), - eg.streamMessage({ id: 4 }), - eg.streamMessage({ id: 5 }), - ], - numBefore: 10, - numAfter: 0, - }); - - const expectedState = { - [ALL_PRIVATE_NARROW_STR]: { - older: true, - newer: true, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - describe('verify that server has send extra message before calculating adjustment', () => { - test('no adjustment is required if messages are less than or equal to requested', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: {}, - }); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: 6, - messages: [ - eg.streamMessage({ id: 1 }), - eg.streamMessage({ id: 2 }), - eg.streamMessage({ id: 3 }), - eg.streamMessage({ id: 4 }), - eg.streamMessage({ id: 5 }), - eg.streamMessage({ id: 6 }), - eg.streamMessage({ id: 7 }), - eg.streamMessage({ id: 8 }), - eg.streamMessage({ id: 9 }), - eg.streamMessage({ id: 10 }), - ], - numBefore: 5, - numAfter: 5, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - - test('dynamically determine adjustment whenever required', () => { - const initialState = deepFreeze({ - [HOME_NARROW_STR]: {}, - }); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: 5, - messages: [ - eg.streamMessage({ id: 0 }), - eg.streamMessage({ id: 1 }), - eg.streamMessage({ id: 2 }), - eg.streamMessage({ id: 3 }), - eg.streamMessage({ id: 4 }), - eg.streamMessage({ id: 5 }), - eg.streamMessage({ id: 6 }), - eg.streamMessage({ id: 7 }), - eg.streamMessage({ id: 8 }), - eg.streamMessage({ id: 9 }), - eg.streamMessage({ id: 10 }), - ], - numBefore: 5, - numAfter: 5, - }); - - const expectedState = { - [HOME_NARROW_STR]: { - older: false, - newer: false, - }, - }; - - const newState = caughtUpReducer(initialState, action); - - expect(newState).toEqual(expectedState); - }); - }); - - test('if `foundNewest` and `foundOldest` are provided use them', () => { - const initialState = deepFreeze({}); - - const action = deepFreeze({ - ...eg.action.message_fetch_complete, - anchor: 3, - messages: [], - numBefore: 2, - numAfter: 2, - foundNewest: true, - foundOldest: true, + foundOldest: false, + foundNewest: false, }); const expectedState = { diff --git a/src/caughtup/caughtUpReducer.js b/src/caughtup/caughtUpReducer.js index 833e11a268c..859adf210f9 100644 --- a/src/caughtup/caughtUpReducer.js +++ b/src/caughtup/caughtUpReducer.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import type { CaughtUp, CaughtUpState, Action } from '../types'; +import type { CaughtUpState, Action } from '../types'; import { REALM_INIT, LOGOUT, @@ -9,54 +9,12 @@ import { MESSAGE_FETCH_ERROR, MESSAGE_FETCH_COMPLETE, } from '../actionConstants'; -import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../anchor'; import { NULL_OBJECT } from '../nullObjects'; import { DEFAULT_CAUGHTUP } from './caughtUpSelectors'; import { isSearchNarrow, keyFromNarrow } from '../utils/narrow'; const initialState: CaughtUpState = NULL_OBJECT; -/** Try to infer the caught-up state, when the server didn't tell us. */ -const legacyInferCaughtUp = (prevCaughtUp: CaughtUp | void, action) => { - if (action.anchor === LAST_MESSAGE_ANCHOR) { - return { - older: action.numBefore > action.messages.length, - newer: true, - }; - } - - let anchorIdx = -1; - - if (action.anchor === FIRST_UNREAD_ANCHOR) { - anchorIdx = action.messages.findIndex(msg => !msg.flags || msg.flags.indexOf('read') === -1); - } else { - anchorIdx = action.messages.findIndex(msg => msg.id === action.anchor); - } - - if (anchorIdx === -1) { - anchorIdx = action.messages.length; - } - - const totalMessagesRequested = action.numBefore + action.numAfter; - // If we're requesting messages before the anchor, the server - // returns one less than we expect (to avoid duplicating the anchor) - // only do adjustment if messages are more than expected - const adjustment = - action.messages.length > totalMessagesRequested && action.numBefore > 0 - ? -(action.messages.length - totalMessagesRequested) - : 0; - - const caughtUpOlder = anchorIdx < action.numBefore; - const caughtUpNewer = action.messages.length - anchorIdx + adjustment < action.numAfter; - - const { older: prevOlder, newer: prevNewer } = prevCaughtUp || DEFAULT_CAUGHTUP; - - return { - older: prevOlder || caughtUpOlder, - newer: prevNewer || caughtUpNewer, - }; -}; - export default (state: CaughtUpState = initialState, action: Action): CaughtUpState => { switch (action.type) { case REALM_INIT: @@ -90,20 +48,13 @@ export default (state: CaughtUpState = initialState, action: Action): CaughtUpSt return state; } const key = keyFromNarrow(action.narrow); - let caughtUp = undefined; - if (action.foundNewest !== undefined && action.foundOldest !== undefined) { - /* This should always be the case for Zulip Server v1.8 or newer. */ - const { older: prevOlder, newer: prevNewer } = state[key] || DEFAULT_CAUGHTUP; - caughtUp = { - older: prevOlder || action.foundOldest, - newer: prevNewer || action.foundNewest, - }; - } else { - caughtUp = legacyInferCaughtUp(state[key], action); - } + const { older: prevOlder, newer: prevNewer } = state[key] || DEFAULT_CAUGHTUP; return { ...state, - [key]: caughtUp, + [key]: { + older: prevOlder || action.foundOldest, + newer: prevNewer || action.foundNewest, + }, }; } diff --git a/src/common/ServerCompatBanner.js b/src/common/ServerCompatBanner.js index 136a9726199..e520ce5a49f 100644 --- a/src/common/ServerCompatBanner.js +++ b/src/common/ServerCompatBanner.js @@ -13,6 +13,11 @@ import { openLinkWithUserPreference } from '../utils/openLink'; // The oldest version we currently support. Should match what we say at // https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html#compatibility-and-upgrading. const minSupportedVersion = '2.1.0'; +// Notes on known breakage at older versions: +// * Before 1.8, the server doesn't send found_newest / found_oldest on +// fetching messages, and so `state.caughtUp` will never have truthy +// values. This probably means annoying behavior in a message list, +// as we keep trying to fetch newer messages. type Props = $ReadOnly<{||}>; @@ -31,7 +36,7 @@ export default function ServerCompatBanner(props: Props): Node { let visible = false; let text = ''; - if (!zulipVersion || zulipVersion.isAtLeast(minSupportedVersion)) { + if (zulipVersion.isAtLeast(minSupportedVersion)) { // don't show } else if (hasDismissedServerCompatNotice) { // don't show diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index c7b09773559..fb38cbda619 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -128,6 +128,7 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null = // Before server feature level 13 (or if we didn't specify the // `bulk_message_deletion` client capability, which we do), this // event has `message_id` instead of `message_ids`. + // TODO(server-3.0): Simplify this. messageIds: event.message_ids ?? [event.message_id], }; @@ -286,6 +287,7 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null = // Servers with feature level 32+ send `op`. Servers will eventually // stop sending `operation`; see #4238. + // TODO(server-4.0): Simplify to just use `op`. op: event.op ?? event.operation, allMessages: state.messages, diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 6c4696036bb..7953ce88da2 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -62,8 +62,8 @@ const messageFetchComplete = (args: {| anchor: number, numBefore: number, numAfter: number, - foundNewest?: boolean, - foundOldest?: boolean, + foundNewest: boolean, + foundOldest: boolean, ownUserId: UserId, |}): Action => { const { diff --git a/src/notification/extract.js b/src/notification/extract.js index 772310b6e8c..cca255f0496 100644 --- a/src/notification/extract.js +++ b/src/notification/extract.js @@ -31,7 +31,8 @@ const asDict = (obj: JSONableInput | void): JSONableInputDict | void => { The original payload was merely `{ message_ids: [number] }`, but this has been expanded incrementally over the years. As of 2020-02, commit - 2.2-dev-775-g10e7e15088, the current form of APNs messages is as follows: + 2.2-dev-775-g10e7e15088 (released with 3.0, as "2.2" became 3.0), the + current form of APNs messages is as follows: ``` type StreamData = { @@ -82,6 +83,10 @@ const asDict = (obj: JSONableInput | void): JSONableInputDict | void => { ... which still didn't permit differentiating between multiple accounts on the same realm. This was only made possible by the addition of the `user_id` field, in 2.1-dev-540-g447a517e6f. + + TODO(server-1.8): Simplify this comment a lot. + TODO(server-1.9): Simplify further. + TODO(server-2.1): Simplify further. */ /** Local error type. */ diff --git a/src/notification/index.js b/src/notification/index.js index bc26a853786..44bc12c14fc 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -37,6 +37,7 @@ export const getAccountFromNotificationData = ( if (realm_uri == null) { // Old server, no realm info included. If needed to cater to 1.8.x // servers, could try to guess using serverHost; for now, don't. + // TODO(server-1.8): Simplify this comment. logging.warn('notification missing field: realm_uri'); return null; } diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index 2116debb523..37d040436c0 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -122,7 +122,7 @@ function getRecentConversationsModernImpl( const getServerIsOld: Selector = createSelector( getServerVersion, - version => !(version && version.isAtLeast(model.MIN_RECENTPMS_SERVER_VERSION)), + version => !version.isAtLeast(model.MIN_RECENTPMS_SERVER_VERSION), ); /** diff --git a/src/start/AuthScreen.js b/src/start/AuthScreen.js index d88216d37d5..aa869e3759d 100644 --- a/src/start/AuthScreen.js +++ b/src/start/AuthScreen.js @@ -85,6 +85,7 @@ const availableDirectMethods: AuthenticationMethodDetails[] = [ // Methods that are covered in external_authentication_methods by servers // which have that key (Zulip Server v2.1+). We refer to this array for // servers that don't. +// TODO(server-2.1): Simplify this away. const availableExternalMethods: AuthenticationMethodDetails[] = [ { name: 'google', diff --git a/src/start/__tests__/AuthScreen-test.js b/src/start/__tests__/AuthScreen-test.js index 283f8c608e9..42e30751795 100644 --- a/src/start/__tests__/AuthScreen-test.js +++ b/src/start/__tests__/AuthScreen-test.js @@ -51,6 +51,7 @@ describe('activeAuthentications: external_authentication_methods (server v2.1+ A }); }); +// TODO(server-2.1): Delete this (and the logic it tests.) describe('activeAuthentications: old server API', () => { test('empty auth methods object result in no available authentications', () => { const authenticationMethods = {}; diff --git a/src/streams/streamsActions.js b/src/streams/streamsActions.js index 54205eb53a9..e4275f0ce20 100644 --- a/src/streams/streamsActions.js +++ b/src/streams/streamsActions.js @@ -24,6 +24,7 @@ export const updateExistingStream = ( // https://github.com/zulip/zulip-mobile/pull/4748#issuecomment-852254404 // https://github.com/zulip/zulip-mobile/issues/4747#issuecomment-946362729 // TODO(#4659): Ideally this belongs inside `api.updateStream`. + // TODO(server-4.0): Simplify this (if it hasn't already moved.) getZulipFeatureLevel(state) >= 64 ? value : JSON.stringify(value); const auth = getAuth(state); diff --git a/src/users/usersActions.js b/src/users/usersActions.js index 39e679daae1..cc813afd6f6 100644 --- a/src/users/usersActions.js +++ b/src/users/usersActions.js @@ -7,7 +7,6 @@ import { PRESENCE_RESPONSE } from '../actionConstants'; import { getAuth, getServerVersion } from '../selectors'; import { isPmNarrow, userIdsOfPmNarrow } from '../utils/narrow'; import { getUserForId } from './userSelectors'; -import { ZulipVersion } from '../utils/zulipVersion'; export const reportPresence = (isActive: boolean): ThunkAction> => async ( dispatch, @@ -29,13 +28,12 @@ export const reportPresence = (isActive: boolean): ThunkAction> => // to refer to this account regardless of what the then-active account might be. const typingWorker = (state: PerAccountState) => { const auth: Auth = getAuth(state); - const serverVersion: ZulipVersion | null = getServerVersion(state); // User ID arrays are only supported in server versions >= 2.0.0-rc1 // (zulip/zulip@2f634f8c0). For versions before this, email arrays - // are used. If current server version is undetermined, user ID - // arrays are optimistically used. - const useEmailArrays = !!serverVersion && !serverVersion.isAtLeast('2.0.0-rc1'); + // are used. + // TODO(server-2.0): Simplify this away. + const useEmailArrays = !getServerVersion(state).isAtLeast('2.0.0-rc1'); const getRecipients = user_ids_array => { if (useEmailArrays) { diff --git a/src/utils/avatar.js b/src/utils/avatar.js index afed8cd48bc..29c5c6f121b 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -46,6 +46,9 @@ export class AvatarURL { // this case isn't thoroughly considered, but at worst, it means a // 404. We could plumb through the server version and // conditionalize on that. + // + // TODO(server-1.9): Simplify this comment. + // TODO(server-3.0): Simplify this comment. return FallbackAvatarURL.validateAndConstructInstance({ realm, userId }); } else if (rawAvatarUrl === null) { // If we announce `client_gravatar`, which we do, `rawAvatarUrl` diff --git a/src/webview/static/base.css b/src/webview/static/base.css index cfff2d826b0..49646ba032f 100644 --- a/src/webview/static/base.css +++ b/src/webview/static/base.css @@ -565,7 +565,8 @@ code, pre { } pre code { /* Starting with Zulip Server 3.0, code blocks have `pre > code`. - Undo the stuff we have for `code` that isn't meant to apply there. */ + Undo the stuff we have for `code` that isn't meant to apply there. + TODO(server-3.0): Simplify this. */ font-size: inherit; white-space: inherit; padding: 0;