Skip to content

Commit

Permalink
api: Intentionally handle removal of realm_email_address_visibility
Browse files Browse the repository at this point in the history
I can't find a message in `#api design` on CZO that's explicitly
about removing realm_email_address_visibility from the register
response. The `#api design` discussion about transitioning that from
an org- to a user-level setting is here:
  https://chat.zulip.org/#narrow/stream/378-api-design/topic/email.20address.20visibility/near/1296131
That discussion includes mentions of the user-object fields that are
about email addresses, and it includes mentions of the per-user
visibility setting, but I don't see anything about removing the
realm setting from the /register response.

In retrospect, its removal there would have been an obvious thing to
anticipate and handle in our types. Oh well, better late than never.

Luckily I don't think there was a user-facing bug here. At FL 163+,
the value that we've been typing as the enum EmailAddressVisibility
will actually be `undefined`, but that's OK because on 163+ we don't
end up doing anything with it.
  • Loading branch information
chrisbobbe committed Dec 22, 2023
1 parent 7c7369d commit 042c4cf
Show file tree
Hide file tree
Showing 9 changed files with 16 additions and 15 deletions.
3 changes: 1 addition & 2 deletions src/__tests__/lib/exampleData.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { randString, randInt } from '../../utils/misc';
import { makeUserId } from '../../api/idTypes';
import type { InitialData } from '../../api/apiTypes';
import { EventTypes, type UpdateMessageEvent } from '../../api/eventTypes';
import { CreateWebPublicStreamPolicy, EmailAddressVisibility } from '../../api/permissionsTypes';
import { CreateWebPublicStreamPolicy } from '../../api/permissionsTypes';
import type {
AccountSwitchAction,
LoginSuccessAction,
Expand Down Expand Up @@ -784,7 +784,6 @@ export const action = Object.freeze({
realm_digest_weekday: 2,
realm_disallow_disposable_email_addresses: true,
realm_edit_topic_policy: 3,
realm_email_address_visibility: EmailAddressVisibility.Admins,
realm_email_auth_enabled: true,
realm_email_changes_disabled: true,
realm_emails_restricted_to_domains: false,
Expand Down
4 changes: 3 additions & 1 deletion src/api/initialDataTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,9 @@ export type InitialDataRealm = $ReadOnly<{|
// TODO(server-5.0): Added in feat. 75, replacing realm_allow_community_topic_editing
realm_edit_topic_policy?: number,

realm_email_address_visibility: EmailAddressVisibility,
// TODO(server-7.0): Removed in feat. 163
realm_email_address_visibility?: EmailAddressVisibility,

realm_email_auth_enabled: boolean,
realm_email_changes_disabled: boolean,
realm_emails_restricted_to_domains: boolean,
Expand Down
2 changes: 1 addition & 1 deletion src/api/realmDataTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export type RealmDataForUpdate = $ReadOnly<{
InitialDataRealm['realm_disallow_disposable_email_addresses'],
edit_topic_policy:
InitialDataRealm['realm_edit_topic_policy'],
email_address_visibility:
email_address_visibility: // removed in feat. 163
InitialDataRealm['realm_email_address_visibility'],
email_changes_disabled:
InitialDataRealm['realm_email_changes_disabled'],
Expand Down
2 changes: 1 addition & 1 deletion src/realm/__tests__/realmReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('realmReducer', () => {
waitingPeriodThreshold: action.data.realm_waiting_period_threshold,
allowEditHistory: action.data.realm_allow_edit_history,
enableReadReceipts: action.data.realm_enable_read_receipts,
emailAddressVisibility: action.data.realm_email_address_visibility,
emailAddressVisibility: null,

//
// InitialDataRealmUser
Expand Down
5 changes: 2 additions & 3 deletions src/realm/realmReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import type {
import {
CreatePublicOrPrivateStreamPolicy,
CreateWebPublicStreamPolicy,
EmailAddressVisibility,
} from '../api/permissionsTypes';
import { EventTypes } from '../api/eventTypes';
import {
Expand Down Expand Up @@ -53,7 +52,7 @@ const initialState = {
waitingPeriodThreshold: 90,
allowEditHistory: false,
enableReadReceipts: false,
emailAddressVisibility: EmailAddressVisibility.Admins,
emailAddressVisibility: null,

//
// InitialDataRealmUser
Expand Down Expand Up @@ -165,7 +164,7 @@ export default (
waitingPeriodThreshold: action.data.realm_waiting_period_threshold,
allowEditHistory: action.data.realm_allow_edit_history,
enableReadReceipts: action.data.realm_enable_read_receipts ?? false,
emailAddressVisibility: action.data.realm_email_address_visibility,
emailAddressVisibility: action.data.realm_email_address_visibility ?? null,

//
// InitialDataRealmUser
Expand Down
2 changes: 1 addition & 1 deletion src/reduxTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ export type RealmState = {|
+waitingPeriodThreshold: number,
+allowEditHistory: boolean,
+enableReadReceipts: boolean,
+emailAddressVisibility: EmailAddressVisibility,
+emailAddressVisibility: EmailAddressVisibility | null,

//
// InitialDataRealmUser
Expand Down
6 changes: 3 additions & 3 deletions src/storage/__tests__/migrations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ describe('migrations', () => {
// What `base` becomes after all migrations.
const endBase = {
...base52,
migrations: { version: 60 },
migrations: { version: 61 },
};

for (const [desc, before, after] of [
Expand All @@ -128,9 +128,9 @@ describe('migrations', () => {
// redundant with this one, because none of the migration steps notice
// whether any properties outside `storeKeys` are present or not.
[
'check dropCache at 60',
'check dropCache at 61',
// Just before the `dropCache`, plus a `cacheKeys` property, plus junk.
{ ...base52, migrations: { version: 59 }, mute: [], nonsense: [1, 2, 3] },
{ ...base52, migrations: { version: 60 }, mute: [], nonsense: [1, 2, 3] },
// Should wind up with the same result as without the extra properties.
endBase,
],
Expand Down
3 changes: 3 additions & 0 deletions src/storage/migrations.js
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,9 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
// Discard invalid enum values from `state.mute`.
'60': dropCache,

// Fix emailAddressVisibility accidentally being undefined/dropped
'61': dropCache,

// TIP: When adding a migration, consider just using `dropCache`.
// (See its jsdoc for guidance on when that's the right answer.)
};
Expand Down
4 changes: 1 addition & 3 deletions src/users/userSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,7 @@ export function getDisplayEmailForUser(realm: RealmState, user: UserOrBot): stri
if (user.delivery_email !== undefined) {
return user.delivery_email;
} else if (realm.emailAddressVisibility === EmailAddressVisibility.Everyone) {
// On future servers, we expect this case will never happen: we'll always include
// a delivery_email when you have access, including when the visibility is Everyone
// https://github.com/zulip/zulip-mobile/pull/5515#discussion_r997731727
// TODO(server-7.0): Not reached on FL 163+, where delivery_email is always present.
return user.email;
} else {
return null;
Expand Down

0 comments on commit 042c4cf

Please sign in to comment.