From 7b437c9401a52cdda06402ebd9fff0a08b67ab2a Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Thu, 23 May 2024 19:18:25 +0800 Subject: [PATCH] FEATURE: Implement new required options in admin user fields UI (#27079) We're planning to implement a feature that allows adding required fields for existing users. This PR does some preparatory refactoring to make that possible. There should be no changes to existing behaviour. Just a small update to the admin UI. --- .../components/admin-user-field-item.hbs | 58 +++++++++++++------ .../addon/components/admin-user-field-item.js | 7 +-- .../stylesheets/common/admin/customize.scss | 16 +++++ .../admin/user_fields_controller.rb | 3 +- app/models/user_field.rb | 10 ++++ app/serializers/user_field_serializer.rb | 5 ++ config/locales/client.en.yml | 8 +++ ...20060901_add_requirement_to_user_fields.rb | 17 ++++++ spec/fabricators/user_field_fabricator.rb | 2 +- spec/models/user_field_spec.rb | 26 +++++++++ .../admin/user_fields_controller_spec.rb | 11 +++- spec/requests/users_controller_spec.rb | 16 ++--- spec/system/admin_user_fields_spec.rb | 28 +++++++++ spec/system/page_objects/modals/signup.rb | 4 ++ .../page_objects/pages/admin_user_fields.rb | 26 +++++++++ spec/system/signup_spec.rb | 34 +++++++++++ 16 files changed, 234 insertions(+), 37 deletions(-) create mode 100644 db/migrate/20240520060901_add_requirement_to_user_fields.rb create mode 100644 spec/system/admin_user_fields_spec.rb create mode 100644 spec/system/page_objects/pages/admin_user_fields.rb diff --git a/app/assets/javascripts/admin/addon/components/admin-user-field-item.hbs b/app/assets/javascripts/admin/addon/components/admin-user-field-item.hbs index b87f7949e50..a6bdfc2183e 100644 --- a/app/assets/javascripts/admin/addon/components/admin-user-field-item.hbs +++ b/app/assets/javascripts/admin/addon/components/admin-user-field-item.hbs @@ -30,29 +30,51 @@ {{/if}} - - - {{i18n "admin.user_fields.editable.title"}} + + + + - - - {{i18n "admin.user_fields.required.title"}} - + + - - - {{i18n "admin.user_fields.show_on_profile.title"}} - + - - - {{i18n "admin.user_fields.show_on_user_card.title"}} - + - - - {{i18n "admin.user_fields.searchable.title"}} + { field_type == "confirm" } has_many :user_field_options, dependent: :destroy @@ -15,10 +18,16 @@ class UserField < ActiveRecord::Base scope :public_fields, -> { where(show_on_profile: true).or(where(show_on_user_card: true)) } + enum :requirement, { optional: 0, for_all_users: 1, on_signup: 2 }.freeze + def self.max_length 2048 end + def required? + !optional? + end + def queue_index_search Jobs.enqueue(:index_user_fields_for_search, user_field_id: self.id) end @@ -50,4 +59,5 @@ end # external_name :string # external_type :string # searchable :boolean default(FALSE), not null +# requirement :integer default("optional"), not null # diff --git a/app/serializers/user_field_serializer.rb b/app/serializers/user_field_serializer.rb index d897a13e443..ca162ce6b9f 100644 --- a/app/serializers/user_field_serializer.rb +++ b/app/serializers/user_field_serializer.rb @@ -7,12 +7,17 @@ class UserFieldSerializer < ApplicationSerializer :field_type, :editable, :required, + :requirement, :show_on_profile, :show_on_user_card, :searchable, :position, :options + def required + object.required? + end + def options object.user_field_options.pluck(:value) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index db4461ba88c..19528a08eca 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6586,6 +6586,7 @@ en: name: "Field Name" type: "Field Type" description: "Field Description" + preferences: "Preferences" save: "Save" edit: "Edit" delete: "Delete" @@ -6596,6 +6597,13 @@ en: title: "Required at signup" enabled: "required" disabled: "not required" + requirement: + title: "Field Requirement" + optional: + title: "Optional" + on_signup: + title: "On signup" + description: "When new users sign up, they must fill out this field. Existing users are unaffected." editable: title: "Editable after signup" enabled: "editable" diff --git a/db/migrate/20240520060901_add_requirement_to_user_fields.rb b/db/migrate/20240520060901_add_requirement_to_user_fields.rb new file mode 100644 index 00000000000..dea339ff564 --- /dev/null +++ b/db/migrate/20240520060901_add_requirement_to_user_fields.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddRequirementToUserFields < ActiveRecord::Migration[7.0] + def up + add_column :user_fields, :requirement, :integer, null: false, default: 0 + + execute <<~SQL + UPDATE user_fields + SET requirement = + (CASE WHEN required IS NOT TRUE THEN 0 ELSE 2 END) + SQL + end + + def down + remove_column :user_fields, :requirement + end +end diff --git a/spec/fabricators/user_field_fabricator.rb b/spec/fabricators/user_field_fabricator.rb index 8534e80657c..354f9d23b1b 100644 --- a/spec/fabricators/user_field_fabricator.rb +++ b/spec/fabricators/user_field_fabricator.rb @@ -5,5 +5,5 @@ Fabricator(:user_field) do description "user field description" field_type "text" editable true - required true + requirement "on_signup" end diff --git a/spec/models/user_field_spec.rb b/spec/models/user_field_spec.rb index 9f6a5f80e97..c4d3568ae45 100644 --- a/spec/models/user_field_spec.rb +++ b/spec/models/user_field_spec.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true RSpec.describe UserField do + it do + is_expected.to define_enum_for(:requirement).with_values(%w[optional for_all_users on_signup]) + end + describe "doesn't validate presence of name if field type is 'confirm'" do subject(:confirm_field) { described_class.new(field_type: "confirm") } @@ -40,4 +44,26 @@ RSpec.describe UserField do job_enqueued?(job: Jobs::IndexUserFieldsForSearch, args: { user_field_id: user_field.id }), ).to eq(true) end + + describe "#required?" do + let(:user_field) { Fabricate(:user_field, requirement:) } + + context "when requirement is optional" do + let(:requirement) { "optional" } + + it { expect(user_field).not_to be_required } + end + + context "when requirement is for all users" do + let(:requirement) { "for_all_users" } + + it { expect(user_field).to be_required } + end + + context "when requirement is on signup" do + let(:requirement) { "on_signup" } + + it { expect(user_field).to be_required } + end + end end diff --git a/spec/requests/admin/user_fields_controller_spec.rb b/spec/requests/admin/user_fields_controller_spec.rb index ff1f2a80ab3..7ae56e25bb7 100644 --- a/spec/requests/admin/user_fields_controller_spec.rb +++ b/spec/requests/admin/user_fields_controller_spec.rb @@ -17,6 +17,7 @@ RSpec.describe Admin::UserFieldsController do name: "hello", description: "hello desc", field_type: "text", + requirement: "on_signup", }, } @@ -33,6 +34,7 @@ RSpec.describe Admin::UserFieldsController do description: "hello desc", field_type: "dropdown", options: %w[a b c], + requirement: "on_signup", }, } @@ -162,13 +164,16 @@ RSpec.describe Admin::UserFieldsController do name: "fraggle", field_type: "confirm", description: "muppet", + requirement: "optional", }, } expect(response.status).to eq(200) - user_field.reload - expect(user_field.name).to eq("fraggle") - expect(user_field.field_type).to eq("confirm") + expect(user_field.reload).to have_attributes( + name: "fraggle", + field_type: "confirm", + required?: false, + ) end it "updates the user field options" do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 8547ee5ac12..49e186ab392 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1432,7 +1432,7 @@ RSpec.describe UsersController do context "with custom fields" do fab!(:user_field) fab!(:another_field) { Fabricate(:user_field) } - fab!(:optional_field) { Fabricate(:user_field, required: false) } + fab!(:optional_field) { Fabricate(:user_field, requirement: "optional") } context "without a value for the fields" do let(:create_params) do @@ -1492,7 +1492,7 @@ RSpec.describe UsersController do it "value can't be nil or empty if the field is required" do put update_user_url, params: { user_fields: { field_id => valid_options } } - user_field.update!(required: true) + user_field.for_all_users! expect do put update_user_url, params: { user_fields: { field_id => nil } } @@ -1506,7 +1506,7 @@ RSpec.describe UsersController do it "value can nil or empty if the field is not required" do put update_user_url, params: { user_fields: { field_id => valid_options } } - user_field.update!(required: false) + user_field.optional! expect do put update_user_url, params: { user_fields: { field_id => nil } } @@ -1547,7 +1547,7 @@ RSpec.describe UsersController do it "value can't be nil if the field is required" do put update_user_url, params: { user_fields: { field_id => valid_options.first } } - user_field.update!(required: true) + user_field.for_all_users! expect do put update_user_url, params: { user_fields: { field_id => nil } } @@ -1557,7 +1557,7 @@ RSpec.describe UsersController do it "value can be set to nil if the field is not required" do put update_user_url, params: { user_fields: { field_id => valid_options.last } } - user_field.update!(required: false) + user_field.optional! expect do put update_user_url, params: { user_fields: { field_id => nil } } @@ -1614,7 +1614,7 @@ RSpec.describe UsersController do end context "with only optional custom fields" do - fab!(:user_field) { Fabricate(:user_field, required: false) } + fab!(:user_field) { Fabricate(:user_field, requirement: "optional") } context "without values for the fields" do let(:create_params) do @@ -2378,8 +2378,8 @@ RSpec.describe UsersController do context "with user fields" do context "with an editable field" do - fab!(:user_field) - fab!(:optional_field) { Fabricate(:user_field, required: false) } + fab!(:user_field) { Fabricate(:user_field, requirement: "for_all_users") } + fab!(:optional_field) { Fabricate(:user_field, requirement: "optional") } it "should update the user field" do put "/u/#{user.username}.json", diff --git a/spec/system/admin_user_fields_spec.rb b/spec/system/admin_user_fields_spec.rb new file mode 100644 index 00000000000..1bcdb66f0f8 --- /dev/null +++ b/spec/system/admin_user_fields_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +describe "Admin User Fields", type: :system, js: true do + fab!(:current_user) { Fabricate(:admin) } + + before { sign_in(current_user) } + + let(:user_fields_page) { PageObjects::Pages::AdminUserFields.new } + + it "correctly saves user fields" do + user_fields_page.visit + user_fields_page.add_field(name: "Occupation", description: "What you do for work") + + expect(user_fields_page).to have_user_field("Occupation") + + user_fields_page.refresh + + expect(user_fields_page).to have_user_field("Occupation") + end + + it "displays an error when missing required fields" do + user_fields_page.visit + + user_fields_page.add_field(name: "Occupation", description: "") + + expect(user_fields_page).to have_text(/Description can't be blank/) + end +end diff --git a/spec/system/page_objects/modals/signup.rb b/spec/system/page_objects/modals/signup.rb index 6dc67b30cb2..b0aace02f11 100644 --- a/spec/system/page_objects/modals/signup.rb +++ b/spec/system/page_objects/modals/signup.rb @@ -55,6 +55,10 @@ module PageObjects find("#inviteCode").fill_in(with: code) end + def fill_custom_field(name, value) + find(".user-field-#{name.downcase} input").fill_in(with: value) + end + def has_valid_email? find(".create-account-email").has_css?("#account-email-validation.good") end diff --git a/spec/system/page_objects/pages/admin_user_fields.rb b/spec/system/page_objects/pages/admin_user_fields.rb new file mode 100644 index 00000000000..3b69ac622fa --- /dev/null +++ b/spec/system/page_objects/pages/admin_user_fields.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module PageObjects + module Pages + class AdminUserFields < PageObjects::Pages::Base + def visit + page.visit "admin/customize/user_fields" + self + end + + def add_field(name: nil, description: nil, requirement: nil, preferences: []) + page.find(".user-fields .btn-primary").click + + form = page.find(".user-field") + + form.find(".user-field-name").fill_in(with: name) + form.find(".user-field-desc").fill_in(with: description) + form.find(".save").click + end + + def has_user_field?(name) + page.has_text?(name) + end + end + end +end diff --git a/spec/system/signup_spec.rb b/spec/system/signup_spec.rb index f8762c7e43a..85e9cdbe304 100644 --- a/spec/system/signup_spec.rb +++ b/spec/system/signup_spec.rb @@ -47,6 +47,40 @@ describe "Signup", type: :system do end end + context "when there are required user fields" do + before do + Fabricate( + :user_field, + name: "Occupation", + requirement: "on_signup", + description: "What you do for work", + ) + end + + it "can signup when filling the custom field" do + signup_modal.open + signup_modal.fill_email("johndoe@example.com") + signup_modal.fill_username("john") + signup_modal.fill_password("supersecurepassword") + signup_modal.fill_custom_field("Occupation", "Jedi") + expect(signup_modal).to have_valid_fields + + signup_modal.click_create_account + expect(page).to have_css(".account-created") + end + + it "cannot signup without filling the custom field" do + signup_modal.open + signup_modal.fill_email("johndoe@example.com") + signup_modal.fill_username("john") + signup_modal.fill_password("supersecurepassword") + + signup_modal.click_create_account + expect(signup_modal).to have_content(I18n.t("js.user_fields.required", name: "Occupation")) + expect(signup_modal).to have_no_css(".account-created") + end + end + context "when user requires approval" do before do SiteSetting.must_approve_users = true