api [nfc]: Start relying on server 1.9+

src/api/apiErrors.js

@@ -146,7 +146,14 @@ export const interpretApiResponse = (httpStatus: number, data: mixed): mixed =>
     // Client error.  We should have a Zulip API error blob.
 
     if (typeof data === 'object' && data !== null) {
+      // For errors `code` should always be present, but there have been
+      // bugs where it was missing; in particular, on rate-limit errors
+      // until Zulip 4.0: https://zulip.com/api/rest-error-handling .
+      // Fall back to `BAD_REQUEST`, the same thing the server uses when
+      // nobody's made it more specific.
+      // TODO(server-4.0): Drop this fallback.
       const { result, msg, code = 'BAD_REQUEST' } = data;
+
       if (result === 'error' && typeof msg === 'string' && typeof code === 'string') {
         // Hooray, we have a well-formed Zulip API error blob.  Use that.
         throw new ApiError(httpStatus, { ...data, result, msg, code });

src/api/initialDataTypes.js

@@ -335,11 +335,8 @@ export type InitialDataRealmUser = $ReadOnly<{|
 |}>;
 
 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<UserGroup>,
+  // New in Zulip 1.8.
+  realm_user_groups: $ReadOnlyArray<UserGroup>,
 |}>;
 
 export type InitialDataRecentPmConversations = $ReadOnly<{|

src/api/modelTypes.js

@@ -108,9 +108,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,
+  date_joined: string,
 
   // is_active doesn't appear in `/register` responses -- instead,
   // users where is_active is true go in `realm_users`, and where false
@@ -132,9 +130,9 @@ export type User = $ReadOnly<{|
   bot_owner?: string,
 
   // The ? is for future-proofing. Greg explains in 2020-02, at
-  // https://github.com/zulip/zulip-mobile/pull/3789#discussion_r378554698, that
-  // both human and bot Users will likely end up having a missing timezone
-  // instead of an empty string.
+  // https://github.com/zulip/zulip-mobile/pull/3789#discussion_r378554698 ,
+  // that both human and bot Users will likely end up having a missing
+  // timezone instead of an empty string.
   timezone?: string,
 
   /**
@@ -178,22 +176,17 @@ 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,
-
+  date_joined: string,
   email: string,
   full_name: string,
   is_admin: boolean,
   is_bot: true,
   user_id: UserId,
 
-  // The timezone field has been included since commit 58ee3fa8c (in 1.9.0). Tim
-  // mentions in 2020-02, at
-  // https://github.com/zulip/zulip-mobile/pull/3789#issuecomment-581218576,
-  // that, as of the time of writing, it is always '' for bots, but it may be
-  // omitted later to reduce payload sizes. So, we're future-proofing this field
-  // by making it optional. See comment on the same field in User.
+  // The ? is for future-proofing.  For bots it's always '':
+  //   https://github.com/zulip/zulip-mobile/pull/3789#issuecomment-581218576
+  // so a future version may omit it to reduce payload sizes.  See comment
+  // on the same field in User.
   timezone?: string,
 |}>;
 

src/api/notificationTypes.js

@@ -0,0 +1,76 @@
+// @flow strict-local
+
+import { type UserId } from './idTypes';
+
+// This format is as of 2020-02, commit 3.0~3347.
+type BaseData = {|
+  /** The realm URL, same as in the `/server_settings` response. */
+  +realm_uri: string,
+
+  // The server and realm_id are an obsolete substitute for realm_uri.
+  -server: string, // settings.EXTERNAL_HOST
+  -realm_id: number, // server-internal realm identifier
+
+  /**
+   * The user this notification is addressed to; our self-user.
+   *
+   * (This lets us distinguish different accounts in the same realm.)
+   */
+  // added 2.1-dev-540-g447a517e6f, release 2.1.0+
+  // TODO(server-2.1): Simplify comment.
+  +user_id: UserId,
+
+  // The rest of the data is about the Zulip message we're being notified
+  // about.
+
+  +message_ids: [number], // single-element tuple!
+
+  +sender_id: UserId,
+  +sender_email: string,
+
+  // (... And see StreamData and PmData below.)
+|};
+
+/** For a notification about a stream message. */
+type StreamData = {|
+  ...BaseData,
+  +recipient_type: 'stream',
+
+  /**
+   * The name of the message's stream.
+   *
+   * Sadly no stream ID: https://github.com/zulip/zulip/issues/18067
+   */
+  +stream: string,
+
+  +topic: string,
+|};
+
+/** For a notification about a PM. */
+type PmData = {|
+  ...BaseData,
+  +recipient_type: 'private',
+
+  /**
+   * The recipient user IDs, if this is a group PM; absent for 1:1 PMs.
+   *
+   * The user IDs are comma-separated ASCII decimal.
+   *
+   * (Does it include the self-user?  Are they sorted?  Unknown.)
+   */
+  +pm_users?: string,
+|};
+
+/** An APNs message sent by the Zulip server. */
+export type ApnsPayload = {|
+  /** Our own data. */
+  +zulip: StreamData | PmData,
+
+  /**
+   * Various Apple-specified fields.
+   *
+   * Upstream docs here:
+   *   https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification
+   */
+  +aps: { ... },
+|};

src/api/transportTypes.js

@@ -52,36 +52,33 @@ export type ApiResponseSuccess = $ReadOnly<{
 }>;
 
 /**
- * A list of current error codes can be found at:
- *   https://github.com/zulip/zulip/blob/main/zerver/lib/exceptions.py
- *
- * Unfortunately, the `code` property is a relatively late addition to the
- * Zulip API, introduced for version 1.7.0. [1]  The modern default, when no
- * other code has been defined, is 'BAD_REQUEST'; we therefore synthesize
- * that value when connecting to old servers that don't provide an error
- * code.
+ * An identifier-like string identifying a Zulip API error.
  *
- * TODO(server-1.7): Simplify this.
+ * See API docs on error handling:
+ *   https://zulip.com/api/rest-error-handling
  *
- * [1] Specifically at 1.7.0~2354 and ancestors, aka 9faa44af6^..709c3b50fc .
- *     See: https://github.com/zulip/zulip/commit/709c3b50fc
+ * (And at present, 2022-01, those are rather buggy.  So see also:
+ *   https://chat.zulip.org/#narrow/stream/378-api-design/topic/error.20docs/near/1308989
+ * )
  *
- * It's tempting to make ApiErrorCode an enumerated type, save for the dual
- * problem: when connecting to _newer_ Zulip servers, we may see values of
- * `code` not known to this version of the client! In particular, new error
- * codes are occasionally assigned to existing classes of error which previously
- * returned 'BAD_REQUEST'.
- *
- * (Note that "newer" here doesn't necessarily mean "newer than this client",
- * but "newer than the last time someone updated the error code list from the
- * server". We have no mechanism in place to share the set of error codes --
- * and, given the facts above, it's not clear that the existence of such a
- * mechanism would be helpful anyway.)
+ * A list of current error codes can be found at:
+ *   https://github.com/zulip/zulip/blob/main/zerver/lib/exceptions.py
  *
- * Though unstable in the general case, `code` is still useful.  It _is_
- * guaranteed to be stable for certain known classes of errors.
- * Nonetheless: let its user beware.
+ * Note that most possible values of `code` are not documented in the API,
+ * and may change in future versions.  This is especially likely when an
+ * error gives the generic `code: 'BAD_REQUEST'`.  When writing logic to
+ * rely on a `code` value, it's best to make sure it gets written into the
+ * API documentation.
  */
+// It's tempting to make ApiErrorCode an enumerated type, but: when
+// connecting to newer Zulip servers, we may see values of `code` not known
+// to this version of the client!
+//
+// (Note that "newer" here doesn't necessarily mean "newer than this client",
+// but "newer than the last time someone updated the error code list from the
+// server". We have no mechanism in place to share the set of error codes --
+// and, given the facts above, it's not clear that the existence of such a
+// mechanism would be helpful anyway.)
 export type ApiErrorCode = string;
 
 /**

src/notification/extract.js

@@ -17,79 +17,6 @@ const asDict = (obj: JSONableInput | void): JSONableInputDict | void => {
   return obj;
 };
 
-/*
-    The Zulip APNs message format, as far back as 2013-03-23 [0], has always
-    been some subtype of the following:
-
-        type Data = { zulip: { ... } }
-
-    (Comments in `zerver/lib/push_notifications.py` may appear to indicate
-    otherwise, but these refer only to the format of inter-server messages for
-    the Zulip-hosted push-notification bouncer service. Messages sent over APNs
-    itself have always had a `zulip` field.)
-
-    [0] GitHub commit 410ee44eb6e40bac4099d1851d949533026fe4b3.
-
-    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 (released with 3.0, as "2.2" became 3.0), the
-    current form of APNs messages is as follows:
-
-    ```
-    type StreamData = {
-        // added 1.7.0-1351-g98943a8333, release 1.8.0+
-        recipient_type: 'stream',
-        stream: string,
-        topic: string,
-    };
-
-    type PmData = {
-        // added 1.7.0-1351-g98943a8333, release 1.8.0+
-        recipient_type: 'private',
-
-        // added 1.7.0-2360-g693a9a5e70, release 1.8.0+
-        // present only on group PMs
-        pm_users?: string,  // CSV of int (user ids)
-    };
-
-    type Data = { zulip: {
-        // present since antiquity
-        message_ids: [number],  // single-element tuple!
-
-        // added 1.7.0-1351-g98943a8333, release 1.8.0+
-        sender_email: string,
-        sender_id: UserId,
-        server: string,     // settings.EXTERNAL_HOST
-        realm_id: number,   // server-internal realm identifier
-
-        // added 1.8.0-2150-g5f8d193bb7, release 1.9.0+
-        realm_uri: string,  // as in `/server_settings` response
-
-        // added 2.1-dev-540-g447a517e6f, release 2.1.0+
-        user_id: UserId,    // recipient id
-
-        ...(StreamData | PmData),
-    } };
-    ```
-
-    Note that prior to 1.7.0-1351-g98943a8333, we only received the
-    `message_ids` field. Messages of this form are not useful.
-
-    The pair `(server, realm_id)`, added in the same commit, uniquely identifies
-    a Zulip realm, and was originally intended to permit the client to associate
-    a notification with its associated realm. Unfortunately, there is no way to
-    get either of these from a server via the API, so this would not be possible
-    until the addition of `realm_uri` in 1.8.0-2150-g5f8d193bb7...
-
-    ... 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. */
 class ApnsMsgValidationError extends logging.ExtendableError {
   extras: JSONable;
@@ -108,6 +35,10 @@ class ApnsMsgValidationError extends logging.ExtendableError {
 // @throws An ApnsMsgValidationError on unexpected failure.
 //
 export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
+  //
+  // For the format this parses, see `ApnsPayload` in src/api/notificationTypes.js .
+  //
+
   /** Helper function: fail. */
   const err = (style: string) =>
     new ApnsMsgValidationError(`Received ${style} APNs notification`, {
@@ -138,7 +69,7 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
     }
   })();
 
-  // Always present; see historical type definition, above.
+  // Always present; see `ApnsPayload`.
   const zulip: JSONableInputDict | void = asDict(data.zulip);
   if (!zulip) {
     throw err('alien');
@@ -158,7 +89,11 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
     return undefined;
   }
 
+  //
   // At this point we can begin trying to construct our `Notification`.
+  //
+  // For the format this code is parsing, see `ApnsPayload` in
+  // src/api/notificationTypes.js .
 
   const { recipient_type } = zulip;
   if (recipient_type === undefined) {

src/notification/index.js

@@ -36,9 +36,8 @@ export const getAccountFromNotificationData = (
 ): number | null => {
   const { realm_uri, user_id } = data;
   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.
+    // Old server, no realm info included.  This field appeared in
+    // Zulip 1.8, so we don't support these servers anyway.
     logging.warn('notification missing field: realm_uri');
     return null;
   }

src/utils/avatar.js

@@ -34,20 +34,12 @@ export class AvatarURL {
   |}): AvatarURL {
     const { rawAvatarUrl, userId, email, realm } = args;
     if (rawAvatarUrl === undefined) {
-      // New in Zulip 3.0, feature level 18, the field may be missing
-      // on user objects in the register response, at the server's
-      // discretion, if we announce the
-      // `user_avatar_url_field_optional` client capability, which we
-      // do. See the note about `user_avatar_url_field_optional` at
-      // https://zulip.com/api/register-queue.
+      // New in Zulip 3.0, feature level 18, the field may be missing on
+      // user objects in the register response, at the server's discretion,
+      // if we announce the `user_avatar_url_field_optional` client
+      // capability, which we do.  See `user_avatar_url_field_optional` at:
+      //   https://zulip.com/api/register-queue
       //
-      // It will also be absent on cross-realm bots from servers prior
-      // to 58ee3fa8c (1.9.0). The effect of using FallbackAvatarURL for
-      // 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) {