From 6d4c6ee1548efc430d5390edca0f1e862c837f32 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Mon, 2 Dec 2024 10:11:23 +1100 Subject: [PATCH] UX: admins users page follows admin ux guideline (#29873) Conversion of `/admin/users` page to follow admin UX guidelines. In addition, add the username to the title on the user admin page. --- .../admin/addon/routes/admin-user-index.js | 4 + .../admin/addon/templates/users-list-show.hbs | 43 +++---- .../admin/addon/templates/users-list.hbs | 84 +++++++------ .../tests/acceptance/admin-users-list-test.js | 14 +-- .../stylesheets/common/admin/admin_base.scss | 14 +++ config/locales/client.en.yml | 1 + spec/system/admin_user_spec.rb | 5 + spec/system/admin_users_list_spec.rb | 117 +++++++++++++----- spec/system/page_objects/pages/admin_users.rb | 40 +++++- 9 files changed, 225 insertions(+), 97 deletions(-) diff --git a/app/assets/javascripts/admin/addon/routes/admin-user-index.js b/app/assets/javascripts/admin/addon/routes/admin-user-index.js index 14a4b67ad93..c04ea3238ff 100644 --- a/app/assets/javascripts/admin/addon/routes/admin-user-index.js +++ b/app/assets/javascripts/admin/addon/routes/admin-user-index.js @@ -6,6 +6,10 @@ export default class AdminUserIndexRoute extends DiscourseRoute { return this.modelFor("adminUser"); } + titleToken() { + return this.currentModel.username; + } + afterModel(model) { if (this.currentUser.admin) { return Group.findAll().then((groups) => { diff --git a/app/assets/javascripts/admin/addon/templates/users-list-show.hbs b/app/assets/javascripts/admin/addon/templates/users-list-show.hbs index 91e7d161d3f..288c5a2369e 100644 --- a/app/assets/javascripts/admin/addon/templates/users-list-show.hbs +++ b/app/assets/javascripts/admin/addon/templates/users-list-show.hbs @@ -1,26 +1,27 @@ + + <:actions as |actions|> + {{#if this.canCheckEmails}} + {{#if this.showEmails}} + + {{else}} + + {{/if}} + {{/if}} + + + -
-

{{this.title}}

- {{#if this.canCheckEmails}} - {{#if this.showEmails}} - - {{else}} - - {{/if}} - {{/if}} -
-
-
+
{{#if this.users}} - + + + +
+
-
+
{{outlet}}
\ No newline at end of file diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-users-list-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-users-list-test.js index d7a6534a277..1fa73fb3d91 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-users-list-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/admin-users-list-test.js @@ -18,7 +18,7 @@ acceptance("Admin - Users List", function (needs) { test("searching users with no matches", async function (assert) { await visit("/admin/users/list/active"); - await fillIn(".admin-users-list__controls .username input", "doesntexist"); + await fillIn(".admin-users-list__search input", "doesntexist"); assert.dom(".users-list-container").hasText(i18n("search.no_results")); }); @@ -50,13 +50,13 @@ acceptance("Admin - Users List", function (needs) { assert.dom(".users-list .user").exists(); - await click(".show-emails"); + await click(".admin-users__subheader-show-emails"); assert .dom(".users-list .user:nth-child(1) .email") .hasText("eviltrout@example.com", "shows the emails"); - await click(".hide-emails"); + await click(".admin-users__subheader-hide-emails"); assert .dom(".users-list .user:nth-child(1) .email .directory-table__value") @@ -71,28 +71,28 @@ acceptance("Admin - Users List", function (needs) { await visit("/admin/users/list/active"); - assert.dom(".admin-title h2").hasText(activeTitle); + assert.dom(".admin-page-subheader__title").hasText(activeTitle); assert .dom(".users-list .user:nth-child(1) .username") .includesText(activeUser); await click('a[href="/admin/users/list/new"]'); - assert.dom(".admin-title h2").hasText(suspectTitle); + assert.dom(".admin-page-subheader__title").hasText(suspectTitle); assert .dom(".users-list .user:nth-child(1) .username") .includesText(suspectUser); await click(".users-list .sortable:nth-child(4)"); - assert.dom(".admin-title h2").hasText(suspectTitle); + assert.dom(".admin-page-subheader__title").hasText(suspectTitle); assert .dom(".users-list .user:nth-child(1) .username") .includesText(suspectUser); await click('a[href="/admin/users/list/active"]'); - assert.dom(".admin-title h2").hasText(activeTitle); + assert.dom(".admin-page-subheader__title").hasText(activeTitle); assert .dom(".users-list .user:nth-child(1) .username") .includesText(activeUser); diff --git a/app/assets/stylesheets/common/admin/admin_base.scss b/app/assets/stylesheets/common/admin/admin_base.scss index 0cc2fe89582..41ef3fdbfcf 100644 --- a/app/assets/stylesheets/common/admin/admin_base.scss +++ b/app/assets/stylesheets/common/admin/admin_base.scss @@ -423,6 +423,20 @@ $mobile-breakpoint: 700px; color: var(--primary-medium); } } +.admin-users-list { + &__search { + @media screen and (max-width: 500px) { + width: 100%; + } + input { + min-width: 15em; + @media screen and (max-width: 500px) { + box-sizing: border-box; + width: 100%; + } + } + } +} .ip-lookup { position: relative; diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5accd159f38..368817a5fd9 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6642,6 +6642,7 @@ en: users: title: "Users" + description: "View and manage users." create: "Add Admin User" last_emailed: "Last Emailed" not_found: "Sorry, that username doesn't exist in our system." diff --git a/spec/system/admin_user_spec.rb b/spec/system/admin_user_spec.rb index c24530b44f8..683e18232d2 100644 --- a/spec/system/admin_user_spec.rb +++ b/spec/system/admin_user_spec.rb @@ -44,6 +44,11 @@ describe "Admin User Page", type: :system do expect(admin_user_page).to have_silence_button end + it "displays username in the title" do + expect(page).to have_css(".display-row.username") + expect(page.title).to eq("#{user.username} - Admin - Discourse") + end + describe "the suspend user modal" do it "displays the list of users who share the same IP but are not mods or admins" do admin_user_page.click_suspend_button diff --git a/spec/system/admin_users_list_spec.rb b/spec/system/admin_users_list_spec.rb index e92634a597a..84351655add 100644 --- a/spec/system/admin_users_list_spec.rb +++ b/spec/system/admin_users_list_spec.rb @@ -3,7 +3,9 @@ describe "Admin Users Page", type: :system do fab!(:current_user) { Fabricate(:admin) } fab!(:another_admin) { Fabricate(:admin) } - fab!(:users) { Fabricate.times(3, :user) } + fab!(:user_1) { Fabricate(:user) } + fab!(:user_2) { Fabricate(:user) } + fab!(:user_3) { Fabricate(:user) } let(:admin_users_page) { PageObjects::Pages::AdminUsers.new } @@ -19,7 +21,7 @@ describe "Admin Users Page", type: :system do expect(admin_users_page.user_row(current_user.id).bulk_select_checkbox.disabled?).to eq(true) expect(admin_users_page.user_row(another_admin.id).bulk_select_checkbox.disabled?).to eq(true) - expect(admin_users_page.user_row(users[0].id).bulk_select_checkbox.disabled?).to eq(false) + expect(admin_users_page.user_row(user_1.id).bulk_select_checkbox.disabled?).to eq(false) admin_users_page.user_row(another_admin.id).bulk_select_checkbox.hover expect(PageObjects::Components::Tooltips.new("bulk-delete-unavailable-reason")).to be_present( @@ -30,25 +32,25 @@ describe "Admin Users Page", type: :system do it "has a button that toggles the bulk select checkboxes" do admin_users_page.visit - expect(admin_users_page).to have_users(users.map(&:id)) + expect(admin_users_page).to have_users([user_1.id, user_2.id, user_3.id]) - expect(admin_users_page.user_row(users[0].id)).to have_no_bulk_select_checkbox - expect(admin_users_page.user_row(users[1].id)).to have_no_bulk_select_checkbox - expect(admin_users_page.user_row(users[2].id)).to have_no_bulk_select_checkbox + expect(admin_users_page.user_row(user_1.id)).to have_no_bulk_select_checkbox + expect(admin_users_page.user_row(user_2.id)).to have_no_bulk_select_checkbox + expect(admin_users_page.user_row(user_3.id)).to have_no_bulk_select_checkbox admin_users_page.bulk_select_button.click - expect(admin_users_page.user_row(users[0].id)).to have_bulk_select_checkbox - expect(admin_users_page.user_row(users[1].id)).to have_bulk_select_checkbox - expect(admin_users_page.user_row(users[2].id)).to have_bulk_select_checkbox + expect(admin_users_page.user_row(user_1.id)).to have_bulk_select_checkbox + expect(admin_users_page.user_row(user_2.id)).to have_bulk_select_checkbox + expect(admin_users_page.user_row(user_3.id)).to have_bulk_select_checkbox expect(admin_users_page).to have_no_bulk_actions_dropdown - admin_users_page.user_row(users[0].id).bulk_select_checkbox.click + admin_users_page.user_row(user_1.id).bulk_select_checkbox.click expect(admin_users_page).to have_bulk_actions_dropdown - admin_users_page.user_row(users[1].id).bulk_select_checkbox.click + admin_users_page.user_row(user_2.id).bulk_select_checkbox.click admin_users_page.bulk_actions_dropdown.expand admin_users_page.bulk_actions_dropdown.option(".bulk-delete").click @@ -64,38 +66,37 @@ describe "Admin Users Page", type: :system do confirmation_modal.confirm_button.click expect(confirmation_modal).to have_successful_log_entry_for_user( - user: users[0], + user: user_1, position: 1, total: 2, ) expect(confirmation_modal).to have_successful_log_entry_for_user( - user: users[1], + user: user_2, position: 2, total: 2, ) expect(confirmation_modal).to have_no_error_log_entries confirmation_modal.close - deleted_ids = users[0..1].map(&:id) - expect(admin_users_page).to have_no_users(deleted_ids) - expect(User.where(id: deleted_ids).count).to eq(0) + expect(admin_users_page).to have_no_users([user_1.id, user_2.id]) + expect(User.where(id: [user_1.id, user_2.id]).count).to eq(0) end it "remembers selected users when the user list refreshes due to search" do admin_users_page.visit admin_users_page.bulk_select_button.click - admin_users_page.search_input.fill_in(with: users[0].username) - admin_users_page.user_row(users[0].id).bulk_select_checkbox.click + admin_users_page.search_input.fill_in(with: user_1.username) + admin_users_page.user_row(user_1.id).bulk_select_checkbox.click - admin_users_page.search_input.fill_in(with: users[1].username) - admin_users_page.user_row(users[1].id).bulk_select_checkbox.click + admin_users_page.search_input.fill_in(with: user_2.username) + admin_users_page.user_row(user_2.id).bulk_select_checkbox.click admin_users_page.search_input.fill_in(with: "") - expect(admin_users_page).to have_users(users.map(&:id)) - expect(admin_users_page.user_row(users[0].id).bulk_select_checkbox).to be_checked - expect(admin_users_page.user_row(users[1].id).bulk_select_checkbox).to be_checked - expect(admin_users_page.user_row(users[2].id).bulk_select_checkbox).not_to be_checked + expect(admin_users_page).to have_users([user_1.id, user_2.id, user_3.id]) + expect(admin_users_page.user_row(user_1.id).bulk_select_checkbox).to be_checked + expect(admin_users_page.user_row(user_2.id).bulk_select_checkbox).to be_checked + expect(admin_users_page.user_row(user_3.id).bulk_select_checkbox).not_to be_checked admin_users_page.bulk_actions_dropdown.expand admin_users_page.bulk_actions_dropdown.option(".bulk-delete").click @@ -104,39 +105,91 @@ describe "Admin Users Page", type: :system do confirmation_modal.fill_in_confirmation_phase(user_count: 2) confirmation_modal.confirm_button.click expect(confirmation_modal).to have_successful_log_entry_for_user( - user: users[0], + user: user_1, position: 1, total: 2, ) expect(confirmation_modal).to have_successful_log_entry_for_user( - user: users[1], + user: user_2, position: 2, total: 2, ) confirmation_modal.close - deleted_ids = users[0..1].map(&:id) - expect(admin_users_page).to have_no_users(deleted_ids) - expect(User.where(id: deleted_ids).count).to eq(0) + expect(admin_users_page).to have_no_users([user_1.id, user_2.id]) + expect(User.where(id: [user_1.id, user_2.id]).count).to eq(0) end it "displays an error message if bulk delete fails" do admin_users_page.visit admin_users_page.bulk_select_button.click - admin_users_page.user_row(users[0].id).bulk_select_checkbox.click + admin_users_page.user_row(user_1.id).bulk_select_checkbox.click admin_users_page.bulk_actions_dropdown.expand admin_users_page.bulk_actions_dropdown.option(".bulk-delete").click confirmation_modal.fill_in_confirmation_phase(user_count: 1) - users[0].update!(admin: true) + user_1.update!(admin: true) confirmation_modal.confirm_button.click expect(confirmation_modal).to have_error_log_entry( I18n.t("js.generic_error_with_reason", error: I18n.t("user.cannot_bulk_delete")), ) confirmation_modal.close - expect(admin_users_page).to have_users([users[0].id]) + expect(admin_users_page).to have_users([user_1.id]) + end + end + + context "when visiting an admin's page" do + it "shows list of active users" do + admin_users_page.visit + expect(admin_users_page).to have_active_tab("active") + expect(page).to have_css(".directory-table__cell.username") + expect(admin_users_page).to have_users( + [current_user.id, another_admin.id, user_1.id, user_2.id, user_3.id], + ) + end + + it "shows list of suspended users" do + admin_users_page.visit + admin_users_page.click_tab("suspended") + expect(admin_users_page).to have_active_tab("suspended") + expect(admin_users_page).to have_none_users + end + + it "shows list of silenced users" do + admin_users_page.visit + user_1.update!(silenced_till: 1.day.from_now) + admin_users_page.click_tab("silenced") + expect(admin_users_page).to have_active_tab("silenced") + expect(page).to have_css(".users-list.silenced") + expect(admin_users_page).to have_users([user_1.id]) + end + + it "shows emails" do + admin_users_page.visit + expect(admin_users_page).to have_no_emails + admin_users_page.click_show_emails + expect(admin_users_page).to have_emails + end + + it "redirects to groups page" do + admin_users_page.visit + admin_users_page.click_tab("groups") + expect(page).to have_current_path("/g") + end + + it "redirect to invites page" do + admin_users_page.visit + admin_users_page.click_send_invites + expect(page).to have_current_path("/u/#{current_user.username}/invited/pending") + end + + it "allows to export users" do + admin_users_page.visit + admin_users_page.click_export + expect(page).to have_css(".dialog-body") + expect(page).to have_content(I18n.t("admin_js.admin.export_csv.success")) end end end diff --git a/spec/system/page_objects/pages/admin_users.rb b/spec/system/page_objects/pages/admin_users.rb index 9603d2dc5c9..08096213850 100644 --- a/spec/system/page_objects/pages/admin_users.rb +++ b/spec/system/page_objects/pages/admin_users.rb @@ -32,7 +32,7 @@ module PageObjects end def search_input - find(".admin-users-list__controls .username input") + find(".admin-users-list__search input") end def user_row(id) @@ -62,6 +62,44 @@ module PageObjects def has_no_bulk_actions_dropdown? has_no_css?(".bulk-select-admin-users-dropdown-trigger") end + + def has_usernames?(usernames) + expect(all(".directory-table__cell.username").map(&:text)).to eq(usernames) + end + + def has_none_users? + has_content?(I18n.t("js.search.no_results")) + end + + def click_tab(tab) + has_css?(".admin-users-tabs__#{tab}") + find(".admin-users-tabs__#{tab}").click + end + + def has_active_tab?(tab) + has_css?(".admin-users-tabs__#{tab} .active") + has_no_css?(".loading-container .visible") + end + + def has_no_emails? + has_no_css?(".directory-table__column-header--email") + end + + def has_emails? + has_css?(".directory-table__column-header--email") + end + + def click_show_emails + find(".admin-users__subheader-show-emails").click + end + + def click_send_invites + find(".admin-users__header-send-invites").click + end + + def click_export + find(".admin-users__header-export-users").click + end end end end