profile: Display user email

[sm-sayedi]

Fixes: #291

[Khader-1]

Thanks @sm-sayedi! LGTM!
Now moving on to mentor review with @hackerkid.

[chrisbobbe]

Thanks! The issue links to zulip-mobile's implementation: https://github.com/zulip/zulip-mobile/blob/8133bf2b8/src/users/userSelectors.js#L318

Here's that function:

/**
 * The given user's real email address, if known, for displaying in the UI.
 *
 * Null if our user isn't able to see this user's real email address.
 */
export function getDisplayEmailForUser(realm: RealmState, user: UserOrBot): string | null {
  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
    return user.email;
  } else {
    return null;
  }
}

What's the user-facing consequence of not including logic like the check on realm.emailAddressVisibility?

[sm-sayedi]

Quoting the documentation for delivery_email from https://zulip.com/api/register-queue:

delivery_email: string | null

The user's real email address. This value will be null if you cannot access user's real email address. For bot users, this field is always set to the real email of the bot, because bot users always have email_address_visibility set to everyone.

Changes: Prior to Zulip 7.0 (feature level 163), this field was present only when email_address_visibility was restricted and you had access to the user's real email. As of this feature level, this field is always present, including the case when email_address_visibility is set to everyone (and therefore not restricted).

So I think that check was only necessary before feature level 163.

[chrisbobbe]

Looks like FL 163 was released in Zulip Server 7: https://zulip.com/api/changelog#changes-in-zulip-70

We aim to support all the way back to Zulip Server 4: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#editing-api-types

but we may bump that to 5 soon: #268

Even when we bump our lower bound to 5, though, we'll still have users running 5 and 6. So the FL 163 threshold is one we should pay attention to.

[sm-sayedi]

Thanks for the review @chrisbobbe. Changes pushed. PTAL.

[chrisbobbe]

Thanks @sm-sayedi!

Small comments below, and also let's isolate the API-types change into the api: … commit, and add PerAccountStore.emailAddressVisibility in a separate commit.

[chrisbobbe]

Let's include a TODO(server-7) for removing the <163 case. That way, when we decide to bump our minimum-supported threshold to 7, this spot will naturally show up in a quick search for backwards-compatibility code that can be removed at that time.

[chrisbobbe]

Let's add a // TODO(server-7): remove on this.

[chrisbobbe]

I'm having trouble parsing this summary line. How about:

Suggested change

	/// The realm-level policy for which a user can see other user's real email address.
	/// The realm-level policy, on pre-FL 163 servers, for visibility of real email addresses.

[chrisbobbe]

Suggested change

	/// **Note:** This field is removed in Zulip 7.0 (FL 163) and replaced with:
	/// * https://zulip.com/api/update-settings#parameter-email_address_visibility
	/// * https://zulip.com/api/update-realm-user-settings-defaults#parameter-email_address_visibility
	/// This field is removed in Zulip 7.0 (FL 163) and replaced with a user-level
	/// setting:
	/// * https://zulip.com/api/update-settings#parameter-email_address_visibility
	/// * https://zulip.com/api/update-realm-user-settings-defaults#parameter-email_address_visibility

[chrisbobbe]

selfUser is formatted here as though it's an identifier in the function's code, but it isn't actually. So let's just say "the self-user".

(Here and below.)

[sm-sayedi]

Thanks @chrisbobbe for the review! Comments addressed. Please have a look.

[chrisbobbe]

Thanks! One nit below, and I'll mark this for Greg's review.

[chrisbobbe]

Suggested change

	// TODO(server-7, FL >= 163): remove
	// TODO(server-7): remove; use user-level setting

(The form we've settled on is TODO(server-x); it doesn't need the feature level. All servers that are 7+ will have FL >= 163.)

[gnprice]

Thanks @sm-sayedi, and thanks @chrisbobbe for the previous reviews!

Generally this looks good. A few small comments below, and then this will be ready to merge.

[gnprice]

This isn't quite right because in this revision it doesn't get updated when the setting changes. This value is an example of a realm setting:

• https://zulip.com/api/get-events#realm-update_dict
• Track realm settings #668

But I don't want dealing with that to block this feature, or indeed to block launching the app. It's rare this setting will change, and the consequences of having an out-of-date value for a while are small. So let's just mark it with a TODO comment:

Suggested change

	final EmailAddressVisibility? emailAddressVisibility; // TODO(server-7): remove
	final EmailAddressVisibility? emailAddressVisibility; // TODO(#668) update this realm setting

(The TODO-server comment can be omitted because it's effectively covered by the one on InitialSnapshot.emailAddressVisibility — when we go to remove that, we'll necessarily notice we need to remove this at the same time.)

[gnprice]

Suggested change

	// TODO(server-7): remove; use user-level setting
	String? _getDisplayEmailFor(User user, {required PerAccountStore store}) {
	if (store.account.zulipFeatureLevel >= 163) {
	String? _getDisplayEmailFor(User user, {required PerAccountStore store}) {
	if (store.account.zulipFeatureLevel >= 163) { // TODO(server-7)

Having the comment right at the condition is helpful for seeing that the condition is covered by a TODO-server comment.

The instruction "use user-level setting" isn't right for this spot — we won't be looking at the user's individual emailAddressVisibility setting, but rather just looking at their deliveryEmail. After all, that's what this code already does for modern servers. If there were something else we should be doing in the future, then for modern servers we should be doing that in the first place.

[gnprice]

Suggested change

	/// The given user's real email address, if known, for displaying in the UI.
	///
	/// Returns null if self-user isn't able to see [user]'s real email address.
	///
	/// **Note:** Starting from Zulip version 7.0 (FL 163), there's a change in
	/// the API about how self-user can access [user]'s real email address.
	/// Search for "delivery_email" in https://zulip.com/api/register-queue.
	/// The given user's real email address, if known, for displaying in the UI.
	///
	/// Returns null if self-user isn't able to see [user]'s real email address.

The caller of this method doesn't need to know about that API change, right? It's something this method itself takes care of.

In that case it doesn't belong in a doc comment.

The reader for whom this information is relevant is one trying to understand the details of the method's implementation. That means the place this information could belong is in an implementation comment — // instead of ///, and inside the method's body. I think the code already covers the fact of the API change; but the comment in the modern-server case could benefit from the 'Search for "delivery_email" …' line.

[sm-sayedi]

Thanks @gnprice for the review! Changes pushed!

[gnprice]

Thanks for the revision! Looks good — merging.