From 3e4326afda996015b90fc923cfff75f70b1204ff Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulipchat.com>
Date: Sat, 28 Dec 2019 16:41:20 +0000
Subject: [PATCH] refactor: Extract email_for_user_settings.

We want to be able to unit test this value,
since it's conditional on several factors:

    - am I an admin?
    - can non-admins view emails?
    - do we have delivery_email for the user?

I'm mocking show_email in the tests, since the
show_email code is in `settings_org` and
kind of hard to unit test.  It's not impossible,
but it's too much for this commit.  (Either
we need to extract it out to a nice file or
deal with mocking jQuery.  That module is
mostly data-oriented, so it would be nice
to have something like `settings_config` that
is actually pure data.)
---
 frontend_tests/node_tests/people.js  | 27 +++++++++++++++++++++++++++
 static/js/people.js                  | 12 ++++++++++++
 static/js/settings_users.js          |  6 +++---
 static/templates/admin_user_list.hbs |  8 ++------
 4 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/frontend_tests/node_tests/people.js b/frontend_tests/node_tests/people.js
index c219dee31a..da222af97b 100644
--- a/frontend_tests/node_tests/people.js
+++ b/frontend_tests/node_tests/people.js
@@ -5,6 +5,7 @@ set_global('blueslip', global.make_zblueslip({
     error: false, // We check for errors in people_errors.js
 }));
 set_global('page_params', {});
+set_global('settings_org', {});
 set_global('md5', function (s) {
     return 'md5-' + s;
 });
@@ -22,6 +23,7 @@ const me = {
 
 const isaac = {
     email: 'isaac@example.com',
+    delivery_email: 'isaac-delivery@example.com',
     user_id: 32,
     full_name: 'Isaac Newton',
 };
@@ -884,6 +886,31 @@ run_test('matches_user_settings_search', () => {
     assert.equal(match({full_name: 'Joe Frederick'}, 're'), true);
 });
 
+run_test('email_for_user_settings', () => {
+    const email = people.email_for_user_settings;
+
+    settings_org.show_email = () => {
+        return false;
+    };
+
+    assert.equal(email(isaac), undefined);
+
+    settings_org.show_email = () => {
+        return true;
+    };
+
+    page_params.is_admin = true;
+    assert.equal(email(isaac), isaac.delivery_email);
+
+    // Fall back to email if delivery_email is not there.
+    assert.equal(
+        email({email: 'foo@example.com'}),
+        'foo@example.com');
+
+    page_params.is_admin = false;
+    assert.equal(email(isaac), isaac.email);
+});
+
 run_test('emails_strings_to_user_ids_array', function () {
     const steven = {
         email: 'steven@example.com',
diff --git a/static/js/people.js b/static/js/people.js
index 783a102358..16c18a4b76 100644
--- a/static/js/people.js
+++ b/static/js/people.js
@@ -995,6 +995,18 @@ exports.matches_user_settings_search = function (person, value) {
     );
 };
 
+exports.email_for_user_settings = function (person) {
+    if (!settings_org.show_email()) {
+        return;
+    }
+
+    if (page_params.is_admin && person.delivery_email) {
+        return person.delivery_email;
+    }
+
+    return person.email;
+};
+
 exports.maybe_incr_recipient_count = function (message) {
     if (message.type !== 'private') {
         return;
diff --git a/static/js/settings_users.js b/static/js/settings_users.js
index acc798e83e..b1fe57694c 100644
--- a/static/js/settings_users.js
+++ b/static/js/settings_users.js
@@ -167,7 +167,7 @@ function populate_users(realm_people_data) {
             return render_admin_user_list({
                 can_modify: page_params.is_admin,
                 // It's always safe to show the fake email addresses for bot users
-                show_email: true,
+                display_email: item.email,
                 user: item,
             });
         },
@@ -212,7 +212,7 @@ function populate_users(realm_people_data) {
             const $row = $(render_admin_user_list({
                 can_modify: page_params.is_admin,
                 is_current_user: people.is_my_user_id(item.user_id),
-                show_email: settings_org.show_email(),
+                display_email: people.email_for_user_settings(item),
                 user: item,
             }));
             $row.find(".last_active").append(get_rendered_last_activity(item));
@@ -248,7 +248,7 @@ function populate_users(realm_people_data) {
         modifier: function (item) {
             return render_admin_user_list({
                 user: item,
-                show_email: settings_org.show_email(),
+                display_email: people.email_for_user_settings(item),
                 can_modify: page_params.is_admin,
             });
         },
diff --git a/static/templates/admin_user_list.hbs b/static/templates/admin_user_list.hbs
index d22a2d1460..ce97f07e69 100644
--- a/static/templates/admin_user_list.hbs
+++ b/static/templates/admin_user_list.hbs
@@ -4,13 +4,9 @@
         <span class="user_name">{{full_name}}</span>
         <i class="fa fa-ban deactivated-user-icon" title="{{t 'User is deactivated' }}" {{#if is_active}}style="display: none;"{{/if}}></i>
     </td>
-    {{#if ../show_email}}
+    {{#if ../display_email}}
     <td>
-        {{#if ../can_modify}}
-        <span class="email">{{delivery_email}}</span>
-        {{else}}
-        <span class="email">{{email}}</span>
-        {{/if}}
+        <span class="email">{{../display_email}}</span>
     </td>
     {{/if}}
     {{#unless is_bot}}