From 14209227056b9cf00b855634177c2a414fe981ea Mon Sep 17 00:00:00 2001 From: Pragati Agrawal Date: Thu, 23 Jan 2020 11:31:34 +0530 Subject: [PATCH] settings_org: Use actual value for value attribute of create-stream-policy. For few settings like `waiting_period_threshold` it makes sense to have the "value" attribute of option to have a value other than the actual setting value because multiple settings are depending upon this dropdown, so handling them in JS code makes more sense. But for many settings (which has integer values), we have followed a wrong trend over the time of representing every new dropdown with human-readable values and manually handling them in JS Code, where it makes more sense to use actual setting value. The result of which is code has become less concise, sensible and less likely to be mistaken. --- frontend_tests/casper_tests/10-admin.js | 6 +- frontend_tests/node_tests/settings_org.js | 122 +++++++++++++++++- static/js/admin.js | 1 + static/js/settings_org.js | 68 +++++++--- .../organization_permissions_admin.hbs | 6 +- 5 files changed, 170 insertions(+), 33 deletions(-) diff --git a/frontend_tests/casper_tests/10-admin.js b/frontend_tests/casper_tests/10-admin.js index 151bfb1724..9ea961b6fd 100644 --- a/frontend_tests/casper_tests/10-admin.js +++ b/frontend_tests/casper_tests/10-admin.js @@ -115,7 +115,7 @@ casper.then(function () { casper.test.info("Test setting create streams policy to 'admins only'."); casper.waitUntilVisible("#id_realm_create_stream_policy", function () { casper.evaluate(function () { - $("#id_realm_create_stream_policy").val("by_admins_only").change(); + $("#id_realm_create_stream_policy").val(settings_org.create_stream_policy_values.by_admins_only.code).change(); }); submit_permissions_change(); }); @@ -134,7 +134,7 @@ casper.then(function () { casper.test.info("Test setting create streams policy to 'members and admins'."); casper.waitUntilVisible("#id_realm_create_stream_policy", function () { casper.evaluate(function () { - $("#id_realm_create_stream_policy").val("by_members").change(); + $("#id_realm_create_stream_policy").val(settings_org.create_stream_policy_values.by_members.code).change(); }); submit_permissions_change(); }); @@ -153,7 +153,7 @@ casper.then(function () { casper.test.info("Test setting create streams policy to 'waiting period."); casper.waitUntilVisible("#id_realm_create_stream_policy", function () { casper.evaluate(function () { - $("#id_realm_create_stream_policy").val("by_full_members").change(); + $("#id_realm_create_stream_policy").val(settings_org.create_stream_policy_values.by_full_members.code).change(); }); submit_permissions_change(); }); diff --git a/frontend_tests/node_tests/settings_org.js b/frontend_tests/node_tests/settings_org.js index 33b7769817..564b90a5be 100644 --- a/frontend_tests/node_tests/settings_org.js +++ b/frontend_tests/node_tests/settings_org.js @@ -197,6 +197,7 @@ function test_submit_settings_form(submit_form) { realm_default_language: '"es"', realm_default_twenty_four_hour_time: 'false', realm_invite_to_stream_policy: 2, + realm_create_stream_policy: 1, }); global.patch_builtin('setTimeout', func => func()); @@ -223,10 +224,17 @@ function test_submit_settings_form(submit_form) { save_button.replace = () => { return `${subsection}`; }; - $("#id_realm_create_stream_policy").val("by_members"); + $("#id_realm_invite_to_stream_policy").val("by_members"); $("#id_realm_waiting_period_threshold").val(10); + const create_stream_policy_elem = $("#id_realm_create_stream_policy"); + create_stream_policy_elem.val('2'); + create_stream_policy_elem.attr("id", 'id_realm_create_stream_policy'); + create_stream_policy_elem.data = () => { + return "integer"; + }; + const add_emoji_by_admins_only_elem = $("#id_realm_add_emoji_by_admins_only"); add_emoji_by_admins_only_elem.val("by_anyone"); add_emoji_by_admins_only_elem.attr("id", 'id_realm_add_emoji_by_admins_only'); @@ -250,6 +258,7 @@ function test_submit_settings_form(submit_form) { bot_creation_policy_elem, email_address_visibility_elem, add_emoji_by_admins_only_elem, + create_stream_policy_elem, ]); patched = false; @@ -261,7 +270,7 @@ function test_submit_settings_form(submit_form) { invite_to_stream_policy: 1, email_address_visibility: '1', add_emoji_by_admins_only: false, - create_stream_policy: 1, + create_stream_policy: '2', waiting_period_threshold: 10, }; assert.deepEqual(data, expected_value); @@ -478,7 +487,8 @@ function test_sync_realm_settings() { page_params.realm_create_stream_policy = 3; settings_org.sync_realm_settings('create_stream_policy'); - assert.equal($("#id_realm_create_stream_policy").val(), "by_full_members"); + assert.equal($("#id_realm_create_stream_policy").val(), + settings_org.create_stream_policy_values.by_full_members.code); } { @@ -493,7 +503,8 @@ function test_sync_realm_settings() { page_params.realm_create_stream_policy = 1; settings_org.sync_realm_settings('create_stream_policy'); - assert.equal($("#id_realm_create_stream_policy").val(), "by_members"); + assert.equal($("#id_realm_create_stream_policy").val(), + settings_org.create_stream_policy_values.by_members.code); } { @@ -508,7 +519,8 @@ function test_sync_realm_settings() { page_params.realm_create_stream_policy = 2; settings_org.sync_realm_settings('create_stream_policy'); - assert.equal($("#id_realm_create_stream_policy").val(), "by_admins_only"); + assert.equal($("#id_realm_create_stream_policy").val(), + settings_org.create_stream_policy_values.by_admins_only.code); } { @@ -810,6 +822,106 @@ run_test('set_up', () => { settings_org.render_notifications_stream_ui = stub_render_notifications_stream_ui; }); +run_test('test get_organization_settings_options', () => { + const sorted_option_values = settings_org.get_organization_settings_options(); + const sorted_create_stream_policy_values = sorted_option_values.create_stream_policy_values; + const expected_create_stream_policy_values = [ + { + key: 'by_admins_only', + order: 1, + code: 2, + description: i18n.t("Admins"), + }, + { + key: 'by_full_members', + order: 2, + code: 3, + description: i18n.t("Admins and full members"), + }, + { + key: 'by_members', + order: 3, + code: 1, + description: i18n.t("Admins and members"), + }, + ]; + assert.deepEqual(sorted_create_stream_policy_values, expected_create_stream_policy_values); +}); + +run_test('test get_sorted_options_list', () => { + const option_values_1 = { + by_admins_only: { + order: 3, + code: 2, + description: i18n.t("Admins"), + }, + by_members: { + order: 2, + code: 1, + description: i18n.t("Admins and members"), + }, + by_full_members: { + order: 1, + code: 3, + description: i18n.t("Admins and full members"), + }, + }; + let expected_option_values = [ + { + key: 'by_full_members', + order: 1, + code: 3, + description: i18n.t("Admins and full members"), + }, + { + key: 'by_members', + order: 2, + code: 1, + description: i18n.t("Admins and members"), + }, + { + key: 'by_admins_only', + order: 3, + code: 2, + description: i18n.t("Admins"), + }, + ]; + assert.deepEqual(settings_org.get_sorted_options_list(option_values_1), expected_option_values); + + const option_values_2 = { + by_admins_only: { + code: 1, + description: i18n.t("Admins"), + }, + by_members: { + code: 2, + description: i18n.t("Admins and members"), + }, + by_full_members: { + code: 3, + description: i18n.t("Admins and full members"), + }, + }; + expected_option_values = [ + { + key: 'by_admins_only', + code: 1, + description: i18n.t("Admins"), + }, + { + key: 'by_full_members', + code: 3, + description: i18n.t("Admins and full members"), + }, + { + key: 'by_members', + code: 2, + description: i18n.t("Admins and members"), + }, + ]; + assert.deepEqual(settings_org.get_sorted_options_list(option_values_2), expected_option_values); +}); + run_test('misc', () => { page_params.is_admin = false; diff --git a/static/js/admin.js b/static/js/admin.js index 9f308ff1fe..f99c15c78d 100644 --- a/static/js/admin.js +++ b/static/js/admin.js @@ -83,6 +83,7 @@ exports.build_page = function () { options.msg_delete_limit_dropdown_values = settings_org.msg_delete_limit_dropdown_values; options.bot_creation_policy_values = settings_bots.bot_creation_policy_values; options.email_address_visibility_values = settings_org.email_address_visibility_values; + _.extend(options, settings_org.get_organization_settings_options()); if (options.realm_logo_source !== 'D' && options.realm_night_logo_source === 'D') { // If no night mode logo is specified but a day mode one is, diff --git a/static/js/settings_org.js b/static/js/settings_org.js index 57adb2f19b..6d435fda24 100644 --- a/static/js/settings_org.js +++ b/static/js/settings_org.js @@ -52,6 +52,53 @@ exports.email_address_visibility_values = { }, }; +exports.create_stream_policy_values = { + by_admins_only: { + order: 1, + code: 2, + description: i18n.t("Admins"), + }, + by_full_members: { + order: 2, + code: 3, + description: i18n.t("Admins and full members"), + }, + by_members: { + order: 3, + code: 1, + description: i18n.t("Admins and members"), + }, +}; + +exports.get_sorted_options_list = function (option_values_object) { + const options_list = Object.keys(option_values_object).map((key) => { + return _.extend(option_values_object[key], {key: key}); + }); + let comparator = (x, y) => x.order - y.order; + if (!options_list[0].order) { + comparator = (x, y) => { + const key_x = x.key.toUpperCase(); + const key_y = y.key.toUpperCase(); + if (key_x < key_y) { + return -1; + } + if (key_x > key_y) { + return 1; + } + return 0; + }; + } + options_list.sort(comparator); + return options_list; +}; + +exports.get_organization_settings_options = () => { + const options = {}; + options.create_stream_policy_values = exports.get_sorted_options_list( + exports.create_stream_policy_values); + return options; +}; + exports.show_email = function () { // TODO: Extend this when we add support for admins_and_members above. if (page_params.realm_email_address_visibility === @@ -93,18 +140,6 @@ function get_property_value(property_name) { return "custom_days"; } - if (property_name === 'realm_create_stream_policy') { - if (page_params.realm_create_stream_policy === 2) { - return "by_admins_only"; - } - if (page_params.realm_create_stream_policy === 1) { - return "by_members"; - } - if (page_params.realm_create_stream_policy === 3) { - return "by_full_members"; - } - } - if (property_name === 'realm_invite_to_stream_policy') { if (page_params.realm_invite_to_stream_policy === 1) { return "by_members"; @@ -818,7 +853,6 @@ exports.build_page = function () { JSON.stringify(parseInt(new_message_retention_days, 10)) : null; } else if (subsection === 'other_permissions') { const waiting_period_threshold = $("#id_realm_waiting_period_setting").val(); - const create_stream_policy = $("#id_realm_create_stream_policy").val(); const invite_to_stream_policy = $("#id_realm_invite_to_stream_policy").val(); const user_group_edit_policy = $("#id_realm_user_group_edit_policy").val(); const private_message_policy = $("#id_realm_private_message_policy").val(); @@ -830,14 +864,6 @@ exports.build_page = function () { data.add_emoji_by_admins_only = false; } - if (create_stream_policy === "by_admins_only") { - data.create_stream_policy = 2; - } else if (create_stream_policy === "by_members") { - data.create_stream_policy = 1; - } else if (create_stream_policy === "by_full_members") { - data.create_stream_policy = 3; - } - if (invite_to_stream_policy === "by_admins_only") { data.invite_to_stream_policy = 2; } else if (invite_to_stream_policy === "by_members") { diff --git a/static/templates/settings/organization_permissions_admin.hbs b/static/templates/settings/organization_permissions_admin.hbs index 51a10a44ae..36a0bc1351 100644 --- a/static/templates/settings/organization_permissions_admin.hbs +++ b/static/templates/settings/organization_permissions_admin.hbs @@ -89,10 +89,8 @@
- + {{> dropdown_options_widget option_values=create_stream_policy_values}}