From 43e17950038aa315da4cbe0b4d4aedff8342b2f4 Mon Sep 17 00:00:00 2001 From: David F Date: Thu, 11 Jun 2026 15:14:06 +0200 Subject: [PATCH 01/14] Primerize the administration user form. wp/72005 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../custom_field_field_component.html.erb | 5 - .../custom_field_section_component.html.erb | 11 -- app/components/users/form_component.html.erb | 33 +++++ app/components/users/form_component.rb | 95 ++++++++++++++ .../custom_fields/custom_field_rendering.rb | 18 ++- app/forms/users/form/attributes_form.rb | 112 ++++++++++++++++ .../users/form/authentication_source_form.rb | 80 +++++++++++ app/forms/users/form/password_form.rb | 124 ++++++++++++++++++ app/forms/users/form/preferences_form.rb | 87 ++++++++++++ app/helpers/users_helper.rb | 30 +++++ app/views/users/_consent.html.erb | 5 +- app/views/users/_form.html.erb | 51 ------- app/views/users/_general.html.erb | 46 +++---- app/views/users/_preferences.html.erb | 51 ------- app/views/users/_simple_form.html.erb | 40 ------ app/views/users/form/_admin_flag.html.erb | 4 - app/views/users/form/_preferences.html.erb | 4 - .../form/authentication/_auth_source.html.erb | 22 ---- .../form/authentication/_external.html.erb | 33 +++-- .../users/form/authentication/_form.html.erb | 15 --- .../form/authentication/_internal.html.erb | 12 -- .../_internal_password.html.erb | 71 ---------- .../form/built_in_fields/_firstname.html.erb | 7 - .../form/built_in_fields/_language.html.erb | 7 - .../form/built_in_fields/_lastname.html.erb | 7 - .../form/built_in_fields/_login.html.erb | 10 -- .../users/form/built_in_fields/_mail.html.erb | 7 - app/views/users/new.html.erb | 30 ++--- spec/components/users/form_component_spec.rb | 93 +++++++++++++ spec/features/users/edit_users_spec.rb | 2 +- spec/features/users/password_change_spec.rb | 6 +- .../custom_field_rendering_spec.rb | 22 ++++ .../forms/users/form/attributes_form_spec.rb | 55 ++++---- .../form/authentication_source_form_spec.rb | 63 +++++++++ spec/forms/users/form/password_form_spec.rb | 61 +++++++++ .../forms/users/form/preferences_form_spec.rb | 35 ++--- spec/helpers/users_helper_form_hooks_spec.rb | 74 +++++++++++ spec/views/users/edit.html.erb_spec.rb | 3 +- 38 files changed, 974 insertions(+), 457 deletions(-) delete mode 100644 app/components/users/form/custom_field_field_component.html.erb delete mode 100644 app/components/users/form/custom_field_section_component.html.erb create mode 100644 app/components/users/form_component.html.erb create mode 100644 app/components/users/form_component.rb create mode 100644 app/forms/users/form/attributes_form.rb create mode 100644 app/forms/users/form/authentication_source_form.rb create mode 100644 app/forms/users/form/password_form.rb create mode 100644 app/forms/users/form/preferences_form.rb delete mode 100644 app/views/users/_form.html.erb delete mode 100644 app/views/users/_preferences.html.erb delete mode 100644 app/views/users/_simple_form.html.erb delete mode 100644 app/views/users/form/_admin_flag.html.erb delete mode 100644 app/views/users/form/_preferences.html.erb delete mode 100644 app/views/users/form/authentication/_auth_source.html.erb delete mode 100644 app/views/users/form/authentication/_form.html.erb delete mode 100644 app/views/users/form/authentication/_internal.html.erb delete mode 100644 app/views/users/form/authentication/_internal_password.html.erb delete mode 100644 app/views/users/form/built_in_fields/_firstname.html.erb delete mode 100644 app/views/users/form/built_in_fields/_language.html.erb delete mode 100644 app/views/users/form/built_in_fields/_lastname.html.erb delete mode 100644 app/views/users/form/built_in_fields/_login.html.erb delete mode 100644 app/views/users/form/built_in_fields/_mail.html.erb create mode 100644 spec/components/users/form_component_spec.rb rename app/components/users/form/custom_field_field_component.rb => spec/forms/users/form/attributes_form_spec.rb (54%) create mode 100644 spec/forms/users/form/authentication_source_form_spec.rb create mode 100644 spec/forms/users/form/password_form_spec.rb rename app/components/users/form/custom_field_section_component.rb => spec/forms/users/form/preferences_form_spec.rb (61%) create mode 100644 spec/helpers/users_helper_form_hooks_spec.rb diff --git a/app/components/users/form/custom_field_field_component.html.erb b/app/components/users/form/custom_field_field_component.html.erb deleted file mode 100644 index b23e7671104b..000000000000 --- a/app/components/users/form/custom_field_field_component.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= @form.fields_for_custom_fields :custom_field_values, @form.object, field_options do |cf_form| %> - <%= content_tag :div, class: css_classes do - cf_form.cf_form_field(container_class:) - end %> -<% end %> diff --git a/app/components/users/form/custom_field_section_component.html.erb b/app/components/users/form/custom_field_section_component.html.erb deleted file mode 100644 index 241e5c33ae26..000000000000 --- a/app/components/users/form/custom_field_section_component.html.erb +++ /dev/null @@ -1,11 +0,0 @@ -
- <%= title %> - - <% @section.attribute_order.each do |key| %> - <% if built_in?(key) %> - <%= render "users/form/built_in_fields/#{key}", f: @form, contract: @contract, user: @user %> - <% elsif (cf = visible_custom_field(key)) %> - <%= render Users::Form::CustomFieldFieldComponent.new(custom_field: cf, form: @form) %> - <% end %> - <% end %> -
diff --git a/app/components/users/form_component.html.erb b/app/components/users/form_component.html.erb new file mode 100644 index 000000000000..09581698f239 --- /dev/null +++ b/app/components/users/form_component.html.erb @@ -0,0 +1,33 @@ +<% if editing? %> + <%= render(Primer::Box.new(mb: 3)) do %> + <%= render(Primer::Beta::Text.new(tag: :div, font_weight: :bold)) { I18n.t("attributes.status") } %> + <%= render(Primer::Beta::Text.new(tag: :div)) { helpers.full_user_status(@user, true) } %> + <% end %> +<% end %> + +<%= render(form_list) %> + +<% if show_no_login_message? %> + <%= render(Primer::Beta::Text.new(tag: :p, color: :muted)) { I18n.t("user.no_login") } %> +<% end %> + +<% if show_external_auth? %> + <%= render("users/form/authentication/external", user: @user) %> +<% end %> + +<%= helpers.render_user_form_hooks(user: @user, form: @builder) %> + +<% if show_consent? %> + <%= render("users/consent", user: @user) %> +<% end %> + +<% if show_preferences? %> + <%= fields_for(:pref, @user.pref, builder: @builder.class) do |pref_f| %> + <%= render(Users::Form::PreferencesForm.new(pref_f)) %> + <% end %> +<% end %> + +<%= render(Primer::Beta::Button.new(type: :submit, scheme: :primary, name: "submit")) { creating? ? I18n.t(:button_create) : I18n.t(:button_save) } %> +<% if creating? %> + <%= render(Primer::Beta::Button.new(type: :submit, name: "continue")) { I18n.t(:button_create_and_continue) } %> +<% end %> diff --git a/app/components/users/form_component.rb b/app/components/users/form_component.rb new file mode 100644 index 000000000000..c19d35937bf8 --- /dev/null +++ b/app/components/users/form_component.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users + # Coordinates the administration user form: receives the surrounding + # `settings_primer_form_with` builder, composes the inner forms into a + # Primer::Forms::FormList and renders the read-only / plain bits (status, + # consent, external auth, hooks, submit) and the pref-scoped preferences form + # around it. Create vs edit is derived from `user.new_record?`. + class FormComponent < ApplicationComponent + def initialize(builder:, user:, contract:) + super() + @builder = builder + @user = user + @contract = contract + end + + private + + def creating? = @user.new_record? + def editing? = !creating? + + def form_list + Primer::Forms::FormList.new(*input_forms) + end + + def input_forms + forms = [Users::Form::AttributesForm.new(@builder, user: @user, contract: @contract)] + forms << Users::Form::AuthenticationSourceForm.new(@builder, user: @user) if show_auth_source? + if show_password? + forms << Users::Form::PasswordForm.new(@builder, user: @user, + assign_random_password_checked: assign_random_password_checked?) + end + forms + end + + def show_auth_source? + return false if editing? && @user.uses_external_authentication? + + creating? ? can_users_have_auth_source? : (User.current.admin? || can_users_have_auth_source?) + end + + def show_password? + editing? && User.current.admin? && !@user.uses_external_authentication? && !disable_password_login? + end + + def show_preferences? = editing? && User.current.admin? + def show_external_auth? = editing? && @user.uses_external_authentication? + + def show_no_login_message? + editing? && User.current.admin? && !@user.uses_external_authentication? && disable_password_login? + end + + def show_consent? = editing? && Setting.consent_required? + + def can_users_have_auth_source? + LdapAuthSource.any? && !disable_password_login? + end + + def disable_password_login? + OpenProject::Configuration.disable_password_login? + end + + def assign_random_password_checked? + helpers.params.dig(:user, :assign_random_password).present? + end + end +end diff --git a/app/forms/custom_fields/custom_field_rendering.rb b/app/forms/custom_fields/custom_field_rendering.rb index ba2ef8744927..1fa122c9f5e0 100644 --- a/app/forms/custom_fields/custom_field_rendering.rb +++ b/app/forms/custom_fields/custom_field_rendering.rb @@ -52,13 +52,17 @@ module CustomFields::CustomFieldRendering def render_custom_fields(form:) custom_fields.each do |custom_field| - form.fields_for(:custom_field_values) do |builder| - custom_field_input(builder, custom_field) - end - if custom_field.has_comment? - form.fields_for(:custom_comments) do |builder| - custom_comment_input(builder, custom_field) - end + render_custom_field(form:, custom_field:) + end + end + + def render_custom_field(form:, custom_field:) + form.fields_for(:custom_field_values) do |builder| + custom_field_input(builder, custom_field) + end + if custom_field.has_comment? + form.fields_for(:custom_comments) do |builder| + custom_comment_input(builder, custom_field) end end end diff --git a/app/forms/users/form/attributes_form.rb b/app/forms/users/form/attributes_form.rb new file mode 100644 index 000000000000..5fc0d814fa65 --- /dev/null +++ b/app/forms/users/form/attributes_form.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users + module Form + # Admin flag + the custom-field sections (built-in fields interleaved with + # custom fields per UserCustomFieldSection, honoring attribute_order). + class AttributesForm < ApplicationForm + include CustomFields::CustomFieldRendering + + form do |f| + admin_flag(f) if User.current.admin? + user_sections(f) + end + + def initialize(user:, contract:) + super() + @user = user + @contract = contract + end + + private + + def custom_fields + @custom_fields ||= @user.available_custom_fields + end + + def admin_flag(form) + form.check_box(name: :admin, + label: User.human_attribute_name(:admin), + disabled: @user == User.current) + end + + def user_sections(form) + UserCustomFieldSection.includes(:custom_fields).each do |section| # rubocop:disable Rails/FindEach -- honor default scope ordering + render_section(form, section) + end + end + + def render_section(form, section) + visible_cfs_by_key = section.custom_fields.visible(User.current).index_by(&:column_name) + + form.fieldset_group(title: section_title(section)) do |group| + section.attribute_order.each do |key| + if UserCustomFieldSection::BUILT_IN_ATTRIBUTES.include?(key) + render_built_in(group, key) + elsif (custom_field = visible_cfs_by_key[key]) + render_custom_field(form: group, custom_field:) + end + end + end + end + + def section_title(section) + section.name.presence || I18n.t("settings.user_custom_fields.label_untitled_section") + end + + def render_built_in(group, key) # rubocop:disable Metrics/AbcSize + case key + when "firstname", "lastname", "mail" + group.text_field(name: key.to_sym, + label: User.human_attribute_name(key), + required: true, + disabled: !@contract.writable?(key.to_sym), + input_width: :medium) + when "login" + return if @user.new_record? + + group.text_field(name: :login, + label: User.human_attribute_name(:login), + required: true, + disabled: !@contract.writable?(:login), + input_width: :medium) + when "language" + group.select_list(name: :language, + label: User.human_attribute_name(:language), + include_blank: "--- #{I18n.t(:actionview_instancetag_blank_option)} ---", + input_width: :medium) do |list| + helpers.lang_options_for_select.each { |label, value| list.option(label:, value:) } + end + end + end + end + end +end diff --git a/app/forms/users/form/authentication_source_form.rb b/app/forms/users/form/authentication_source_form.rb new file mode 100644 index 000000000000..8a399d7a63c6 --- /dev/null +++ b/app/forms/users/form/authentication_source_form.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users + module Form + # LDAP authentication source select. For a new record it also renders the + # hidden login field the admin--users controller reveals when an LDAP source + # is selected; for a persisted record the login is already a built-in field, + # so the select is wrapped in a titled "Authentication" fieldset only. + # + # The coordinator decides whether to include this form at all. + class AuthenticationSourceForm < ApplicationForm + form do |f| + if @user.new_record? + ldap_auth_source_select(f) + hidden_login(f) + else + f.fieldset_group(title: I18n.t(:label_authentication)) do |group| + ldap_auth_source_select(group) + end + end + end + + def initialize(user:) + super() + @user = user + end + + private + + def ldap_auth_source_select(target) + target.select_list( + name: :ldap_auth_source_id, + label: User.human_attribute_name(:auth_source), + include_blank: I18n.t(:label_internal), + input_width: :medium, + data: { action: "admin--users#toggleAuthenticationFields" } + ) do |list| + LdapAuthSource.order(:name).each { |source| list.option(label: source.name, value: source.id) } + end + end + + def hidden_login(form) + form.group(hidden: true, data: { "admin--users-target": "authSourceFields" }) do |group| + group.text_field(name: :login, + label: User.human_attribute_name(:login), + required: true, + input_width: :medium) + end + end + end + end +end diff --git a/app/forms/users/form/password_form.rb b/app/forms/users/form/password_form.rb new file mode 100644 index 000000000000..30d56dd0f3bb --- /dev/null +++ b/app/forms/users/form/password_form.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users + module Form + # The internal password / activation block as a single Primer group. All three + # Stimulus controllers sit on the wrapper: the password-requirements controller + # finds both its passwordInput and requirement targets within it. + # assign_random_password and send_information are not model + # attributes, so they render with an explicit checked: and no hidden companion + # (the controller relies on their absence when unchecked); send_information is + # unscoped (top-level param). Included by the coordinator only for editing an + # internal user as admin while password login is enabled. + class PasswordForm < ApplicationForm + form do |f| + f.group( + hidden: !@user.change_password_allowed?, + data: { + controller: "disable-when-checked password-force-change password-requirements", + "admin--users-target": "passwordFields" + } + ) do |group| + assign_random_password(group) + password_fields(group) unless disable_password_choice? + send_information(group) + force_password_change(group) + end + end + + def initialize(user:, assign_random_password_checked:) + super() + @user = user + @assign_random_password_checked = assign_random_password_checked + end + + private + + def disable_password_choice? + OpenProject::Configuration.disable_password_choice? + end + + def assign_random_password(group) + group.check_box(name: :assign_random_password, + id: "user_assign_random_password", + checked: @assign_random_password_checked, + include_hidden: false, + label: I18n.t("user.assign_random_password"), + data: { + "disable-when-checked-target": "cause", + "password-force-change-target": "assignRandomPassword" + }) + end + + def password_fields(group) + group.text_field(name: :password, + id: "user_password", + type: :password, + required: @user.new_record?, + label: User.human_attribute_name(:password), + caption: helpers.password_complexity_requirements, + input_width: :medium, + data: { + "disable-when-checked-target": "effect", + "password-requirements-target": "passwordInput" + }) + group.text_field(name: :password_confirmation, + id: "user_password_confirmation", + type: :password, + required: @user.new_record?, + label: User.human_attribute_name(:password_confirmation), + input_width: :medium, + data: { "disable-when-checked-target": "effect" }) + end + + def send_information(group) + group.check_box(name: :send_information, + id: "send_information", + scope_name_to_model: false, + scope_id_to_model: false, + checked: false, + include_hidden: false, + label: I18n.t(:label_send_information), + caption: I18n.t("users.send_information_hint"), + data: { "password-force-change-target": "sendInformationCheckbox" }) + end + + def force_password_change(group) + group.check_box(name: :force_password_change, + id: "user_force_password_change", + checked: @user.force_password_change, + label: User.human_attribute_name(:force_password_change), + caption: I18n.t("users.force_password_change_hint"), + data: { "password-force-change-target": "forceChangeCheckbox" }) + end + end + end +end diff --git a/app/forms/users/form/preferences_form.rb b/app/forms/users/form/preferences_form.rb new file mode 100644 index 000000000000..019c787f7829 --- /dev/null +++ b/app/forms/users/form/preferences_form.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users + module Form + # Preferences section of the administration user form: time zone, color mode + # and keyboard shortcuts. Bound to the user's preference (scope: :pref). + # + # My::LookAndFeelForm could not be reused directly because it renders its own + # submit button and extra fields (comments sorting, contrast); the time zone + # option building mirrors My::TimeZoneForm. + class PreferencesForm < ApplicationForm + form do |f| + f.fieldset_group(title: I18n.t(:label_preferences)) do |group| + group.select_list( + name: :time_zone, + label: UserPreference.human_attribute_name(:time_zone), + include_blank: false, + input_width: :medium + ) do |list| + time_zone_options.each { |label, value| list.option(label:, value:) } + end + + group.select_list( + name: :theme, + label: UserPreference.human_attribute_name(:theme), + caption: UserPreference.human_attribute_name(:mode_guideline), + include_blank: false, + input_width: :medium + ) do |list| + helpers.theme_options_for_select.each { |label, value| list.option(label:, value:) } + end + + group.check_box( + name: :disable_keyboard_shortcuts, + label: UserPreference.human_attribute_name(:disable_keyboard_shortcuts), + caption: helpers.link_translate(:"user_preferences.disable_keyboard_shortcuts_caption", + links: { docs_url: %i[shortcuts] }) + ) + end + end + + private + + def time_zone_options + UserPreferences::UpdateContract + .assignable_time_zones + .group_by { |zone| zone.tzinfo.canonical_zone } + .map { |canonical_zone, zones| time_zone_entry(canonical_zone, zones) } + end + + def time_zone_entry(canonical_zone, zones) + zone_names = zones.map(&:name).join(", ") + offset = ActiveSupport::TimeZone.seconds_to_utc_offset(canonical_zone.base_utc_offset) + + ["(UTC#{offset}) #{zone_names}", canonical_zone.identifier] + end + end + end +end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 779cda7ff61b..7ca94bbc8a4f 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -153,4 +153,34 @@ def allowed_management_user_profile_path(user) def can_users_have_auth_source? LdapAuthSource.any? && !OpenProject::Configuration.disable_password_login? end + + # Renders the user form extension hooks inside the (Primer) user form. + # + # - +view_users_primer_form+ receives the Primer form builder and is the API + # plugins should use going forward. + # - +view_users_form+ is deprecated but kept working: it receives a legacy + # TabularFormBuilder rendered inside the same form, so existing plugins keep + # submitting their fields. It is only rendered (and the deprecation logged) + # when a listener is actually registered. + def render_user_form_hooks(user:, form:) + safe_join([ + call_hook(:view_users_primer_form, user:, form:), + legacy_user_form_hook(user:) + ].compact) + end + + private + + def legacy_user_form_hook(user:) + return unless OpenProject::Hook.hook_listeners(:view_users_form).any? + + OpenProject::Deprecation.warn( + "The `view_users_form` hook is deprecated; migrate to `view_users_primer_form` " \ + "which receives a Primer form builder." + ) + + fields_for(:user, user, builder: TabularFormBuilder) do |legacy_form| + call_hook(:view_users_form, user:, form: legacy_form) + end + end end diff --git a/app/views/users/_consent.html.erb b/app/views/users/_consent.html.erb index ee2c08b82c92..4e728216868b 100644 --- a/app/views/users/_consent.html.erb +++ b/app/views/users/_consent.html.erb @@ -1,11 +1,12 @@ +<%# locals: (user:) -%>

<%= t("consent.title") %>

<% consent_link = admin_settings_users_path(anchor: "consent_settings") %> <%= render ::Components::OnOffStatusComponent.new( { - is_on: @user.consented_at.present?, - on_text: format_time(@user.consented_at), + is_on: user.consented_at.present?, + on_text: format_time(user.consented_at), on_description: link_translate("consent.user_has_consented", links: { consent_settings: consent_link }), off_text: t(:label_never), off_description: link_translate("consent.not_yet_consented", links: { consent_settings: consent_link }) diff --git a/app/views/users/_form.html.erb b/app/views/users/_form.html.erb deleted file mode 100644 index 7c55671ce628..000000000000 --- a/app/views/users/_form.html.erb +++ /dev/null @@ -1,51 +0,0 @@ -<%#-- copyright -OpenProject is an open source project management software. -Copyright (C) the OpenProject GmbH - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License version 3. - -OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -Copyright (C) 2006-2013 Jean-Philippe Lang -Copyright (C) 2010-2013 the ChiliProject Team - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License -as published by the Free Software Foundation; either version 2 -of the License, or (at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU General Public License for more details. - -You should have received a copy of the GNU General Public License -along with this program; if not, write to the Free Software -Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - -See COPYRIGHT and LICENSE files for more details. - -++#%> - -<%= error_messages_for @user %> - -
- <% if current_user.admin? %> - <%= render partial: "users/form/admin_flag", locals: { f: f } %> - <% end %> - - <% UserCustomFieldSection.includes(:custom_fields).each do |section| # rubocop:disable Rails/FindEach -- to honor default scope ordering %> - <%= render Users::Form::CustomFieldSectionComponent.new(section:, form: f, contract: @contract, user: @user) %> - <% end %> - <%= call_hook(:view_users_form, user: @user, form: f) %> -
- -<% if Setting.consent_required? %> - <%= render partial: "users/consent" %> -<% end %> - -<%= render partial: "users/form/authentication/form", locals: { f: f } %> - -<% if current_user.admin? %> - <%= render partial: "users/form/preferences", locals: { f: f } %> -<% end %> diff --git a/app/views/users/_general.html.erb b/app/views/users/_general.html.erb index d869adf9db59..02928a5246bf 100644 --- a/app/views/users/_general.html.erb +++ b/app/views/users/_general.html.erb @@ -27,32 +27,20 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -
-
- <%= styled_label_tag "FIXME", I18n.t("attributes.status") %> -
- - <%= full_user_status(@user, true) %> - -
-
-
- -<%= labelled_tabular_form_for @user, - url: { controller: "/users", - action: "update", - tab: nil }, - html: { - method: :put, - autocomplete: "off" - }, - data: { - controller: "admin--users", - turbo: false, - "admin--users-password-auth-selected-value": @user.ldap_auth_source_id.blank? - }, - as: :user do |f| %> - <%= render partial: "users/form", locals: { f: f } %> - - <%= styled_button_tag t(:button_save), class: "-primary -with-icon icon-checkmark" %> -<% end %> +<%= error_messages_for @user %> + +<%= + settings_primer_form_with( + model: @user, + url: { controller: "/users", action: "update", tab: nil }, + method: :put, + html: { autocomplete: "off" }, + data: { + controller: "admin--users", + turbo: false, + "admin--users-password-auth-selected-value": @user.ldap_auth_source_id.blank? + } + ) do |f| + render(Users::FormComponent.new(builder: f, user: @user, contract: @contract)) + end +%> diff --git a/app/views/users/_preferences.html.erb b/app/views/users/_preferences.html.erb deleted file mode 100644 index 02be01dfd762..000000000000 --- a/app/views/users/_preferences.html.erb +++ /dev/null @@ -1,51 +0,0 @@ -<%#-- copyright -OpenProject is an open source project management software. -Copyright (C) the OpenProject GmbH - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License version 3. - -OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -Copyright (C) 2006-2013 Jean-Philippe Lang -Copyright (C) 2010-2013 the ChiliProject Team - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License -as published by the Free Software Foundation; either version 2 -of the License, or (at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU General Public License for more details. - -You should have received a copy of the GNU General Public License -along with this program; if not, write to the Free Software -Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - -See COPYRIGHT and LICENSE files for more details. - -++#%> -<%= fields_for :pref, @user.pref, builder: TabularFormBuilder, lang: current_language do |pref_fields| %> - <%= render Settings::TimeZoneSettingComponent.new( - "time_zone", - form: pref_fields, - include_blank: false, - container_class: defined?(input_size) ? "-#{input_size}" : "-wide" - ) %> -
- <%= pref_fields.select :theme, theme_options_for_select, container_class: "-middle" %> -
- <%= I18n.t("activerecord.attributes.user_preference.mode_guideline") %> -
-
- -
- <%= pref_fields.check_box :disable_keyboard_shortcuts, - label: I18n.t("activerecord.attributes.user_preference.disable_keyboard_shortcuts") %> - - <%= link_translate(:"user_preferences.disable_keyboard_shortcuts_caption", - links: { docs_url: %i[shortcuts] }) %> - -
-<% end %> diff --git a/app/views/users/_simple_form.html.erb b/app/views/users/_simple_form.html.erb deleted file mode 100644 index 3a46855ecac4..000000000000 --- a/app/views/users/_simple_form.html.erb +++ /dev/null @@ -1,40 +0,0 @@ -<%#-- copyright -OpenProject is an open source project management software. -Copyright (C) the OpenProject GmbH - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License version 3. - -OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -Copyright (C) 2006-2013 Jean-Philippe Lang -Copyright (C) 2010-2013 the ChiliProject Team - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License -as published by the Free Software Foundation; either version 2 -of the License, or (at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU General Public License for more details. - -You should have received a copy of the GNU General Public License -along with this program; if not, write to the Free Software -Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - -See COPYRIGHT and LICENSE files for more details. - -++#%> - -
- <% if current_user.admin? %> - <%= render partial: "users/form/authentication/auth_source", locals: { f: f } %> - <%= render partial: "users/form/admin_flag", locals: { f: f } %> - <% end %> - - <% UserCustomFieldSection.includes(:custom_fields).each do |section| # rubocop:disable Rails/FindEach -- to honor default scope ordering %> - <%= render Users::Form::CustomFieldSectionComponent.new(section:, form: f, contract: @contract, user: @user) %> - <% end %> - <%= call_hook(:view_users_form, user: @user, form: f) %> -
diff --git a/app/views/users/form/_admin_flag.html.erb b/app/views/users/form/_admin_flag.html.erb deleted file mode 100644 index a77037e4366a..000000000000 --- a/app/views/users/form/_admin_flag.html.erb +++ /dev/null @@ -1,4 +0,0 @@ -
- <%= f.check_box :admin, - disabled: (@user == User.current) %> -
diff --git a/app/views/users/form/_preferences.html.erb b/app/views/users/form/_preferences.html.erb deleted file mode 100644 index fe723dd53a36..000000000000 --- a/app/views/users/form/_preferences.html.erb +++ /dev/null @@ -1,4 +0,0 @@ -
-

<%= t(:label_preferences) %>

- <%= render partial: "users/preferences", locals: { input_size: :middle } %> -
diff --git a/app/views/users/form/authentication/_auth_source.html.erb b/app/views/users/form/authentication/_auth_source.html.erb deleted file mode 100644 index efdf690ea024..000000000000 --- a/app/views/users/form/authentication/_auth_source.html.erb +++ /dev/null @@ -1,22 +0,0 @@ -<% if can_users_have_auth_source? %> -
- <%= f.collection_select :ldap_auth_source_id, - LdapAuthSource.all, - :id, - :name, - { - label: :"activerecord.attributes.user.auth_source", - container_class: "-middle", - include_blank: t(:label_internal) - }, - data: { - action: "admin--users#toggleAuthenticationFields" - } %> -
- - -<% end %> diff --git a/app/views/users/form/authentication/_external.html.erb b/app/views/users/form/authentication/_external.html.erb index c14c24105a5d..749daf79d816 100644 --- a/app/views/users/form/authentication/_external.html.erb +++ b/app/views/users/form/authentication/_external.html.erb @@ -1,19 +1,16 @@ -
- <%= styled_label_tag nil, I18n.t("user.authentication_provider") %> -
- <%= @user.human_authentication_provider %> -
-
-
- <%= I18n.t("user.authentication_settings_disabled_due_to_external_authentication") %> -
+<%# locals: (user:) -%> +<%= render(Primer::Box.new(mb: 3)) do %> + <%= render(Primer::Beta::Text.new(tag: :div, font_weight: :bold)) { I18n.t("user.authentication_provider") } %> + <%= render(Primer::Beta::Text.new(tag: :div)) { user.human_authentication_provider } %> + <%= render(Primer::Beta::Text.new(tag: :div, color: :muted, font_size: :small)) do %> + <%= I18n.t("user.authentication_settings_disabled_due_to_external_authentication") %> + <% end %> +<% end %> -
- <%= styled_label_tag nil, User.human_attribute_name(:identity_url) %> -
- <%= @user.identity_url.split(":", 2).last %> -
-
-
- <%= I18n.t("user.identity_url_text") %> -
+<%= render(Primer::Box.new) do %> + <%= render(Primer::Beta::Text.new(tag: :div, font_weight: :bold)) { User.human_attribute_name(:identity_url) } %> + <%= render(Primer::Beta::Text.new(tag: :div)) { user.identity_url.split(":", 2).last } %> + <%= render(Primer::Beta::Text.new(tag: :div, color: :muted, font_size: :small)) do %> + <%= I18n.t("user.identity_url_text") %> + <% end %> +<% end %> diff --git a/app/views/users/form/authentication/_form.html.erb b/app/views/users/form/authentication/_form.html.erb deleted file mode 100644 index 1bec280b2832..000000000000 --- a/app/views/users/form/authentication/_form.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -
- <% if current_user.admin? || can_users_have_auth_source? || @user.uses_external_authentication? %> -

<%= t(:label_authentication) %>

- <% end %> - - <% if @user.uses_external_authentication? %> - <%= render partial: "users/form/authentication/external", locals: { f: f } %> - <% else %> - <%= render partial: "users/form/authentication/auth_source", locals: { f: f } %> - - <% if current_user.admin? %> - <%= render partial: "users/form/authentication/internal", locals: { f: f } %> - <% end %> - <% end %> -
diff --git a/app/views/users/form/authentication/_internal.html.erb b/app/views/users/form/authentication/_internal.html.erb deleted file mode 100644 index 1b7c7f220e0a..000000000000 --- a/app/views/users/form/authentication/_internal.html.erb +++ /dev/null @@ -1,12 +0,0 @@ -<% if OpenProject::Configuration.disable_password_login? %> -
-
- <%= styled_label_tag nil, I18n.t(:warning) %> -
- <%= I18n.t "user.no_login" %> -
-
-
-<% else %> - <%= render partial: "users/form/authentication/internal_password", locals: { f: f } %> -<% end %> diff --git a/app/views/users/form/authentication/_internal_password.html.erb b/app/views/users/form/authentication/_internal_password.html.erb deleted file mode 100644 index 405a627d9fc5..000000000000 --- a/app/views/users/form/authentication/_internal_password.html.erb +++ /dev/null @@ -1,71 +0,0 @@ -<%= content_tag :div, - data: { - controller: "disable-when-checked password-force-change", - "admin--users-target": "passwordFields" - }, - hidden: !@user.change_password_allowed? do %> - <% assign_random_password_enabled = params[:user] && - params[:user][:assign_random_password] %> -
- <%= styled_label_tag "user_assign_random_password", - I18n.t("user.assign_random_password") %> -
- <%= styled_check_box_tag "user[assign_random_password]", - "1", - assign_random_password_enabled, - data: { - "disable-when-checked-target": "cause", - "password-force-change-target": "assignRandomPassword" - } %> -
-
- - <% unless OpenProject::Configuration.disable_password_choice? %> -
- <%= f.password_field :password, - required: @user.new_record?, - disabled: assign_random_password_enabled, - data: { - "disable-when-checked-target": "effect", - "password-requirements-target": "passwordInput" - }, - container_class: "-middle" %> -
- <%= password_complexity_requirements %> -
-
-
- <%= f.password_field :password_confirmation, - required: @user.new_record?, - disabled: assign_random_password_enabled, - data: { - "disable-when-checked-target": "effect" - }, - container_class: "-middle" %> -
- <% end %> - -
- <%= styled_label_tag "send_information", t(:label_send_information) %> -
- <%= styled_check_box_tag( - "send_information", - "1", - false, - data: { "password-force-change-target": "sendInformationCheckbox" } - ) %> -
-
- <%= t("users.send_information_hint") %> -
-
- -
- <%= f.check_box :force_password_change, - disabled: assign_random_password_enabled, - data: { "password-force-change-target": "forceChangeCheckbox" } %> -
- <%= t("users.force_password_change_hint") %> -
-
-<% end %> diff --git a/app/views/users/form/built_in_fields/_firstname.html.erb b/app/views/users/form/built_in_fields/_firstname.html.erb deleted file mode 100644 index 623344bcd621..000000000000 --- a/app/views/users/form/built_in_fields/_firstname.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%# locals: (f:, contract:, user:) -%> -
- <%= f.text_field :firstname, - required: true, - disabled: !contract.writable?(:firstname), - container_class: "-middle" %> -
diff --git a/app/views/users/form/built_in_fields/_language.html.erb b/app/views/users/form/built_in_fields/_language.html.erb deleted file mode 100644 index ed8562925f05..000000000000 --- a/app/views/users/form/built_in_fields/_language.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%# locals: (f:, contract:, user:) -%> -
- <%= f.select :language, - lang_options_for_select, - prompt: "--- #{t(:actionview_instancetag_blank_option)} ---", - container_class: "-middle" %> -
diff --git a/app/views/users/form/built_in_fields/_lastname.html.erb b/app/views/users/form/built_in_fields/_lastname.html.erb deleted file mode 100644 index e28ad9940a26..000000000000 --- a/app/views/users/form/built_in_fields/_lastname.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%# locals: (f:, contract:, user:) -%> -
- <%= f.text_field :lastname, - required: true, - disabled: !contract.writable?(:lastname), - container_class: "-middle" %> -
diff --git a/app/views/users/form/built_in_fields/_login.html.erb b/app/views/users/form/built_in_fields/_login.html.erb deleted file mode 100644 index 9630997392b0..000000000000 --- a/app/views/users/form/built_in_fields/_login.html.erb +++ /dev/null @@ -1,10 +0,0 @@ -<%# locals: (f:, contract:, user:) -%> -<% unless user.new_record? %> -
- <%= f.text_field :login, - required: true, - disabled: !contract.writable?(:login), - size: 25, - container_class: "-middle" %> -
-<% end %> diff --git a/app/views/users/form/built_in_fields/_mail.html.erb b/app/views/users/form/built_in_fields/_mail.html.erb deleted file mode 100644 index 1d0245ce5226..000000000000 --- a/app/views/users/form/built_in_fields/_mail.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%# locals: (f:, contract:, user:) -%> -
- <%= f.text_field :mail, - required: true, - disabled: !contract.writable?(:mail), - container_class: "-middle" %> -
diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 35acaba0f5ba..e03cb16a1458 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -43,19 +43,17 @@ See COPYRIGHT and LICENSE files for more details. <%= error_messages_for @user %> -<%= labelled_tabular_form_for @user, - url: { action: "create" }, - html: { class: nil, autocomplete: "off" }, - data: { - controller: "admin--users", - turbo: false, - "admin--users-password-auth-selected-value": @user.ldap_auth_source_id.blank? - }, - as: :user do |f| %> - <%= render partial: "simple_form", locals: { f: f, auth_sources: @ldap_auth_sources, user: @user } %> - -

- <%= styled_button_tag t(:button_create), class: "-primary -with-icon icon-checkmark" %> - <%= styled_button_tag t(:button_create_and_continue), name: "continue", class: "-primary -with-icon icon-checkmark" %> -

-<% end %> +<%= + settings_primer_form_with( + model: @user, + url: { action: "create" }, + html: { autocomplete: "off" }, + data: { + controller: "admin--users", + turbo: false, + "admin--users-password-auth-selected-value": @user.ldap_auth_source_id.blank? + } + ) do |f| + render(Users::FormComponent.new(builder: f, user: @user, contract: @contract)) + end +%> diff --git a/spec/components/users/form_component_spec.rb b/spec/components/users/form_component_spec.rb new file mode 100644 index 000000000000..92569b9c6f0e --- /dev/null +++ b/spec/components/users/form_component_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Users::FormComponent, type: :component do + let(:current_user) { build_stubbed(:admin) } + let(:contract) { instance_double(Users::UpdateContract, writable?: true) } + + before do + User.current = current_user + create(:user_custom_field_section, attribute_order: UserCustomFieldSection::BUILT_IN_ATTRIBUTES) + end + + def render_component(user:) + render_in_view_context(user, contract) do |form_user, form_contract| + primer_form_with(model: form_user, url: "/users") do |form| + render(Users::FormComponent.new(builder: form, user: form_user, contract: form_contract)) + end + end + end + + context "for a new user" do + it "renders the attributes and the create buttons, but no password block" do + render_component(user: User.new) + + expect(page).to have_field("user[firstname]") + expect(page).to have_button(I18n.t(:button_create)) + expect(page).to have_button(I18n.t(:button_create_and_continue)) + expect(page).to have_no_css("[data-admin--users-target='passwordFields']", visible: :all) + end + end + + context "for a persisted internal user edited by an admin" do + let(:user) { create(:user) } + + it "renders status, the password block, preferences and a Save button" do + render_component(user:) + + expect(page).to have_css("[data-admin--users-target='passwordFields']", visible: :all) + expect(page).to have_select("pref[time_zone]") + expect(page).to have_button(I18n.t(:button_save)) + end + + it "renders the consent status when consent is required" do + allow(Setting).to receive(:consent_required?).and_return(true) + + render_component(user:) + + expect(page).to have_text(I18n.t("consent.title")) + end + end + + context "for a persisted external-auth user" do + let(:user) { create(:user, identity_url: "saml:user-id") } + + before { allow(user).to receive(:uses_external_authentication?).and_return(true) } + + it "renders the read-only authentication provider instead of the password block" do + render_component(user:) + + expect(page).to have_text(I18n.t("user.authentication_provider")) + expect(page).to have_no_css("[data-admin--users-target='passwordFields']", visible: :all) + end + end +end diff --git a/spec/features/users/edit_users_spec.rb b/spec/features/users/edit_users_spec.rb index 23c65f6b8784..15355fb483bd 100644 --- a/spec/features/users/edit_users_spec.rb +++ b/spec/features/users/edit_users_spec.rb @@ -59,7 +59,7 @@ def user_password before { visit edit_user_path(user) } it "saves a custom field value" do - within "fieldset", text: "Professional info".upcase do + within "fieldset", text: "Professional info" do fill_in "Job title", with: "Software Engineer" fill_in "Internal code", with: "SE" end diff --git a/spec/features/users/password_change_spec.rb b/spec/features/users/password_change_spec.rb index 035094ad054b..a5f046612a4f 100644 --- a/spec/features/users/password_change_spec.rb +++ b/spec/features/users/password_change_spec.rb @@ -143,19 +143,19 @@ # And I try to set my new password to "adminADMIN" fill_in "user_password", with: "adminADMIN" fill_in "user_password_confirmation", with: "adminADMIN" - scroll_to_and_click(find(".button", text: "Save")) + scroll_to_and_click(find_button("Save")) expect_flash(type: :error, message: "Password Must include characters of the following types") # Has numeric but still missing special fill_in "user_password", with: "adminADMIN123" fill_in "user_password_confirmation", with: "adminADMIN123" - scroll_to_and_click(find(".button", text: "Save")) + scroll_to_and_click(find_button("Save")) expect_flash(type: :error, message: "Password Must include characters of the following types") # All classes fill_in "user_password", with: "adminADMIN1!" fill_in "user_password_confirmation", with: "adminADMIN1!" - scroll_to_and_click(find(".button", text: "Save")) + scroll_to_and_click(find_button("Save")) expect_flash(message: I18n.t(:notice_successful_update)) end end diff --git a/spec/forms/custom_fields/custom_field_rendering_spec.rb b/spec/forms/custom_fields/custom_field_rendering_spec.rb index e87c717d791b..c3f9fdd8d30a 100644 --- a/spec/forms/custom_fields/custom_field_rendering_spec.rb +++ b/spec/forms/custom_fields/custom_field_rendering_spec.rb @@ -204,4 +204,26 @@ end end end + + describe "#render_custom_field" do + let(:values_builder) { instance_double(ActionView::Helpers::FormBuilder) } + let(:custom_field) { build(:custom_field, :string) } + + before do + allow(builder).to receive(:fields_for).with(:custom_field_values).and_yield(values_builder) + allow(form_instance).to receive(:additional_custom_field_input_arguments).and_return({}) + end + + it "renders a single custom field input" do + allow(CustomFields::Inputs::String).to receive(:new) + + form_instance.render_custom_field(form: builder, custom_field:) + + expect(CustomFields::Inputs::String).to have_received(:new).with( + values_builder, + custom_field:, + object: model + ) + end + end end diff --git a/app/components/users/form/custom_field_field_component.rb b/spec/forms/users/form/attributes_form_spec.rb similarity index 54% rename from app/components/users/form/custom_field_field_component.rb rename to spec/forms/users/form/attributes_form_spec.rb index 2279a5904e4f..f862cf759efe 100644 --- a/app/components/users/form/custom_field_field_component.rb +++ b/spec/forms/users/form/attributes_form_spec.rb @@ -28,41 +28,38 @@ # See COPYRIGHT and LICENSE files for more details. #++ -module Users - module Form - class CustomFieldFieldComponent < ApplicationComponent - include ApplicationHelper +require "spec_helper" - def initialize(custom_field:, form:) - super() - @custom_field = custom_field - @form = form - end +RSpec.describe Users::Form::AttributesForm, type: :forms do + before do + User.current = current_user + create(:user_custom_field_section, attribute_order: UserCustomFieldSection::BUILT_IN_ATTRIBUTES) + end + + include_context "with rendered form" - def field_options - { - custom_value: @form.object.custom_value_for(@custom_field), - custom_field: @custom_field - } - end + let(:current_user) { build_stubbed(:admin) } + let(:contract) { instance_double(Users::UpdateContract, writable?: true) } + let(:params) { { user: model, contract: } } - def css_classes - ["form--field", @custom_field.attribute_name, required_class].compact - end + context "for a persisted user" do + let(:model) { build_stubbed(:user) } - def container_class - case @custom_field.field_format - when "text" then "-xxwide" - when "date" then "-xslim" - else "-middle" - end - end + it "renders the built-in fields, login and the admin flag" do + expect(page).to have_field("user[firstname]") + expect(page).to have_field("user[lastname]") + expect(page).to have_field("user[mail]") + expect(page).to have_select("user[language]") + expect(page).to have_field("user[login]") + expect(page).to have_field("user[admin]") + end + end - private + context "for a new user" do + let(:model) { User.new } - def required_class - "-required" if @custom_field.is_required? && !@custom_field.boolean? - end + it "omits the login field" do + expect(page).to have_no_field("user[login]") end end end diff --git a/spec/forms/users/form/authentication_source_form_spec.rb b/spec/forms/users/form/authentication_source_form_spec.rb new file mode 100644 index 000000000000..734075743baa --- /dev/null +++ b/spec/forms/users/form/authentication_source_form_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Users::Form::AuthenticationSourceForm, type: :forms do + before do + User.current = build_stubbed(:admin) + create(:ldap_auth_source) + end + + include_context "with rendered form" + + let(:params) { { user: model } } + + context "for a new user" do + let(:model) { User.new } + + it "renders the auth source select with the toggle action and a hidden login group" do + expect(page).to have_select("user[ldap_auth_source_id]") + expect(page).to have_css("[data-action~='admin--users#toggleAuthenticationFields']") + expect(page).to have_css("[data-admin--users-target='authSourceFields'][hidden]", visible: :all) + expect(page).to have_field("user[login]", visible: :all) + end + end + + context "for a persisted user" do + let(:model) { build_stubbed(:user) } + + it "renders the select inside a titled Authentication fieldset and no hidden login" do + expect(page).to have_select("user[ldap_auth_source_id]") + expect(page).to have_css("fieldset", text: /#{I18n.t(:label_authentication)}/i) + expect(page).to have_no_css("[data-admin--users-target='authSourceFields']", visible: :all) + end + end +end diff --git a/spec/forms/users/form/password_form_spec.rb b/spec/forms/users/form/password_form_spec.rb new file mode 100644 index 000000000000..f79435c00442 --- /dev/null +++ b/spec/forms/users/form/password_form_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Users::Form::PasswordForm, type: :forms do + include_context "with rendered form" + + let(:model) { build_stubbed(:user) } + let(:params) { { user: model, assign_random_password_checked: false } } + + before { allow(model).to receive(:change_password_allowed?).and_return(true) } + + it "renders one wrapper carrying the three controllers and the passwordFields target" do + expect(page).to have_css( + "[data-controller~='disable-when-checked'][data-controller~='password-force-change']" \ + "[data-controller~='password-requirements'][data-admin--users-target='passwordFields']", + visible: :all + ) + end + + it "renders the password fields with their ids and password type" do + expect(page).to have_field("user[password]", type: "password", visible: :all) + expect(page).to have_field("user[password_confirmation]", type: "password", visible: :all) + expect(page).to have_css("#user_password", visible: :all) + end + + it "renders the unscoped send_information checkbox and the scoped flags" do + expect(page).to have_field("send_information", visible: :all) + expect(page).to have_css("#send_information", visible: :all) + expect(page).to have_field("user[assign_random_password]", visible: :all) + expect(page).to have_field("user[force_password_change]", visible: :all) + end +end diff --git a/app/components/users/form/custom_field_section_component.rb b/spec/forms/users/form/preferences_form_spec.rb similarity index 61% rename from app/components/users/form/custom_field_section_component.rb rename to spec/forms/users/form/preferences_form_spec.rb index 9ab2bf9da8de..3aabc66cc119 100644 --- a/app/components/users/form/custom_field_section_component.rb +++ b/spec/forms/users/form/preferences_form_spec.rb @@ -28,35 +28,16 @@ # See COPYRIGHT and LICENSE files for more details. #++ -module Users - module Form - class CustomFieldSectionComponent < ApplicationComponent - def initialize(section:, form:, contract:, user:) - super() - @section = section - @form = form - @contract = contract - @user = user - @visible_cfs_by_key = visible_cfs_by_key(section) - end +require "spec_helper" - def title - @section.name.presence || I18n.t("settings.user_custom_fields.label_untitled_section") - end +RSpec.describe Users::Form::PreferencesForm, type: :forms do + include_context "with rendered form" - def built_in?(key) - UserCustomFieldSection::BUILT_IN_ATTRIBUTES.include?(key) - end + let(:model) { build_stubbed(:user_preference) } - def visible_custom_field(key) - @visible_cfs_by_key[key] - end - - private - - def visible_cfs_by_key(section) - section.custom_fields.visible(User.current).index_by(&:column_name) - end - end + it "renders the time zone, color mode and keyboard shortcuts fields" do + expect(page).to have_select("Time zone") + expect(page).to have_select("Color mode") + expect(page).to have_field("Disable keyboard shortcuts") end end diff --git a/spec/helpers/users_helper_form_hooks_spec.rb b/spec/helpers/users_helper_form_hooks_spec.rb new file mode 100644 index 000000000000..046a3517e09d --- /dev/null +++ b/spec/helpers/users_helper_form_hooks_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe UsersHelper do + describe "#render_user_form_hooks" do + let(:form) { instance_double(Primer::Forms::Builder) } + let(:user) { build_stubbed(:user) } + + before do + allow(helper).to receive(:call_hook).and_return("".html_safe) + allow(OpenProject::Deprecation).to receive(:warn) + allow(helper).to receive(:fields_for).and_yield(instance_double(TabularFormBuilder)) + end + + it "always calls the new Primer hook" do + allow(OpenProject::Hook).to receive(:hook_listeners).with(:view_users_form).and_return([]) + + helper.render_user_form_hooks(user:, form:) + + expect(helper).to have_received(:call_hook).with(:view_users_primer_form, hash_including(form:)) + end + + context "when no legacy listener is registered" do + before { allow(OpenProject::Hook).to receive(:hook_listeners).with(:view_users_form).and_return([]) } + + it "does not warn and does not render the legacy hook" do + helper.render_user_form_hooks(user:, form:) + + expect(OpenProject::Deprecation).not_to have_received(:warn) + expect(helper).not_to have_received(:fields_for) + end + end + + context "when a legacy listener is registered" do + before { allow(OpenProject::Hook).to receive(:hook_listeners).with(:view_users_form).and_return([instance_double(Object)]) } + + it "logs a deprecation and renders the legacy hook" do + helper.render_user_form_hooks(user:, form:) + + expect(OpenProject::Deprecation).to have_received(:warn) + expect(helper).to have_received(:call_hook).with(:view_users_form, hash_including(:form)) + end + end + end +end diff --git a/spec/views/users/edit.html.erb_spec.rb b/spec/views/users/edit.html.erb_spec.rb index 1b64177c74b1..7e8f8df16e0a 100644 --- a/spec/views/users/edit.html.erb_spec.rb +++ b/spec/views/users/edit.html.erb_spec.rb @@ -43,6 +43,7 @@ assign(:auth_sources, []) assign(:contract, Users::UpdateContract.new(user, current_user)) + User.current = current_user without_partial_double_verification do allow(view).to receive(:current_user).and_return(current_user) end @@ -105,7 +106,7 @@ end context "with password-based login" do - let(:user) { build(:user, id: 42) } + let(:user) { build_stubbed(:user) } context "with password login disabled" do before do From e06580eab575796e7e401ef7b7041ee5a88f9036 Mon Sep 17 00:00:00 2001 From: David F Date: Tue, 16 Jun 2026 09:05:19 +0200 Subject: [PATCH 02/14] Primerize user avatar into edition form. OP-19485 --- app/components/users/form_component.html.erb | 7 - app/forms/users/form/attributes_form.rb | 33 ++++- app/views/users/_general.html.erb | 5 + .../content/modules/_avatars.sass | 46 ++++-- frontend/src/turbo/setup.ts | 1 + .../controllers/avatars/base_controller.rb | 11 +- .../avatars/my_avatar_controller.rb | 4 - .../controllers/avatars/users_controller.rb | 2 +- .../app/views/avatars/my/avatar.html.erb | 6 +- .../app/views/avatars/users/_avatar.html.erb | 8 -- .../avatars/users/_avatar_fields.html.erb | 44 ++++++ .../avatars/users/_avatar_section.html.erb | 8 ++ .../views/avatars/users/_avatar_tab.html.erb | 5 - .../views/avatars/users/_gravatars.html.erb | 16 --- .../avatars/users/_local_avatars.html.erb | 33 ----- modules/avatars/config/locales/en.yml | 18 +-- modules/avatars/config/locales/js-en.yml | 4 - .../avatar-upload-form.component.ts | 15 +- .../avatar-upload-form.html | 48 ++----- .../lib/open_project/avatars/engine.rb | 9 +- .../avatars/lib/open_project/avatars/hooks.rb | 28 ++++ .../controllers/avatars/my_controller_spec.rb | 5 +- .../avatars/users_controller_spec.rb | 10 +- .../avatars/spec/features/my_avatar_spec.rb | 85 ++++++++++- .../spec/features/shared_avatar_examples.rb | 136 ------------------ .../avatars/spec/features/user_avatar_spec.rb | 105 +++++++++++--- 26 files changed, 356 insertions(+), 336 deletions(-) delete mode 100644 modules/avatars/app/views/avatars/users/_avatar.html.erb create mode 100644 modules/avatars/app/views/avatars/users/_avatar_fields.html.erb create mode 100644 modules/avatars/app/views/avatars/users/_avatar_section.html.erb delete mode 100644 modules/avatars/app/views/avatars/users/_avatar_tab.html.erb delete mode 100644 modules/avatars/app/views/avatars/users/_gravatars.html.erb delete mode 100644 modules/avatars/app/views/avatars/users/_local_avatars.html.erb create mode 100644 modules/avatars/lib/open_project/avatars/hooks.rb delete mode 100644 modules/avatars/spec/features/shared_avatar_examples.rb diff --git a/app/components/users/form_component.html.erb b/app/components/users/form_component.html.erb index 09581698f239..a530d65a4500 100644 --- a/app/components/users/form_component.html.erb +++ b/app/components/users/form_component.html.erb @@ -1,10 +1,3 @@ -<% if editing? %> - <%= render(Primer::Box.new(mb: 3)) do %> - <%= render(Primer::Beta::Text.new(tag: :div, font_weight: :bold)) { I18n.t("attributes.status") } %> - <%= render(Primer::Beta::Text.new(tag: :div)) { helpers.full_user_status(@user, true) } %> - <% end %> -<% end %> - <%= render(form_list) %> <% if show_no_login_message? %> diff --git a/app/forms/users/form/attributes_form.rb b/app/forms/users/form/attributes_form.rb index 5fc0d814fa65..401b41c5f9f2 100644 --- a/app/forms/users/form/attributes_form.rb +++ b/app/forms/users/form/attributes_form.rb @@ -36,7 +36,7 @@ class AttributesForm < ApplicationForm include CustomFields::CustomFieldRendering form do |f| - admin_flag(f) if User.current.admin? + account_section(f) user_sections(f) end @@ -52,10 +52,33 @@ def custom_fields @custom_fields ||= @user.available_custom_fields end - def admin_flag(form) - form.check_box(name: :admin, - label: User.human_attribute_name(:admin), - disabled: @user == User.current) + # Built-in account attributes grouped under their own section so they read + # as account settings rather than as part of the avatar block rendered + # above the form. The current status is folded into the section title + # (e.g. "Account: active") for persisted users. + def account_section(form) + return unless show_account_section? + + form.fieldset_group(title: account_title) do |group| + admin_flag(group) if User.current.admin? + end + end + + def show_account_section? + @user.persisted? || User.current.admin? + end + + def account_title + title = I18n.t(:label_account) + return title unless @user.persisted? + + "#{title}: #{helpers.full_user_status(@user, true)}" + end + + def admin_flag(group) + group.check_box(name: :admin, + label: User.human_attribute_name(:admin), + disabled: @user == User.current) end def user_sections(form) diff --git a/app/views/users/_general.html.erb b/app/views/users/_general.html.erb index 02928a5246bf..27cc4c9130ba 100644 --- a/app/views/users/_general.html.erb +++ b/app/views/users/_general.html.erb @@ -27,6 +27,11 @@ See COPYRIGHT and LICENSE files for more details. ++#%> +<%# Constrain hook content (e.g. the avatar section) to the same width as the form below. %> +<%= content_tag(:div, class: "op-admin-settings-form-wrapper") do %> + <%= call_hook(:view_users_general_top, user: @user) %> +<% end %> + <%= error_messages_for @user %> <%= diff --git a/frontend/src/global_styles/content/modules/_avatars.sass b/frontend/src/global_styles/content/modules/_avatars.sass index 6300710453ea..c797d612bf9d 100644 --- a/frontend/src/global_styles/content/modules/_avatars.sass +++ b/frontend/src/global_styles/content/modules/_avatars.sass @@ -1,19 +1,41 @@ -.avatars--current-gravatar, -.avatars--current-local-avatar +.avatars--layout + display: flex + align-items: flex-start + gap: 1.5rem + +.avatars--media + display: flex + flex-direction: column + align-items: center + gap: 0.25rem + flex-shrink: 0 + +.avatars--description + flex: 1 + +.avatars--current-avatar display: inline-block - padding: 10px - border: 1px solid #aaa - img - width: 64px + + .op-avatar height: 64px + width: 64px + line-height: 60px + font-size: 28px -.avatars--local-avatar-preview - width: 128px - height: 128px - border-radius: 50% +.avatars--upload-trigger + cursor: pointer + &:hover + opacity: 0.8 -.avatars--current-gravatar - margin-bottom: 20px +.avatars--file-input + position: absolute + width: 1px + height: 1px + padding: 0 + margin: -1px + overflow: hidden + clip: rect(0, 0, 0, 0) + border: 0 .avatars--error-pane color: var(--content-form-error-color) diff --git a/frontend/src/turbo/setup.ts b/frontend/src/turbo/setup.ts index 84e0363f876e..ece314637758 100644 --- a/frontend/src/turbo/setup.ts +++ b/frontend/src/turbo/setup.ts @@ -61,6 +61,7 @@ TurboPower.register('turbo_frame_set_src', TurboPower.Actions.turbo_frame_set_sr TurboPower.register('redirect_to', TurboPower.Actions.redirect_to, StreamActions); TurboPower.register('set_dataset_attribute', TurboPower.Actions.set_dataset_attribute, StreamActions); TurboPower.register('set_title', TurboPower.Actions.set_title, StreamActions); +TurboPower.register('reload', TurboPower.Actions.reload, StreamActions); // Error handling when "Content missing" returned document.addEventListener('turbo:frame-missing', (event) => { diff --git a/modules/avatars/app/controllers/avatars/base_controller.rb b/modules/avatars/app/controllers/avatars/base_controller.rb index d35d49688775..6b7b5f8a8937 100644 --- a/modules/avatars/app/controllers/avatars/base_controller.rb +++ b/modules/avatars/app/controllers/avatars/base_controller.rb @@ -6,6 +6,7 @@ def update if request.put? result = service_request(type: :update) if result.success? + flash[:notice] = result.result render plain: result.result, status: :ok else render plain: result.errors.full_messages.join(", "), status: :bad_request @@ -18,12 +19,20 @@ def update def destroy if request.delete? result = service_request(type: :destroy) + + # Regular flash (not flash.now): the turbo_stream "reload" response below + # triggers a full page reload, so the message must survive to that request. + # rubocop:disable Rails/ActionControllerFlashBeforeRender if result.success? flash[:notice] = result.result else flash[:error] = result.errors.full_messages.join(", ") end - redirect_to redirect_path, status: :see_other + # rubocop:enable Rails/ActionControllerFlashBeforeRender + + # A full reload (not a Turbo visit) is needed so the browser refetches the + # cached avatar image; the turbo_power "reload" action does just that. + render turbo_stream: turbo_stream.reload else head :method_not_allowed end diff --git a/modules/avatars/app/controllers/avatars/my_avatar_controller.rb b/modules/avatars/app/controllers/avatars/my_avatar_controller.rb index ba6bcda521d1..dc69db2293af 100644 --- a/modules/avatars/app/controllers/avatars/my_avatar_controller.rb +++ b/modules/avatars/app/controllers/avatars/my_avatar_controller.rb @@ -16,10 +16,6 @@ def show private - def redirect_path - edit_my_avatar_path - end - def set_user @user = current_user end diff --git a/modules/avatars/app/controllers/avatars/users_controller.rb b/modules/avatars/app/controllers/avatars/users_controller.rb index ab0f79e212aa..d9c216505e18 100644 --- a/modules/avatars/app/controllers/avatars/users_controller.rb +++ b/modules/avatars/app/controllers/avatars/users_controller.rb @@ -12,7 +12,7 @@ def show private def redirect_path - edit_user_path(@user, tab: "avatar") + edit_user_path(@user) end def find_user diff --git a/modules/avatars/app/views/avatars/my/avatar.html.erb b/modules/avatars/app/views/avatars/my/avatar.html.erb index ed62fa87aa1d..9e41f55aefc9 100644 --- a/modules/avatars/app/views/avatars/my/avatar.html.erb +++ b/modules/avatars/app/views/avatars/my/avatar.html.erb @@ -10,8 +10,4 @@ end %> -<% @manager = ::OpenProject::Avatars::AvatarManager %> -<% @target_avatar_path = edit_my_avatar_path %> -<%= form_tag(edit_my_avatar_path, method: :put, name: "avatar_form", multipart: true) do %> - <%= render partial: "avatars/users/avatar" %> -<% end %> +<%= render "avatars/users/avatar_fields", user: current_user, target_avatar_path: edit_my_avatar_path %> diff --git a/modules/avatars/app/views/avatars/users/_avatar.html.erb b/modules/avatars/app/views/avatars/users/_avatar.html.erb deleted file mode 100644 index 577e56f3035c..000000000000 --- a/modules/avatars/app/views/avatars/users/_avatar.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -<% @manager = ::OpenProject::Avatars::AvatarManager %> -<% if @manager.gravatar_enabled? %> - <%= render partial: "avatars/users/gravatars" %> -<% end %> - -<% if @manager.local_avatars_enabled? %> - <%= render partial: "avatars/users/local_avatars" %> -<% end %> diff --git a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb new file mode 100644 index 000000000000..ed8307c00b4d --- /dev/null +++ b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb @@ -0,0 +1,44 @@ +<%# locals: (user:, target_avatar_path:) -%> +<% manager = ::OpenProject::Avatars::AvatarManager %> + +
+
+ <% if manager.local_avatars_enabled? %> + <%# Clicking the avatar activates the hidden file input rendered by the + component below; the upload then fires as soon as a file is selected. %> + + + <% if local_avatar?(user) %> + <%= form_with( + url: target_avatar_path, + method: :delete, + data: { turbo_confirm: t(:are_you_sure_delete_avatar) } + ) do %> + <%= render( + Primer::Beta::Button.new( + type: :submit, + scheme: :link, + test_selector: "avatar-delete-link" + ) + ) { t(:button_delete) } %> + <% end %> + <% end %> + <% else %> +
+ <%= avatar(user, hover_card: { active: false }) %> +
+ <% end %> +
+ +
+ <% if manager.gravatar_enabled? %> + <%= t("avatars.text_avatar_gravatar_html", gravatar_url: link_to("gravatar.com", "https://gravatar.com")) %> + <% end %> + <% if manager.local_avatars_enabled? %> + <%= t("avatars.text_avatar_local") %> + <% end %> +
+
diff --git a/modules/avatars/app/views/avatars/users/_avatar_section.html.erb b/modules/avatars/app/views/avatars/users/_avatar_section.html.erb new file mode 100644 index 000000000000..492b48f111b7 --- /dev/null +++ b/modules/avatars/app/views/avatars/users/_avatar_section.html.erb @@ -0,0 +1,8 @@ +<% manager = ::OpenProject::Avatars::AvatarManager %> +<% if User.current.admin? && manager.avatars_enabled? %> + <%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> + <% subhead.with_heading(tag: :h2) { t("avatars.label_avatar") } %> + <% end %> + + <%= render "avatars/users/avatar_fields", user:, target_avatar_path: edit_user_avatar_path(user) %> +<% end %> diff --git a/modules/avatars/app/views/avatars/users/_avatar_tab.html.erb b/modules/avatars/app/views/avatars/users/_avatar_tab.html.erb deleted file mode 100644 index 2da01640a9d4..000000000000 --- a/modules/avatars/app/views/avatars/users/_avatar_tab.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<% @manager = ::OpenProject::Avatars::AvatarManager %> -<% @target_avatar_path = edit_user_avatar_path(@user) %> -<%= form_tag(edit_user_avatar_path(@user), method: :put, name: "avatar_form", multipart: true) do %> - <%= render partial: "avatars/users/avatar" %> -<% end %> diff --git a/modules/avatars/app/views/avatars/users/_gravatars.html.erb b/modules/avatars/app/views/avatars/users/_gravatars.html.erb deleted file mode 100644 index b2da6da97db5..000000000000 --- a/modules/avatars/app/views/avatars/users/_gravatars.html.erb +++ /dev/null @@ -1,16 +0,0 @@ -
- <%= t "avatars.label_gravatar" %> -

- <%= t "avatars.text_your_current_gravatar" %> -

-
- <%= avatar @user %> -
- - <% if @user == current_user %> -

- <%= t "note" %>: - <%= t("avatars.text_change_gravatar_html", gravatar_url: link_to("gravatar.com", "https://www.gravatar.com")) %> -

- <% end %> -
diff --git a/modules/avatars/app/views/avatars/users/_local_avatars.html.erb b/modules/avatars/app/views/avatars/users/_local_avatars.html.erb deleted file mode 100644 index 07972e382a63..000000000000 --- a/modules/avatars/app/views/avatars/users/_local_avatars.html.erb +++ /dev/null @@ -1,33 +0,0 @@ -
- <%= t "avatars.label_local_avatar" %> -

- <%= t "avatars.text_your_local_avatar" %> - <% if @manager.gravatar_enabled? %> -
- <%= t "avatars.text_local_avatar_over_gravatar" %> - <% end %> -

-
- -
-
-
- <% if local_avatar? @user %> - <%= avatar(@user) %> - <% else %> - <%= t :label_none %> - <% end %> -
- <% if local_avatar? @user %> - <%= link_to t(:button_delete), - @target_avatar_path, - class: "avatars--local-avatar-delete-link", - data: { turbo_method: :delete, turbo_confirm: t(:are_you_sure_delete_avatar) } %> - <% end %> -
-
-
- -
diff --git a/modules/avatars/config/locales/en.yml b/modules/avatars/config/locales/en.yml index 4d8bb1fbce49..d240c3c83b9a 100644 --- a/modules/avatars/config/locales/en.yml +++ b/modules/avatars/config/locales/en.yml @@ -23,21 +23,11 @@ en: avatars: label_avatar: "Avatar" label_gravatar: 'Gravatar' - label_current_avatar: 'Current avatar' label_local_avatar: 'Custom avatar' - text_current_avatar: | - The following image shows the current avatar. - text_upload_instructions: | - Upload your own custom avatar of 128 by 128 pixels. Larger files will be resized and cropped to match. - A preview of your avatar will be shown before uploading, once you selected an image. - text_change_gravatar_html: 'To change or add the Gravatar for your mail address, go to %{gravatar_url}.' - text_your_local_avatar: | - OpenProject allows you to upload your own custom avatar. - text_local_avatar_over_gravatar: | - If you set one, this custom avatar is used in precedence over the gravatar above. - text_your_current_gravatar: | - OpenProject uses your gravatar if you registered one, or a default image or icon if one exists. - The current gravatar is as follows: + text_avatar_gravatar_html: >- + OpenProject uses your Gravatar if you registered one at %{gravatar_url}, or a default image or icon if one exists. + text_avatar_local: >- + You can also upload a custom avatar that overrides the Gravatar by clicking the image. We recommend an image of 128 by 128 pixels. settings: enable_gravatars: 'Enable user gravatars' gravatar_default: "Default Gravatar image" diff --git a/modules/avatars/config/locales/js-en.yml b/modules/avatars/config/locales/js-en.yml index f8c3d3d64884..d1c634d250bf 100644 --- a/modules/avatars/config/locales/js-en.yml +++ b/modules/avatars/config/locales/js-en.yml @@ -7,10 +7,6 @@ en: avatars: label_choose_avatar: "Choose Avatar from file" uploading_avatar: "Uploading your avatar." - text_upload_instructions: | - Upload your own custom avatar of 128 by 128 pixels. Larger files will be resized and cropped to match. - A preview of your avatar will be shown before uploading, once you selected an image. - error_image_too_large: "Image is too large." wrong_file_format: "Allowed formats are jpg, png, gif" empty_file_error: "Please upload a valid image (jpg, png, gif)" diff --git a/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.component.ts b/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.component.ts index 1ddb98d6269b..6faf9256aa8c 100644 --- a/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.component.ts +++ b/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.component.ts @@ -50,16 +50,12 @@ export class AvatarUploadFormComponent implements OnInit { protected toastService = inject(ToastService); protected uploadService = inject(OpUploadService); - public form:any; - public target:string; public method:string; public avatarFile:File; - public avatarPreviewUrl:string; - public busy = false; public fileInvalid = false; @@ -69,12 +65,9 @@ export class AvatarUploadFormComponent implements OnInit { // Text public text = { label_choose_avatar: this.I18n.t('js.avatars.label_choose_avatar'), - upload_instructions: this.I18n.t('js.avatars.text_upload_instructions'), error_too_large: this.I18n.t('js.avatars.error_image_too_large'), wrong_file_format: this.I18n.t('js.avatars.wrong_file_format'), - button_update: this.I18n.t('js.button_update'), uploading: this.I18n.t('js.avatars.uploading_avatar'), - preview: this.I18n.t('js.label_preview'), }; public ngOnInit() { @@ -96,17 +89,15 @@ export class AvatarUploadFormComponent implements OnInit { return; } - void resizeFile(128, file).then(([dataURL, blob]) => { - // Create resized file + void resizeFile(128, file).then(([, blob]) => { this.avatarFile = new File([blob], file.name); - this.avatarPreviewUrl = dataURL; this.fileInvalid = false; this.cdRef.detectChanges(); + this.uploadAvatar(); }); } - public uploadAvatar(event:Event) { - event.preventDefault(); + public uploadAvatar() { this.busy = true; const uploadFile:AvatarUploadFile = { file: this.avatarFile, method: this.method }; const observable = this.uploadService.upload(this.target, [uploadFile])[0]; diff --git a/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html b/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html index 7e68d649fa7a..8df6262bb865 100644 --- a/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html +++ b/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html @@ -1,36 +1,14 @@ -
- -
- -
-
- -
- @if (fileInvalid) { - - } -
-
+ +
+ @if (fileInvalid) { + + }
-@if (avatarPreviewUrl) { -
- - {{ text.preview }} -
-} - - diff --git a/modules/avatars/lib/open_project/avatars/engine.rb b/modules/avatars/lib/open_project/avatars/engine.rb index 05f0537e71aa..2c47ff944f85 100644 --- a/modules/avatars/lib/open_project/avatars/engine.rb +++ b/modules/avatars/lib/open_project/avatars/engine.rb @@ -45,11 +45,8 @@ class Engine < ::Rails::Engine mount ::API::V3::Users::UserAvatarAPI end - add_tab_entry :user, - name: "avatar", - partial: "avatars/users/avatar_tab", - path: ->(params) { edit_user_path(params[:user], tab: :avatar) }, - label: :label_avatar, - only_if: ->(*) { User.current.admin? && ::OpenProject::Avatars::AvatarManager.avatars_enabled? } + config.to_prepare do + OpenProject::Avatars::Hooks + end end end diff --git a/modules/avatars/lib/open_project/avatars/hooks.rb b/modules/avatars/lib/open_project/avatars/hooks.rb new file mode 100644 index 000000000000..7ba3374f9fea --- /dev/null +++ b/modules/avatars/lib/open_project/avatars/hooks.rb @@ -0,0 +1,28 @@ +# OpenProject Avatars plugin +# +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +module OpenProject + module Avatars + class Hooks < ::OpenProject::Hook::ViewListener + # Renders the avatar management section at the top of the user + # administration "Details" tab, where it used to live in its own tab. + render_on :view_users_general_top, + partial: "avatars/users/avatar_section" + end + end +end diff --git a/modules/avatars/spec/controllers/avatars/my_controller_spec.rb b/modules/avatars/spec/controllers/avatars/my_controller_spec.rb index aafde77f4e26..44e037096a35 100644 --- a/modules/avatars/spec/controllers/avatars/my_controller_spec.rb +++ b/modules/avatars/spec/controllers/avatars/my_controller_spec.rb @@ -85,7 +85,7 @@ delete :destroy expect(flash[:notice]).to include "message" expect(flash[:error]).not_to be_present - expect(response).to redirect_to controller.send :redirect_path + expect(response.body).to include 'action="reload"' end it "calls the service for delete" do @@ -97,10 +97,9 @@ .and_return(result) delete :destroy - expect(response).not_to be_successful expect(flash[:notice]).not_to be_present expect(flash[:error]).to include "error" - expect(response).to redirect_to controller.send :redirect_path + expect(response.body).to include 'action="reload"' end end end diff --git a/modules/avatars/spec/controllers/avatars/users_controller_spec.rb b/modules/avatars/spec/controllers/avatars/users_controller_spec.rb index 7dd50e651552..a2fac9966ad9 100644 --- a/modules/avatars/spec/controllers/avatars/users_controller_spec.rb +++ b/modules/avatars/spec/controllers/avatars/users_controller_spec.rb @@ -23,7 +23,7 @@ end it "renders the edit action" do - expect(response).to redirect_to edit_user_path(target_user, tab: "avatar") + expect(response).to redirect_to edit_user_path(target_user) end end @@ -70,11 +70,12 @@ it "calls the service for put" do expect_any_instance_of(Avatars::UpdateService) .to receive(:replace) - .and_return(ServiceResult.success) + .and_return(ServiceResult.success(result: "Avatar changed successfully.")) put :update, params: { id: target_user.id } expect(response).to be_successful expect(response).to have_http_status :ok + expect(flash[:notice]).to eq("Avatar changed successfully.") end it "calls the service for put" do @@ -114,7 +115,7 @@ delete :destroy, params: { id: target_user.id } expect(flash[:notice]).to include "message" expect(flash[:error]).not_to be_present - expect(response).to redirect_to controller.send :redirect_path + expect(response.body).to include 'action="reload"' end it "calls the service for delete" do @@ -126,10 +127,9 @@ .and_return(result) delete :destroy, params: { id: target_user.id } - expect(response).not_to be_successful expect(flash[:notice]).not_to be_present expect(flash[:error]).to include "error" - expect(response).to redirect_to controller.send :redirect_path + expect(response.body).to include 'action="reload"' end end end diff --git a/modules/avatars/spec/features/my_avatar_spec.rb b/modules/avatars/spec/features/my_avatar_spec.rb index 5724b0bd0102..8095eea1f7e3 100644 --- a/modules/avatars/spec/features/my_avatar_spec.rb +++ b/modules/avatars/spec/features/my_avatar_spec.rb @@ -1,18 +1,96 @@ +# frozen_string_literal: true + require "spec_helper" -require_relative "shared_avatar_examples" +require "mini_magick" RSpec.describe "My avatar management", :js do include Rails.application.routes.url_helpers + let(:image_base_path) { File.expand_path("#{File.dirname(__FILE__)}/../fixtures/") } let(:user) { create(:user) } - let(:target_user) { user } let(:avatar_management_path) { edit_my_avatar_path } + let(:enable_gravatars) { false } + let(:enable_local_avatars) { false } + let(:plugin_settings) do + { + "enable_gravatars" => enable_gravatars, + "enable_local_avatars" => enable_local_avatars + } + end + before do login_as user + allow(Setting) + .to receive(:plugin_openproject_avatars) + .and_return(plugin_settings) + end + + describe "only gravatars enabled" do + let(:enable_gravatars) { true } + + it "shows the Gravatar hint and the current avatar but no upload field" do + visit avatar_management_path + + expect(page).to have_css(".avatars--current-avatar") + expect(page).to have_link("gravatar.com") + expect(page).to have_no_field("avatar_file_input") + end + end + + describe "only local avatars enabled" do + let(:enable_local_avatars) { true } + + it "can upload and delete a custom avatar" do + visit avatar_management_path + expect(page).to have_css(".avatars--upload-trigger") + + # Gravatar hint is not rendered + expect(page).to have_no_link("gravatar.com") + + # Attach a new invalid image + attach_file("avatar_file_input", + UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path, + make_visible: true) + + # Expect error + expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") + + # Attach a valid image; it is uploaded as soon as it is selected + visit avatar_management_path + attach_file("avatar_file_input", + UploadedFile.load_from(File.join(image_base_path, "too_big.jpg")).path, + make_visible: true) + + # Expect the avatar to be uploaded and resized + expect(page).to have_test_selector("avatar-delete-link", wait: 20) + avatar_path = user.local_avatar_attachment.file.path + content_type = OpenProject::ContentTypeDetector.new(avatar_path).detect + image = MiniMagick::Image.open(avatar_path) + + expect(image.dimensions).to eq [128, 128] + expect(content_type).to eq("image/jpeg") + + # Delete the avatar + accept_alert do + find_test_selector("avatar-delete-link").click + end + + expect(page).to have_no_test_selector("avatar-delete-link", wait: 20) + end end - it_behaves_like "avatar management" + describe "both gravatars and local avatars enabled" do + let(:enable_gravatars) { true } + let(:enable_local_avatars) { true } + + it "renders the Gravatar hint and the upload trigger in a single section" do + visit avatar_management_path + + expect(page).to have_link("gravatar.com") + expect(page).to have_css(".avatars--upload-trigger") + end + end describe "none enabled" do before do @@ -26,6 +104,7 @@ expect(page).to have_text "[Error 404]" visit my_account_path + expect(page).to have_text(I18n.t(:label_my_account)) expect(page).to have_no_css ".avatar-menu-item" end end diff --git a/modules/avatars/spec/features/shared_avatar_examples.rb b/modules/avatars/spec/features/shared_avatar_examples.rb deleted file mode 100644 index a238388896d7..000000000000 --- a/modules/avatars/spec/features/shared_avatar_examples.rb +++ /dev/null @@ -1,136 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ -require "mini_magick" - -RSpec.shared_examples "avatar management" do - let(:image_base_path) { File.expand_path(File.dirname(__FILE__) + "/../fixtures/") } - - let(:enable_gravatars) { false } - let(:enable_local_avatars) { false } - let(:plugin_settings) do - { - "enable_gravatars" => enable_gravatars, - "enable_local_avatars" => enable_local_avatars - } - end - - before do - allow(Setting) - .to receive(:plugin_openproject_avatars) - .and_return(plugin_settings) - end - - describe "only gravatars enabled" do - let(:enable_gravatars) { true } - - it "shows the gravatar avatar" do - visit avatar_management_path - - expect(page).to have_css(".form--fieldset-legend", text: "GRAVATAR") - expect(page).to have_css(".avatars--current-gravatar") - - # Local not rendered - expect(page).to have_no_css(".form--fieldset-legend", text: "CUSTOM AVATAR") - expect(page).to have_no_css(".avatars--current-local-avatar", text: "none") - end - end - - describe "only local avatars enabled" do - let(:enable_local_avatars) { true } - - it "can upload a new image" do - visit avatar_management_path - expect(page).to have_css(".form--fieldset-legend", text: "CUSTOM AVATAR") - expect(page).to have_css(".avatars--current-local-avatar", text: "none") - - # Gravatars not rendered - expect(page).to have_no_css(".form--fieldset-legend", text: "GRAVATAR") - - # Attach a new invalid image - find_by_id("avatar_file_input").set UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path - - # Expect error - expect(page).to have_css(".form--label.-error") - expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") - - # Attach new image - visit avatar_management_path - expect(page).to have_css(".avatars--current-local-avatar", text: "none") - find_by_id("avatar_file_input").set UploadedFile.load_from(File.join(image_base_path, "too_big.jpg")).path - - # Expect not error, since ng-file-upload resizes the image - expect(page).to have_no_css(".form--label.-error") - expect(page).to have_no_css(".avatars--error-pane span") - - # Expect preview - expect(page).to have_css(".preview img") - - # Click button - click_on "Update" - - # Expect avatar rendered - expect(page).to have_css(".form--fieldset-legend", text: "CUSTOM AVATAR") - avatar_tag = find(".avatars--current-local-avatar img") - expect(avatar_tag[:src]).to include user_avatar_path(target_user) - - # Expect the avatar to be resized - avatar_path = target_user.local_avatar_attachment.file.path - content_type = OpenProject::ContentTypeDetector.new(avatar_path).detect - image = MiniMagick::Image.open(avatar_path) - - expect(image.dimensions).to eq [128, 128] - expect(content_type).to eq("image/jpeg") - - # Delete the avatar - accept_alert do - find(".avatars--local-avatar-delete-link").click - end - - expect(page).to have_css(".avatars--current-local-avatar", text: "none", wait: 20) - end - end - - describe "both local avatars enabled" do - let(:enable_gravatars) { true } - let(:enable_local_avatars) { true } - - it "renders both sections" do - visit avatar_management_path - - # Gravatar - expect(page).to have_css(".form--fieldset-legend", text: "GRAVATAR") - expect(page).to have_css(".avatars--current-gravatar") - - # Local - expect(page).to have_css(".form--fieldset-legend", text: "CUSTOM AVATAR") - expect(page).to have_css(".avatars--current-local-avatar", text: "none") - end - end -end diff --git a/modules/avatars/spec/features/user_avatar_spec.rb b/modules/avatars/spec/features/user_avatar_spec.rb index 8259b67d33c1..1262d1d00f00 100644 --- a/modules/avatars/spec/features/user_avatar_spec.rb +++ b/modules/avatars/spec/features/user_avatar_spec.rb @@ -1,20 +1,98 @@ +# frozen_string_literal: true + require "spec_helper" -require_relative "shared_avatar_examples" RSpec.describe "User avatar management", :js do include Rails.application.routes.url_helpers - let(:user) { create(:admin) } - let(:avatar_management_path) { edit_user_path(target_user, tab: "avatar") } + let(:image_base_path) { File.expand_path("#{File.dirname(__FILE__)}/../fixtures/") } + let(:avatar_management_path) { edit_user_path(target_user) } + + let(:enable_gravatars) { false } + let(:enable_local_avatars) { false } + let(:plugin_settings) do + { + "enable_gravatars" => enable_gravatars, + "enable_local_avatars" => enable_local_avatars + } + end before do login_as user + allow(Setting) + .to receive(:plugin_openproject_avatars) + .and_return(plugin_settings) end context "when user is admin" do + let(:user) { create(:admin) } let(:target_user) { create(:user) } - it_behaves_like "avatar management" + describe "only gravatars enabled" do + let(:enable_gravatars) { true } + + it "shows the avatar section with the Gravatar hint but no upload field" do + visit avatar_management_path + + expect(page).to have_css("h2", text: "Avatar") + expect(page).to have_css(".avatars--current-avatar") + expect(page).to have_css(".avatars--description", text: "Gravatar") + expect(page).to have_no_field("avatar_file_input") + end + end + + describe "only local avatars enabled" do + let(:enable_local_avatars) { true } + + it "exposes the avatar as an upload trigger and validates the file format" do + visit avatar_management_path + expect(page).to have_css("h2", text: "Avatar") + expect(page).to have_css(".avatars--upload-trigger") + + # Gravatar hint is not rendered + expect(page).to have_no_link("gravatar.com") + + # The upload component is active on this tab and rejects invalid files + attach_file("avatar_file_input", + UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path, + make_visible: true) + + expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") + end + + it "offers to delete an existing custom avatar" do + target_user.attachments = [build(:avatar_attachment, author: target_user)] + + visit avatar_management_path + + accept_alert do + find_test_selector("avatar-delete-link").click + end + + expect(page).to have_no_test_selector("avatar-delete-link", wait: 20) + end + end + + describe "both gravatars and local avatars enabled" do + let(:enable_gravatars) { true } + let(:enable_local_avatars) { true } + + it "renders the Gravatar hint and the upload trigger in a single section" do + visit avatar_management_path + + expect(page).to have_css("h2", text: "Avatar") + expect(page).to have_css(".avatars--description", text: "Gravatar") + expect(page).to have_css(".avatars--upload-trigger") + end + end + + describe "none enabled" do + it "does not render the avatar section" do + visit avatar_management_path + expect(page).to have_button(I18n.t(:button_save)) + expect(page).to have_no_css("h2", text: "Avatar") + end + end end context "when user is self" do @@ -22,7 +100,7 @@ let(:target_user) { user } it "forbids the user to access" do - visit avatar_management_path + visit edit_user_path(target_user) expect(page).to have_text("[Error 403]") end end @@ -32,23 +110,8 @@ let(:user) { create(:user) } it "forbids the user to access" do - visit avatar_management_path + visit edit_user_path(target_user) expect(page).to have_text("[Error 403]") end end - - describe "none enabled" do - let(:target_user) { create(:user) } - - before do - allow(Setting) - .to receive(:plugin_openproject_avatars) - .and_return({}) - end - - it "does not render the user edit tab" do - visit edit_user_path(user) - expect(page).to have_no_css "#tab-avatar" - end - end end From 892a3dbd87c97f603af981a0670008a54b97c7a9 Mon Sep 17 00:00:00 2001 From: David F Date: Tue, 16 Jun 2026 11:17:04 +0200 Subject: [PATCH 03/14] Refactor my account settings to display avatar and sections. OP-19485 --- app/controllers/my_controller.rb | 15 +++- .../base/autocomplete/multi_value_input.rb | 4 +- .../base/autocomplete/single_value_input.rb | 4 +- app/forms/custom_fields/inputs/base/utils.rb | 4 +- app/forms/my/account_form.rb | 89 ------------------- app/forms/my/attributes_form.rb | 38 ++++++++ app/forms/my/custom_fields_form.rb | 45 ---------- app/forms/users/form/attributes_form.rb | 17 ++-- app/views/my/account.html.erb | 8 +- .../avatars/my_avatar_controller.rb | 10 +-- .../views/avatars/my/_avatar_section.html.erb | 8 ++ .../app/views/avatars/my/avatar.html.erb | 13 --- modules/avatars/config/routes.rb | 2 +- .../lib/open_project/avatars/engine.rb | 8 +- .../avatars/lib/open_project/avatars/hooks.rb | 7 ++ .../controllers/avatars/my_controller_spec.rb | 11 --- .../avatars/spec/features/my_avatar_spec.rb | 10 +-- spec/controllers/my_controller_spec.rb | 17 ++++ spec/features/users/my_spec.rb | 18 ++++ .../inputs/single_select_list_spec.rb | 12 +++ spec/forms/my/attributes_form_spec.rb | 52 +++++++++++ spec/forms/users/form/attributes_form_spec.rb | 12 +++ 22 files changed, 212 insertions(+), 192 deletions(-) delete mode 100644 app/forms/my/account_form.rb create mode 100644 app/forms/my/attributes_form.rb delete mode 100644 app/forms/my/custom_fields_form.rb create mode 100644 modules/avatars/app/views/avatars/my/_avatar_section.html.erb delete mode 100644 modules/avatars/app/views/avatars/my/avatar.html.erb create mode 100644 spec/forms/my/attributes_form_spec.rb diff --git a/app/controllers/my_controller.rb b/app/controllers/my_controller.rb index c226fe181290..983e62eae39c 100644 --- a/app/controllers/my_controller.rb +++ b/app/controllers/my_controller.rb @@ -185,7 +185,20 @@ def user_params # The Users::UpdateService updates the user's pref using the UserPreferences::UpdateService # which has a contract/schema applied to the values which is why it is ok # to blindly allow all scalar values in pref. - permitted_params.user.to_h.merge(params.permit(pref: {})) + attributes = permitted_params.user.to_h.merge(params.permit(pref: {})) + drop_non_editable_custom_field_values(attributes) + end + + # On the self-service account page only custom fields the user is allowed to + # edit themselves (editable: true) may be changed. Drop the rest so a crafted + # request cannot persist values the UI renders read-only. + def drop_non_editable_custom_field_values(attributes) + values = attributes["custom_field_values"] + return attributes if values.blank? + + editable_ids = current_user.available_custom_fields.select(&:editable?).map { |cf| cf.id.to_s } + attributes["custom_field_values"] = values.slice(*editable_ids) + attributes end def update_global_notification_setting(update_params) diff --git a/app/forms/custom_fields/inputs/base/autocomplete/multi_value_input.rb b/app/forms/custom_fields/inputs/base/autocomplete/multi_value_input.rb index a635482ff44f..e490a64ba972 100644 --- a/app/forms/custom_fields/inputs/base/autocomplete/multi_value_input.rb +++ b/app/forms/custom_fields/inputs/base/autocomplete/multi_value_input.rb @@ -40,12 +40,14 @@ def input_attributes end def autocomplete_options - { + opts = { multiple: true, decorated: decorated?, focusDirectly: false, append_to: } + opts[:disabled] = true if options[:disabled] + opts end def decorated? diff --git a/app/forms/custom_fields/inputs/base/autocomplete/single_value_input.rb b/app/forms/custom_fields/inputs/base/autocomplete/single_value_input.rb index 5a0466e2defb..bdeec34e42d7 100644 --- a/app/forms/custom_fields/inputs/base/autocomplete/single_value_input.rb +++ b/app/forms/custom_fields/inputs/base/autocomplete/single_value_input.rb @@ -40,12 +40,14 @@ def input_attributes end def autocomplete_options - { + opts = { multiple: false, decorated: decorated?, focusDirectly: false, append_to: } + opts[:disabled] = true if options[:disabled] + opts end def decorated? diff --git a/app/forms/custom_fields/inputs/base/utils.rb b/app/forms/custom_fields/inputs/base/utils.rb index 77b40cd20efe..774667774727 100644 --- a/app/forms/custom_fields/inputs/base/utils.rb +++ b/app/forms/custom_fields/inputs/base/utils.rb @@ -32,7 +32,7 @@ module CustomFields::Inputs::Base::Utils delegate :attribute_name, to: :@custom_field def base_input_attributes - { + attributes = { name:, label:, value:, @@ -41,6 +41,8 @@ def base_input_attributes validation_message:, help_text_options: { attribute_name: } } + attributes[:disabled] = true if options[:disabled] + attributes end def name diff --git a/app/forms/my/account_form.rb b/app/forms/my/account_form.rb deleted file mode 100644 index fae9f5ae269f..000000000000 --- a/app/forms/my/account_form.rb +++ /dev/null @@ -1,89 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -class My::AccountForm < ApplicationForm - form do |f| - f.text_field( - name: :username, - label: User.human_attribute_name(:login), - input_width: :small, - value: @user.login, - readonly: disabled?(:login) - ) - - f.text_field( - name: :firstname, - label: User.human_attribute_name(:firstname), - input_width: :small, - readonly: disabled?(:firstname), - caption: disabled_caption(:firstname), - required: true, - autocomplete: "given-name" - ) - - f.text_field( - name: :lastname, - label: User.human_attribute_name(:lastname), - input_width: :small, - readonly: disabled?(:lastname), - caption: disabled_caption(:lastname), - required: true, - autocomplete: "family-name" - ) - - f.text_field( - name: :mail, - type: :email, - label: User.human_attribute_name(:mail), - input_width: :small, - readonly: disabled?(:mail), - caption: disabled_caption(:mail), - required: true, - autocomplete: "email" - ) - end - - def initialize(user:) - super() - @user = user - - @contract = Users::UpdateContract.new(@user, User.current) - end - - def disabled?(attribute) - !@contract.writable?(attribute) - end - - def disabled_caption(attribute) - return nil if @contract.writable?(attribute) - - I18n.t("user.text_change_disabled_for_provider_login") - end -end diff --git a/app/forms/my/attributes_form.rb b/app/forms/my/attributes_form.rb new file mode 100644 index 000000000000..90945321994d --- /dev/null +++ b/app/forms/my/attributes_form.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +# Self-service variant of the administration user attributes form. Renders the +# same section-grouped built-in + custom fields, but without the admin-only +# "Account" section, and shows non-writable built-ins read-only (with the +# provider-login caption) rather than disabled. +class My::AttributesForm < Users::Form::AttributesForm + # The parent stores its `form do` block in a (non-inherited) class instance + # variable, so the block must be re-declared here. Self-service renders only + # the custom-field sections, never the admin-only account section. + form do |f| + user_sections(f) + end + + def initialize(user:) + super(user:, contract: Users::UpdateContract.new(user, User.current)) + end + + private + + def editability(key) + return {} if @contract.writable?(key.to_sym) + + options = { readonly: true } + if %w[firstname lastname mail].include?(key.to_s) + options[:caption] = I18n.t("user.text_change_disabled_for_provider_login") + end + options + end + + # Custom fields a user may not edit themselves (editable: false) are shown + # read-only on their own account; admins manage those on the admin user form. + def form_arguments(custom_field) + args = super + args[:disabled] = true unless custom_field.editable? + args + end +end diff --git a/app/forms/my/custom_fields_form.rb b/app/forms/my/custom_fields_form.rb deleted file mode 100644 index d18faacbea15..000000000000 --- a/app/forms/my/custom_fields_form.rb +++ /dev/null @@ -1,45 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module My - class CustomFieldsForm < ApplicationForm - include CustomFields::CustomFieldRendering - - form do |f| - render_custom_fields(form: f) - end - - private - - def custom_fields - @custom_fields ||= model.available_custom_fields - end - end -end diff --git a/app/forms/users/form/attributes_form.rb b/app/forms/users/form/attributes_form.rb index 401b41c5f9f2..1b8b6d482940 100644 --- a/app/forms/users/form/attributes_form.rb +++ b/app/forms/users/form/attributes_form.rb @@ -105,22 +105,22 @@ def section_title(section) section.name.presence || I18n.t("settings.user_custom_fields.label_untitled_section") end - def render_built_in(group, key) # rubocop:disable Metrics/AbcSize + def render_built_in(group, key) case key when "firstname", "lastname", "mail" group.text_field(name: key.to_sym, label: User.human_attribute_name(key), required: true, - disabled: !@contract.writable?(key.to_sym), - input_width: :medium) + input_width: :medium, + **editability(key)) when "login" return if @user.new_record? group.text_field(name: :login, label: User.human_attribute_name(:login), required: true, - disabled: !@contract.writable?(:login), - input_width: :medium) + input_width: :medium, + **editability(:login)) when "language" group.select_list(name: :language, label: User.human_attribute_name(:language), @@ -130,6 +130,13 @@ def render_built_in(group, key) # rubocop:disable Metrics/AbcSize end end end + + # Editability options for a built-in text field. Administration disables + # non-writable attributes; self-service overrides this to render them + # read-only with an explanatory caption instead. + def editability(key) + { disabled: !@contract.writable?(key.to_sym) } + end end end end diff --git a/app/views/my/account.html.erb b/app/views/my/account.html.erb index 082dcdc99308..917b27ceed0b 100644 --- a/app/views/my/account.html.erb +++ b/app/views/my/account.html.erb @@ -57,6 +57,11 @@ See COPYRIGHT and LICENSE files for more details. end %> +<%# Constrain hook content (the avatar section) to the same width as the form below. %> +<%= content_tag(:div, class: "op-admin-settings-form-wrapper") do %> + <%= call_hook(:view_my_account_top, user: current_user) %> +<% end %> + <%= error_messages_for "user" %> <%= @@ -71,8 +76,7 @@ See COPYRIGHT and LICENSE files for more details. ) do |form| render( Primer::Forms::FormList.new( - My::AccountForm.new(form, user: @user), - My::CustomFieldsForm.new(form), + My::AttributesForm.new(form, user: @user), My::AccountSubmitForm.new(form) ) ) diff --git a/modules/avatars/app/controllers/avatars/my_avatar_controller.rb b/modules/avatars/app/controllers/avatars/my_avatar_controller.rb index dc69db2293af..963a0f182c64 100644 --- a/modules/avatars/app/controllers/avatars/my_avatar_controller.rb +++ b/modules/avatars/app/controllers/avatars/my_avatar_controller.rb @@ -3,17 +3,9 @@ class MyAvatarController < BaseController before_action :require_login before_action :set_user - no_authorization_required! :show, - :update, + no_authorization_required! :update, :destroy - layout "my" - menu_item :avatar - - def show - render "avatars/my/avatar" - end - private def set_user diff --git a/modules/avatars/app/views/avatars/my/_avatar_section.html.erb b/modules/avatars/app/views/avatars/my/_avatar_section.html.erb new file mode 100644 index 000000000000..c536d252f4b4 --- /dev/null +++ b/modules/avatars/app/views/avatars/my/_avatar_section.html.erb @@ -0,0 +1,8 @@ +<% manager = ::OpenProject::Avatars::AvatarManager %> +<% if manager.avatars_enabled? %> + <%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> + <% subhead.with_heading(tag: :h2) { t("avatars.label_avatar") } %> + <% end %> + + <%= render "avatars/users/avatar_fields", user:, target_avatar_path: edit_my_avatar_path %> +<% end %> diff --git a/modules/avatars/app/views/avatars/my/avatar.html.erb b/modules/avatars/app/views/avatars/my/avatar.html.erb deleted file mode 100644 index 9e41f55aefc9..000000000000 --- a/modules/avatars/app/views/avatars/my/avatar.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -<% html_title t(:label_my_account), t(:label_avatar) %> - -<%= - render(Primer::OpenProject::PageHeader.new) do |header| - header.with_title { t(:label_avatar) } - header.with_breadcrumbs( - [{ href: my_account_path, text: t(:label_my_account) }, - t(:label_avatar)] - ) - end -%> - -<%= render "avatars/users/avatar_fields", user: current_user, target_avatar_path: edit_my_avatar_path %> diff --git a/modules/avatars/config/routes.rb b/modules/avatars/config/routes.rb index 29da5f695560..bfa133f03a36 100644 --- a/modules/avatars/config/routes.rb +++ b/modules/avatars/config/routes.rb @@ -1,7 +1,7 @@ Rails.application.routes.draw do # Update my avatar scope "my" do - resource :avatar, controller: "avatars/my_avatar", as: "edit_my_avatar", only: %i[show update destroy] + resource :avatar, controller: "avatars/my_avatar", as: "edit_my_avatar", only: %i[update destroy] end # Get the current avatar diff --git a/modules/avatars/lib/open_project/avatars/engine.rb b/modules/avatars/lib/open_project/avatars/engine.rb index 2c47ff944f85..891ed3d85565 100644 --- a/modules/avatars/lib/open_project/avatars/engine.rb +++ b/modules/avatars/lib/open_project/avatars/engine.rb @@ -33,13 +33,7 @@ class Engine < ::Rails::Engine breadcrumb_elements: -> { [{ href: admin_settings_users_path, text: I18n.t(:label_user_and_permission) }] }, menu_item: :user_avatars }, - bundled: true do - add_menu_item :my_menu, :avatar, - { controller: "/avatars/my_avatar", action: "show" }, - caption: ->(*) { I18n.t("avatars.label_avatar") }, - if: ->(*) { ::OpenProject::Avatars::AvatarManager::avatars_enabled? }, - icon: "image" - end + bundled: true add_api_endpoint "API::V3::Users::UsersAPI", :id do mount ::API::V3::Users::UserAvatarAPI diff --git a/modules/avatars/lib/open_project/avatars/hooks.rb b/modules/avatars/lib/open_project/avatars/hooks.rb index 7ba3374f9fea..a1dd88cc4390 100644 --- a/modules/avatars/lib/open_project/avatars/hooks.rb +++ b/modules/avatars/lib/open_project/avatars/hooks.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # OpenProject Avatars plugin # # Copyright (C) the OpenProject GmbH @@ -23,6 +25,11 @@ class Hooks < ::OpenProject::Hook::ViewListener # administration "Details" tab, where it used to live in its own tab. render_on :view_users_general_top, partial: "avatars/users/avatar_section" + + # Renders the avatar management section at the top of the my/account page, + # where it replaces the former standalone "Avatar" my-menu page. + render_on :view_my_account_top, + partial: "avatars/my/avatar_section" end end end diff --git a/modules/avatars/spec/controllers/avatars/my_controller_spec.rb b/modules/avatars/spec/controllers/avatars/my_controller_spec.rb index 44e037096a35..570c7f6630fc 100644 --- a/modules/avatars/spec/controllers/avatars/my_controller_spec.rb +++ b/modules/avatars/spec/controllers/avatars/my_controller_spec.rb @@ -13,17 +13,6 @@ allow(OpenProject::Avatars::AvatarManager).to receive(:avatars_enabled?).and_return enabled end - describe "#show" do - before do - get :show - end - - it "renders the edit action" do - expect(response).to be_successful - expect(response).to render_template "avatars/my/avatar" - end - end - describe "#update" do context "when not logged in" do let(:user) { User.anonymous } diff --git a/modules/avatars/spec/features/my_avatar_spec.rb b/modules/avatars/spec/features/my_avatar_spec.rb index 8095eea1f7e3..9d69cb48cdf9 100644 --- a/modules/avatars/spec/features/my_avatar_spec.rb +++ b/modules/avatars/spec/features/my_avatar_spec.rb @@ -8,7 +8,7 @@ let(:image_base_path) { File.expand_path("#{File.dirname(__FILE__)}/../fixtures/") } let(:user) { create(:user) } - let(:avatar_management_path) { edit_my_avatar_path } + let(:avatar_management_path) { my_account_path } let(:enable_gravatars) { false } let(:enable_local_avatars) { false } @@ -99,13 +99,11 @@ .and_return({}) end - it "renders 404 when visiting and does not render the menu item" do - visit edit_my_avatar_path - expect(page).to have_text "[Error 404]" - + it "does not render the avatar section on the account page" do visit my_account_path + expect(page).to have_text(I18n.t(:label_my_account)) - expect(page).to have_no_css ".avatar-menu-item" + expect(page).to have_no_css(".avatars--current-avatar") end end end diff --git a/spec/controllers/my_controller_spec.rb b/spec/controllers/my_controller_spec.rb index bdac1939a240..5fc066886411 100644 --- a/spec/controllers/my_controller_spec.rb +++ b/spec/controllers/my_controller_spec.rb @@ -300,6 +300,23 @@ end end + describe "updating custom field values" do + let!(:editable_cf) { create(:user_custom_field, :string, editable: true) } + let!(:readonly_cf) { create(:user_custom_field, :string, editable: false) } + + it "persists editable custom fields but ignores non-editable ones" do + as_logged_in_user user do + patch :update_settings, params: { + user: { custom_field_values: { editable_cf.id.to_s => "ok", + readonly_cf.id.to_s => "tampered" } } + } + end + + expect(user.reload.custom_value_for(editable_cf)&.value).to eq "ok" + expect(user.custom_value_for(readonly_cf)&.value).to be_blank + end + end + describe "changing changing mail" do let!(:recovery_token) { create(:recovery_token, user:) } let!(:plain_session) { create(:user_session, user:, session_id: "internal_foobar") } diff --git a/spec/features/users/my_spec.rb b/spec/features/users/my_spec.rb index 95c1480486bc..175addf53293 100644 --- a/spec/features/users/my_spec.rb +++ b/spec/features/users/my_spec.rb @@ -33,6 +33,11 @@ RSpec.describe "my", :js do let(:user_password) { "bob" * 4 } let!(:string_cf) { create(:user_custom_field, :string, name: "Hobbies", is_required: false) } + let!(:default_section) do + create(:user_custom_field_section).tap do |section| + section.update_column(:attribute_order, UserCustomFieldSection::BUILT_IN_ATTRIBUTES) + end + end let(:user) do create(:user, mail: "old@mail.com", @@ -127,6 +132,19 @@ def expect_changed! end end + describe "non-editable custom fields" do + let!(:readonly_cf) do + create(:user_custom_field, :string, name: "Employee ID", editable: false, + user_custom_field_section: default_section) + end + + it "renders them read-only on the account page" do + visit my_account_path + + expect(page).to have_field("Employee ID", disabled: true) + end + end + context "user" do describe "#account" do let(:dialog) { Components::PasswordConfirmationDialog.new } diff --git a/spec/forms/custom_fields/inputs/single_select_list_spec.rb b/spec/forms/custom_fields/inputs/single_select_list_spec.rb index bf01b25d304f..f14d2b514f59 100644 --- a/spec/forms/custom_fields/inputs/single_select_list_spec.rb +++ b/spec/forms/custom_fields/inputs/single_select_list_spec.rb @@ -44,6 +44,18 @@ end end + context "when the field is rendered read-only" do + def build_form(builder) + described_class.new(builder, custom_field:, object: model, disabled: true) + end + + it_behaves_like "rendering autocompleter", "List field" do + it "disables the autocompleter" do + expect(autocompleter["data-disabled"]).to be_json_eql(true) + end + end + end + context "with a default value" do before do custom_field.possible_values[1].update default_value: true diff --git a/spec/forms/my/attributes_form_spec.rb b/spec/forms/my/attributes_form_spec.rb new file mode 100644 index 000000000000..62f131eaa527 --- /dev/null +++ b/spec/forms/my/attributes_form_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe My::AttributesForm, type: :forms do + # NOTE: My::AttributesForm builds its own contract during render, and the + # shared "with rendered form" context renders in a `before` of its own. This + # setup `before` is declared ahead of the include so the contract stub is in + # place before that render runs; the `let`s follow the include so they win + # over the shared context's defaults (e.g. `params`). + before do + User.current = current_user + create(:user_custom_field_section, attribute_order: UserCustomFieldSection::BUILT_IN_ATTRIBUTES) + allow(Users::UpdateContract).to receive(:new).and_return(contract) + allow(contract).to receive(:writable?) { |attr| writable_attributes.include?(attr.to_sym) } + end + + include_context "with rendered form" + + let(:writable_attributes) { %i[firstname lastname mail language] } # internal user; login never writable + let(:contract) { instance_double(Users::UpdateContract) } + let(:current_user) { model } + let(:model) { build_stubbed(:user) } + let(:params) { { user: model } } + + context "for an internal user" do + it "renders editable name/mail, a language select, and a read-only login" do + expect(page).to have_field("user[firstname]") + expect(page).to have_no_field("user[firstname]", readonly: true) + expect(page).to have_field("user[lastname]") + expect(page).to have_field("user[mail]") + expect(page).to have_select("user[language]") + expect(page).to have_field("user[login]", readonly: true) + end + + it "omits the admin flag and the account section" do + expect(page).to have_no_field("user[admin]") + expect(page).to have_no_css("fieldset", text: I18n.t(:label_account)) + end + end + + context "for a provider-managed login (name and email not writable)" do + let(:writable_attributes) { %i[language] } + + it "renders name and mail read-only with the provider caption" do + expect(page).to have_field("user[firstname]", readonly: true) + expect(page).to have_field("user[lastname]", readonly: true) + expect(page).to have_field("user[mail]", readonly: true) + expect(page).to have_text(I18n.t("user.text_change_disabled_for_provider_login"), count: 3) + end + end +end diff --git a/spec/forms/users/form/attributes_form_spec.rb b/spec/forms/users/form/attributes_form_spec.rb index f862cf759efe..5359e3ef3ac5 100644 --- a/spec/forms/users/form/attributes_form_spec.rb +++ b/spec/forms/users/form/attributes_form_spec.rb @@ -62,4 +62,16 @@ expect(page).to have_no_field("user[login]") end end + + context "with a section that has no visible content" do + let(:model) { build_stubbed(:user) } + + before do + create(:user_custom_field_section, name: "Empty", attribute_order: []) + end + + it "does not render an empty section fieldset" do + expect(page).to have_no_css("fieldset", text: "Empty") + end + end end From 1241a754584cc6d2309ba5020d9a30ded7469406 Mon Sep 17 00:00:00 2001 From: David F Date: Tue, 16 Jun 2026 18:53:21 +0200 Subject: [PATCH 04/14] Fix 2FA spec expectations and test seeds. OP-19485 --- .../spec/features/account_activation_spec.rb | 4 +- .../features/shared_two_factor_examples.rb | 2 +- spec/components/users/form_component_spec.rb | 1 - spec/features/auth/auth_stages_spec.rb | 6 +-- spec/features/auth/consent_auth_stage_spec.rb | 10 +--- spec/features/security/session_ttl_spec.rb | 2 +- spec/features/users/create_spec.rb | 5 -- spec/features/users/edit_users_spec.rb | 5 -- spec/features/users/my_spec.rb | 8 +-- spec/forms/my/attributes_form_spec.rb | 1 - spec/forms/users/form/attributes_form_spec.rb | 1 - ...ute_order_to_custom_field_sections_spec.rb | 5 ++ ...m_field_section_from_custom_fields_spec.rb | 8 +++ spec/support/user_attribute_section.rb | 50 +++++++++++++++++++ 14 files changed, 72 insertions(+), 36 deletions(-) create mode 100644 spec/support/user_attribute_section.rb diff --git a/modules/two_factor_authentication/spec/features/account_activation_spec.rb b/modules/two_factor_authentication/spec/features/account_activation_spec.rb index a0e06ed7dd0d..2e5efa9bc15a 100644 --- a/modules/two_factor_authentication/spec/features/account_activation_spec.rb +++ b/modules/two_factor_authentication/spec/features/account_activation_spec.rb @@ -39,7 +39,7 @@ def activate! visit my_account_path within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login + expect(page).to have_field "user_login", with: user.login end end end @@ -66,7 +66,7 @@ def activate! visit my_account_path within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login + expect(page).to have_field "user_login", with: user.login end end diff --git a/modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb b/modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb index 3a7b264d75de..23f79cc34f32 100644 --- a/modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb +++ b/modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb @@ -29,7 +29,7 @@ def expect_logged_in wait_for_network_idle visit my_account_path within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login + expect(page).to have_field "user_login", with: user.login end end diff --git a/spec/components/users/form_component_spec.rb b/spec/components/users/form_component_spec.rb index 92569b9c6f0e..f6dcdd9df58f 100644 --- a/spec/components/users/form_component_spec.rb +++ b/spec/components/users/form_component_spec.rb @@ -36,7 +36,6 @@ before do User.current = current_user - create(:user_custom_field_section, attribute_order: UserCustomFieldSection::BUILT_IN_ATTRIBUTES) end def render_component(user:) diff --git a/spec/features/auth/auth_stages_spec.rb b/spec/features/auth/auth_stages_spec.rb index 70d4f61f1271..98fc78f461d0 100644 --- a/spec/features/auth/auth_stages_spec.rb +++ b/spec/features/auth/auth_stages_spec.rb @@ -146,7 +146,7 @@ def login! visit "/my/account" within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login # just checking we're really logged in + expect(page).to have_field "user_login", with: user.login # just checking we're really logged in end end @@ -220,7 +220,7 @@ def login! visit "/my/account" within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login # just checking we're really logged in + expect(page).to have_field "user_login", with: user.login # just checking we're really logged in end end @@ -252,7 +252,7 @@ def login! visit "/my/account" within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login # just checking we're really logged in + expect(page).to have_field "user_login", with: user.login # just checking we're really logged in end end end diff --git a/spec/features/auth/consent_auth_stage_spec.rb b/spec/features/auth/consent_auth_stage_spec.rb index 8bf22545708c..1197bc9fb695 100644 --- a/spec/features/auth/consent_auth_stage_spec.rb +++ b/spec/features/auth/consent_auth_stage_spec.rb @@ -31,14 +31,6 @@ require "spec_helper" RSpec.describe "Authentication Stages" do - # The new-user form renders the built-in attributes (login, mail, …) through - # the user attribute sections, so at least one section must list them. - shared_let(:default_section) do - create(:user_custom_field_section).tap do |s| - s.update_column(:attribute_order, UserCustomFieldSection::BUILT_IN_ATTRIBUTES) - end - end - let(:language) { "en" } let(:user_password) { "bob" * 4 } let(:user) do @@ -63,7 +55,7 @@ def expect_logged_in(path = home_path) wait_for_network_idle within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: user.login + expect(page).to have_field "user_login", with: user.login end end diff --git a/spec/features/security/session_ttl_spec.rb b/spec/features/security/session_ttl_spec.rb index de4d2afbc386..c6e3e3893f92 100644 --- a/spec/features/security/session_ttl_spec.rb +++ b/spec/features/security/session_ttl_spec.rb @@ -50,7 +50,7 @@ def expire! visit "/my/account" within_test_selector "my-account-form" do - expect(page).to have_field "user_username", with: admin.login + expect(page).to have_field "user_login", with: admin.login end # Expire the session diff --git a/spec/features/users/create_spec.rb b/spec/features/users/create_spec.rb index 0a53164a33bf..7a546531d57e 100644 --- a/spec/features/users/create_spec.rb +++ b/spec/features/users/create_spec.rb @@ -32,11 +32,6 @@ RSpec.describe "create users" do shared_let(:admin) { create(:admin) } - shared_let(:default_section) do - create(:user_custom_field_section).tap do |s| - s.update_column(:attribute_order, UserCustomFieldSection::BUILT_IN_ATTRIBUTES) - end - end let(:current_user) { admin } let!(:auth_source) { create(:ldap_auth_source) } let(:new_user_page) { Pages::NewUser.new } diff --git a/spec/features/users/edit_users_spec.rb b/spec/features/users/edit_users_spec.rb index 15355fb483bd..b81ff44112de 100644 --- a/spec/features/users/edit_users_spec.rb +++ b/spec/features/users/edit_users_spec.rb @@ -179,11 +179,6 @@ def have_visible_tab(label) context "as global user" do shared_let(:global_manage_user) { create(:user, global_permissions: %i[view_all_principals manage_user create_user]) } let(:current_user) { global_manage_user } - let!(:default_section) do - create(:user_custom_field_section).tap do |s| - s.update_column(:attribute_order, UserCustomFieldSection::BUILT_IN_ATTRIBUTES) - end - end it "can too edit the user" do visit edit_user_path(user) diff --git a/spec/features/users/my_spec.rb b/spec/features/users/my_spec.rb index 175addf53293..74afd390a9b1 100644 --- a/spec/features/users/my_spec.rb +++ b/spec/features/users/my_spec.rb @@ -33,11 +33,6 @@ RSpec.describe "my", :js do let(:user_password) { "bob" * 4 } let!(:string_cf) { create(:user_custom_field, :string, name: "Hobbies", is_required: false) } - let!(:default_section) do - create(:user_custom_field_section).tap do |section| - section.update_column(:attribute_order, UserCustomFieldSection::BUILT_IN_ATTRIBUTES) - end - end let(:user) do create(:user, mail: "old@mail.com", @@ -134,8 +129,7 @@ def expect_changed! describe "non-editable custom fields" do let!(:readonly_cf) do - create(:user_custom_field, :string, name: "Employee ID", editable: false, - user_custom_field_section: default_section) + create(:user_custom_field, :string, name: "Employee ID", editable: false) end it "renders them read-only on the account page" do diff --git a/spec/forms/my/attributes_form_spec.rb b/spec/forms/my/attributes_form_spec.rb index 62f131eaa527..1e81c4f60da5 100644 --- a/spec/forms/my/attributes_form_spec.rb +++ b/spec/forms/my/attributes_form_spec.rb @@ -10,7 +10,6 @@ # over the shared context's defaults (e.g. `params`). before do User.current = current_user - create(:user_custom_field_section, attribute_order: UserCustomFieldSection::BUILT_IN_ATTRIBUTES) allow(Users::UpdateContract).to receive(:new).and_return(contract) allow(contract).to receive(:writable?) { |attr| writable_attributes.include?(attr.to_sym) } end diff --git a/spec/forms/users/form/attributes_form_spec.rb b/spec/forms/users/form/attributes_form_spec.rb index 5359e3ef3ac5..5c587468df93 100644 --- a/spec/forms/users/form/attributes_form_spec.rb +++ b/spec/forms/users/form/attributes_form_spec.rb @@ -33,7 +33,6 @@ RSpec.describe Users::Form::AttributesForm, type: :forms do before do User.current = current_user - create(:user_custom_field_section, attribute_order: UserCustomFieldSection::BUILT_IN_ATTRIBUTES) end include_context "with rendered form" diff --git a/spec/migrations/add_attribute_order_to_custom_field_sections_spec.rb b/spec/migrations/add_attribute_order_to_custom_field_sections_spec.rb index dfde5de4e1e7..361b289dbcd6 100644 --- a/spec/migrations/add_attribute_order_to_custom_field_sections_spec.rb +++ b/spec/migrations/add_attribute_order_to_custom_field_sections_spec.rb @@ -81,6 +81,11 @@ def attribute_order_of(section_id) conn.remove_column :custom_fields, :position_in_custom_field_section if conn.column_exists?(:custom_fields, :position_in_custom_field_section) end + + # The raw add/remove above changes columns without refreshing ActiveRecord's + # cached column lists, leaving the models out of sync with the restored + # schema. Reset them so later specs build SQL against the real columns. + [CustomField, CustomFieldSection].each(&:reset_column_information) end describe "up migration" do diff --git a/spec/migrations/remove_position_in_custom_field_section_from_custom_fields_spec.rb b/spec/migrations/remove_position_in_custom_field_section_from_custom_fields_spec.rb index 75a72adda538..fcb397586db4 100644 --- a/spec/migrations/remove_position_in_custom_field_section_from_custom_fields_spec.rb +++ b/spec/migrations/remove_position_in_custom_field_section_from_custom_fields_spec.rb @@ -60,6 +60,10 @@ def position_of(cf) conn.remove_column :custom_fields, :position_in_custom_field_section if conn.column_exists?(:custom_fields, :position_in_custom_field_section) end + + # The raw column changes above don't refresh ActiveRecord's cached column + # lists; reset them so later specs build SQL against the restored schema. + [CustomField, CustomFieldSection].each(&:reset_column_information) end it "seeds attribute_order from position_in_custom_field_section" do @@ -100,6 +104,10 @@ def position_of(cf) conn.remove_column :custom_fields, :position_in_custom_field_section if conn.column_exists?(:custom_fields, :position_in_custom_field_section) end + + # The raw column changes above don't refresh ActiveRecord's cached column + # lists; reset them so later specs build SQL against the restored schema. + [CustomField, CustomFieldSection].each(&:reset_column_information) end it "adds position_in_custom_field_section back" do diff --git a/spec/support/user_attribute_section.rb b/spec/support/user_attribute_section.rb new file mode 100644 index 000000000000..f459eea700f7 --- /dev/null +++ b/spec/support/user_attribute_section.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# A default user attribute section always exists in a real installation: it is +# created by a migration for existing installations and by the seeder for fresh +# installs. The administration user form and the my/account page render the +# built-in user attributes (login, mail, …) through these sections, so they only +# show up when one lists them. +# +# Seed that same default section — using the very seeder fresh installs run — for +# every spec that renders those forms, so specs match production and never rely +# on an empty table that cannot occur in practice. It is seeded per example (and +# rolled back with the example's transaction) rather than committed once, so it +# never leaks into the data-layer specs that legitimately control their own +# sections: the model, service and migration specs (the latter exercise the +# schema from before this data existed) are intentionally left untouched. +RSpec.configure do |config| + %i[feature request view component forms].each do |spec_type| + config.before(:each, type: spec_type) do + BasicData::UserCustomFieldSectionSeeder.new.seed! + end + end +end From 7a4ea0f849df9f1e2c4516749114fd911ade113c Mon Sep 17 00:00:00 2001 From: David F Date: Thu, 18 Jun 2026 17:17:02 +0200 Subject: [PATCH 05/14] Primerize consent heading and tidy avatar layout CSS. OP-19485 --- app/views/users/_consent.html.erb | 11 ++++++----- .../src/global_styles/content/modules/_avatars.sass | 9 +++++---- .../app/views/avatars/users/_avatar_fields.html.erb | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/app/views/users/_consent.html.erb b/app/views/users/_consent.html.erb index 4e728216868b..64c63669fde8 100644 --- a/app/views/users/_consent.html.erb +++ b/app/views/users/_consent.html.erb @@ -1,8 +1,9 @@ <%# locals: (user:) -%> -
-

<%= t("consent.title") %>

- <% consent_link = admin_settings_users_path(anchor: "consent_settings") %> - +<% consent_link = admin_settings_users_path(anchor: "consent_settings") %> +<%= render(Primer::Box.new(tag: :section, mb: 4)) do %> + <%= render(Primer::Beta::Subhead.new) do |subhead| %> + <% subhead.with_heading(tag: :h3, size: :medium).with_content(t("consent.title")) %> + <% end %> <%= render ::Components::OnOffStatusComponent.new( { is_on: user.consented_at.present?, @@ -12,4 +13,4 @@ off_description: link_translate("consent.not_yet_consented", links: { consent_settings: consent_link }) } ) %> -
+<% end %> diff --git a/frontend/src/global_styles/content/modules/_avatars.sass b/frontend/src/global_styles/content/modules/_avatars.sass index c797d612bf9d..e64e2cfa50a8 100644 --- a/frontend/src/global_styles/content/modules/_avatars.sass +++ b/frontend/src/global_styles/content/modules/_avatars.sass @@ -2,6 +2,7 @@ display: flex align-items: flex-start gap: 1.5rem + margin-bottom: 1rem .avatars--media display: flex @@ -17,10 +18,10 @@ display: inline-block .op-avatar - height: 64px - width: 64px - line-height: 60px - font-size: 28px + height: 4rem + width: 4rem + line-height: 3.75rem + font-size: 1.75rem .avatars--upload-trigger cursor: pointer diff --git a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb index ed8307c00b4d..168a436940cc 100644 --- a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb +++ b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb @@ -1,7 +1,7 @@ <%# locals: (user:, target_avatar_path:) -%> <% manager = ::OpenProject::Avatars::AvatarManager %> -
+
<% if manager.local_avatars_enabled? %> <%# Clicking the avatar activates the hidden file input rendered by the From d5dd8b7822cca526bd01b45c2fb3f0fd7681798d Mon Sep 17 00:00:00 2001 From: David F Date: Thu, 18 Jun 2026 17:17:16 +0200 Subject: [PATCH 06/14] Consolidate user authentication settings into one fieldset. OP-19485 Merge AuthenticationSourceForm and PasswordForm into a single AuthenticationForm that renders every authentication scheme -- external provider, LDAP source select, password settings and the cannot-login notice -- inside one titled "Authentication" fieldset, replacing the blocks previously rendered outside the form. --- app/components/users/form_component.html.erb | 8 - app/components/users/form_component.rb | 16 +- ...assword_form.rb => authentication_form.rb} | 110 +++++++++++--- .../users/form/authentication_source_form.rb | 80 ---------- .../form/authentication/_external.html.erb | 2 +- config/initializers/primer_forms.rb | 13 +- .../open_project/forms/dsl/input_methods.rb | 6 + .../users/form/authentication_form_spec.rb | 140 ++++++++++++++++++ .../form/authentication_source_form_spec.rb | 63 -------- spec/forms/users/form/password_form_spec.rb | 61 -------- 10 files changed, 258 insertions(+), 241 deletions(-) rename app/forms/users/form/{password_form.rb => authentication_form.rb} (52%) delete mode 100644 app/forms/users/form/authentication_source_form.rb create mode 100644 spec/forms/users/form/authentication_form_spec.rb delete mode 100644 spec/forms/users/form/authentication_source_form_spec.rb delete mode 100644 spec/forms/users/form/password_form_spec.rb diff --git a/app/components/users/form_component.html.erb b/app/components/users/form_component.html.erb index a530d65a4500..16c0843af05f 100644 --- a/app/components/users/form_component.html.erb +++ b/app/components/users/form_component.html.erb @@ -1,13 +1,5 @@ <%= render(form_list) %> -<% if show_no_login_message? %> - <%= render(Primer::Beta::Text.new(tag: :p, color: :muted)) { I18n.t("user.no_login") } %> -<% end %> - -<% if show_external_auth? %> - <%= render("users/form/authentication/external", user: @user) %> -<% end %> - <%= helpers.render_user_form_hooks(user: @user, form: @builder) %> <% if show_consent? %> diff --git a/app/components/users/form_component.rb b/app/components/users/form_component.rb index c19d35937bf8..46d2ffcdba6d 100644 --- a/app/components/users/form_component.rb +++ b/app/components/users/form_component.rb @@ -53,14 +53,22 @@ def form_list def input_forms forms = [Users::Form::AttributesForm.new(@builder, user: @user, contract: @contract)] - forms << Users::Form::AuthenticationSourceForm.new(@builder, user: @user) if show_auth_source? - if show_password? - forms << Users::Form::PasswordForm.new(@builder, user: @user, - assign_random_password_checked: assign_random_password_checked?) + if show_authentication? + forms << Users::Form::AuthenticationForm.new(@builder, + user: @user, + render_auth_source: show_auth_source?, + render_password: show_password?, + render_no_login_message: show_no_login_message?, + render_external_auth: show_external_auth?, + assign_random_password_checked: assign_random_password_checked?) end forms end + def show_authentication? + show_auth_source? || show_password? || show_no_login_message? || show_external_auth? + end + def show_auth_source? return false if editing? && @user.uses_external_authentication? diff --git a/app/forms/users/form/password_form.rb b/app/forms/users/form/authentication_form.rb similarity index 52% rename from app/forms/users/form/password_form.rb rename to app/forms/users/form/authentication_form.rb index 30d56dd0f3bb..a566d206f73c 100644 --- a/app/forms/users/form/password_form.rb +++ b/app/forms/users/form/authentication_form.rb @@ -30,38 +30,98 @@ module Users module Form - # The internal password / activation block as a single Primer group. All three - # Stimulus controllers sit on the wrapper: the password-requirements controller - # finds both its passwordInput and requirement targets within it. - # assign_random_password and send_information are not model - # attributes, so they render with an explicit checked: and no hidden companion - # (the controller relies on their absence when unchecked); send_information is - # unscoped (top-level param). Included by the coordinator only for editing an - # internal user as admin while password login is enabled. - class PasswordForm < ApplicationForm + # The "Authentication" fieldset: every authentication scheme for the user, + # in one titled section. The coordinator decides which parts apply and only + # includes this form when at least one does: + # - external authentication (OmniAuth/OIDC): a read-only display of the + # provider and identity url -- mutually exclusive with everything below; + # - the LDAP source select when LDAP sources can be assigned (and, for a new + # record, the hidden login the admin--users controller reveals once a + # source is selected); + # - the internal password settings when editing an internal user as admin + # while password login is enabled (the controller toggles these against + # the source select); + # - a notice that the user cannot log in when password login is disabled. + class AuthenticationForm < ApplicationForm form do |f| - f.group( - hidden: !@user.change_password_allowed?, - data: { - controller: "disable-when-checked password-force-change password-requirements", - "admin--users-target": "passwordFields" - } - ) do |group| - assign_random_password(group) - password_fields(group) unless disable_password_choice? - send_information(group) - force_password_change(group) + f.fieldset_group(title: I18n.t(:label_authentication)) do |group| + if @render_external_auth + external_authentication(group) + else + if @render_auth_source + ldap_auth_source_select(group) + hidden_login(group) if @user.new_record? + end + + password_group(group) if @render_password + no_login_message(group) if @render_no_login_message + end end end - def initialize(user:, assign_random_password_checked:) + def initialize(user:, render_auth_source:, render_password:, render_no_login_message:, render_external_auth:, + assign_random_password_checked: false) super() @user = user + @render_auth_source = render_auth_source + @render_password = render_password + @render_no_login_message = render_no_login_message + @render_external_auth = render_external_auth @assign_random_password_checked = assign_random_password_checked end private + # The provider and identity url as a read-only block, shown for users that + # authenticate through an external OmniAuth/OIDC provider. + def external_authentication(group) + group.html_content do + render("users/form/authentication/external", user: @user) + end + end + + # The LDAP source select, with a blank "internal" option. Its admin--users + # action toggles the password and LDAP groups as the selection changes. + def ldap_auth_source_select(group) + group.select_list( + name: :ldap_auth_source_id, + label: User.human_attribute_name(:auth_source), + include_blank: I18n.t(:label_internal), + input_width: :medium, + data: { action: "admin--users#toggleAuthenticationFields" } + ) do |list| + LdapAuthSource.order(:name).each { |source| list.option(label: source.name, value: source.id) } + end + end + + # The login as a nested group, hidden by default. The admin--users controller + # reveals it (its authSourceFields target) when an LDAP source is selected. + def hidden_login(group) + group.group(hidden: true, data: { "admin--users-target": "authSourceFields" }) do |login_group| + login_group.text_field(name: :login, + label: User.human_attribute_name(:login), + required: true, + input_width: :medium) + end + end + + # The password options as a nested group, visible by default. The admin--users controller + # hides it (its passwordFields target) when an LDAP source is selected. + def password_group(group) + group.group( + hidden: !@user.change_password_allowed?, + data: { + controller: "disable-when-checked password-force-change password-requirements", + "admin--users-target": "passwordFields" + } + ) do |password_group| + assign_random_password(password_group) + password_fields(password_group) unless disable_password_choice? + send_information(password_group) + force_password_change(password_group) + end + end + def disable_password_choice? OpenProject::Configuration.disable_password_choice? end @@ -119,6 +179,14 @@ def force_password_change(group) caption: I18n.t("users.force_password_change_hint"), data: { "password-force-change-target": "forceChangeCheckbox" }) end + + # A warning that the user cannot log in, shown for an internal user when + # password login is disabled instance-wide. + def no_login_message(group) + group.html_content do + render(Primer::OpenProject::InlineMessage.new(scheme: :warning)) { I18n.t("user.no_login") } + end + end end end end diff --git a/app/forms/users/form/authentication_source_form.rb b/app/forms/users/form/authentication_source_form.rb deleted file mode 100644 index 8a399d7a63c6..000000000000 --- a/app/forms/users/form/authentication_source_form.rb +++ /dev/null @@ -1,80 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module Users - module Form - # LDAP authentication source select. For a new record it also renders the - # hidden login field the admin--users controller reveals when an LDAP source - # is selected; for a persisted record the login is already a built-in field, - # so the select is wrapped in a titled "Authentication" fieldset only. - # - # The coordinator decides whether to include this form at all. - class AuthenticationSourceForm < ApplicationForm - form do |f| - if @user.new_record? - ldap_auth_source_select(f) - hidden_login(f) - else - f.fieldset_group(title: I18n.t(:label_authentication)) do |group| - ldap_auth_source_select(group) - end - end - end - - def initialize(user:) - super() - @user = user - end - - private - - def ldap_auth_source_select(target) - target.select_list( - name: :ldap_auth_source_id, - label: User.human_attribute_name(:auth_source), - include_blank: I18n.t(:label_internal), - input_width: :medium, - data: { action: "admin--users#toggleAuthenticationFields" } - ) do |list| - LdapAuthSource.order(:name).each { |source| list.option(label: source.name, value: source.id) } - end - end - - def hidden_login(form) - form.group(hidden: true, data: { "admin--users-target": "authSourceFields" }) do |group| - group.text_field(name: :login, - label: User.human_attribute_name(:login), - required: true, - input_width: :medium) - end - end - end - end -end diff --git a/app/views/users/form/authentication/_external.html.erb b/app/views/users/form/authentication/_external.html.erb index 749daf79d816..da67e645f0df 100644 --- a/app/views/users/form/authentication/_external.html.erb +++ b/app/views/users/form/authentication/_external.html.erb @@ -1,5 +1,5 @@ <%# locals: (user:) -%> -<%= render(Primer::Box.new(mb: 3)) do %> +<%= render(Primer::Box.new) do %> <%= render(Primer::Beta::Text.new(tag: :div, font_weight: :bold)) { I18n.t("user.authentication_provider") } %> <%= render(Primer::Beta::Text.new(tag: :div)) { user.human_authentication_provider } %> <%= render(Primer::Beta::Text.new(tag: :div, color: :muted, font_size: :small)) do %> diff --git a/config/initializers/primer_forms.rb b/config/initializers/primer_forms.rb index b3dec93ff219..19d84c0dfc33 100644 --- a/config/initializers/primer_forms.rb +++ b/config/initializers/primer_forms.rb @@ -29,7 +29,14 @@ #++ Rails.application.config.to_prepare do - Primer::Forms::Dsl::FormObject.include(Primer::OpenProject::Forms::Dsl::InputMethods) - Primer::Forms::Dsl::InputGroup.include(Primer::OpenProject::Forms::Dsl::InputMethods) - Primer::Forms::Dsl::MultiInput.include(Primer::OpenProject::Forms::Dsl::InputMethods) + [Primer::Forms::Dsl::FormObject, + Primer::Forms::Dsl::InputGroup, + Primer::Forms::Dsl::MultiInput, + Primer::Forms::Dsl::FieldsetGroupInput].each do |klass| + klass.include(Primer::OpenProject::Forms::Dsl::InputMethods) + end + + # A nested group must report its own visibility: the parent Group/FieldsetGroup + # collapses (display: none) when inputs.all?(&:hidden?). + Primer::Forms::Dsl::InputGroup.define_method(:hidden?) { !!system_arguments[:hidden] } end diff --git a/lib/primer/open_project/forms/dsl/input_methods.rb b/lib/primer/open_project/forms/dsl/input_methods.rb index 3efc5ae272f4..dae9530fd948 100644 --- a/lib/primer/open_project/forms/dsl/input_methods.rb +++ b/lib/primer/open_project/forms/dsl/input_methods.rb @@ -52,6 +52,12 @@ def html_content(&) add_input HtmlContent.new(&) end + # Open a nested input group. Upstream only defines this on the top-level + # FormObject, so it is otherwise unavailable inside a group or fieldset_group. + def group(**, &) + add_input Primer::Forms::Dsl::InputGroup.new(builder:, form:, **, &) + end + def pattern_input(**, &) add_input PatternInput.new(builder:, form:, **decorate_options(**), &) end diff --git a/spec/forms/users/form/authentication_form_spec.rb b/spec/forms/users/form/authentication_form_spec.rb new file mode 100644 index 000000000000..0453c1f7ba74 --- /dev/null +++ b/spec/forms/users/form/authentication_form_spec.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Users::Form::AuthenticationForm, type: :forms do + include_context "with rendered form" + + before do + User.current = build_stubbed(:admin) + create(:ldap_auth_source) + end + + let(:params) do + { user: model, + render_auth_source:, + render_password:, + render_no_login_message:, + render_external_auth:, + assign_random_password_checked: false } + end + let(:render_auth_source) { true } + let(:render_password) { true } + let(:render_no_login_message) { false } + let(:render_external_auth) { false } + + context "with only the auth source select, for a new user" do + let(:model) { User.new } + let(:render_password) { false } + + it "renders the auth source select with the toggle action and a hidden login group" do + expect(page).to have_select("user[ldap_auth_source_id]") + expect(page).to have_css("[data-action~='admin--users#toggleAuthenticationFields']") + expect(page).to have_css("[data-admin--users-target='authSourceFields'][hidden]", visible: :all) + expect(page).to have_field("user[login]", visible: :all) + end + end + + context "with only the auth source select, for a persisted user" do + let(:model) { build_stubbed(:user) } + let(:render_password) { false } + + it "renders the select inside a titled Authentication fieldset and no hidden login" do + expect(page).to have_select("user[ldap_auth_source_id]") + expect(page).to have_css("fieldset", text: /#{I18n.t(:label_authentication)}/i) + expect(page).to have_no_css("[data-admin--users-target='authSourceFields']", visible: :all) + end + end + + context "with only the password settings" do + let(:model) { build_stubbed(:user) } + let(:render_auth_source) { false } + + before { allow(model).to receive(:change_password_allowed?).and_return(true) } + + it "renders one wrapper carrying the three controllers and the passwordFields target" do + expect(page).to have_css( + "[data-controller~='disable-when-checked'][data-controller~='password-force-change']" \ + "[data-controller~='password-requirements'][data-admin--users-target='passwordFields']", + visible: :all + ) + end + + it "renders the password fields with their ids and password type" do + expect(page).to have_field("user[password]", type: "password", visible: :all) + expect(page).to have_field("user[password_confirmation]", type: "password", visible: :all) + expect(page).to have_css("#user_password", visible: :all) + end + + it "renders the unscoped send_information checkbox and the scoped flags" do + expect(page).to have_field("send_information", visible: :all) + expect(page).to have_css("#send_information", visible: :all) + expect(page).to have_field("user[assign_random_password]", visible: :all) + expect(page).to have_field("user[force_password_change]", visible: :all) + end + end + + context "with both the auth source select and the password settings" do + let(:model) { build_stubbed(:user) } + + before { allow(model).to receive(:change_password_allowed?).and_return(true) } + + it "nests both the select and the password fields inside the Authentication fieldset" do + expect(page).to have_css("fieldset", text: /#{I18n.t(:label_authentication)}/i) + expect(page).to have_css("fieldset select[name='user[ldap_auth_source_id]']") + expect(page).to have_css("fieldset input[name='user[password]']", visible: :all) + end + end + + context "with the no-login notice" do + let(:model) { build_stubbed(:user) } + let(:render_auth_source) { false } + let(:render_password) { false } + let(:render_no_login_message) { true } + + it "renders the cannot-login notice as an InlineMessage inside the Authentication fieldset" do + expect(page).to have_css("fieldset .InlineMessage", text: I18n.t("user.no_login")) + end + end + + context "with external authentication" do + let(:model) { create(:user, identity_url: "saml:user-id") } + let(:render_auth_source) { false } + let(:render_password) { false } + let(:render_external_auth) { true } + + it "renders the provider read-only inside the Authentication fieldset and no auth inputs" do + expect(page).to have_css("fieldset", text: /#{I18n.t(:label_authentication)}/i) + expect(page).to have_text(I18n.t("user.authentication_provider")) + expect(page).to have_no_select("user[ldap_auth_source_id]") + end + end +end diff --git a/spec/forms/users/form/authentication_source_form_spec.rb b/spec/forms/users/form/authentication_source_form_spec.rb deleted file mode 100644 index 734075743baa..000000000000 --- a/spec/forms/users/form/authentication_source_form_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -require "spec_helper" - -RSpec.describe Users::Form::AuthenticationSourceForm, type: :forms do - before do - User.current = build_stubbed(:admin) - create(:ldap_auth_source) - end - - include_context "with rendered form" - - let(:params) { { user: model } } - - context "for a new user" do - let(:model) { User.new } - - it "renders the auth source select with the toggle action and a hidden login group" do - expect(page).to have_select("user[ldap_auth_source_id]") - expect(page).to have_css("[data-action~='admin--users#toggleAuthenticationFields']") - expect(page).to have_css("[data-admin--users-target='authSourceFields'][hidden]", visible: :all) - expect(page).to have_field("user[login]", visible: :all) - end - end - - context "for a persisted user" do - let(:model) { build_stubbed(:user) } - - it "renders the select inside a titled Authentication fieldset and no hidden login" do - expect(page).to have_select("user[ldap_auth_source_id]") - expect(page).to have_css("fieldset", text: /#{I18n.t(:label_authentication)}/i) - expect(page).to have_no_css("[data-admin--users-target='authSourceFields']", visible: :all) - end - end -end diff --git a/spec/forms/users/form/password_form_spec.rb b/spec/forms/users/form/password_form_spec.rb deleted file mode 100644 index f79435c00442..000000000000 --- a/spec/forms/users/form/password_form_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -require "spec_helper" - -RSpec.describe Users::Form::PasswordForm, type: :forms do - include_context "with rendered form" - - let(:model) { build_stubbed(:user) } - let(:params) { { user: model, assign_random_password_checked: false } } - - before { allow(model).to receive(:change_password_allowed?).and_return(true) } - - it "renders one wrapper carrying the three controllers and the passwordFields target" do - expect(page).to have_css( - "[data-controller~='disable-when-checked'][data-controller~='password-force-change']" \ - "[data-controller~='password-requirements'][data-admin--users-target='passwordFields']", - visible: :all - ) - end - - it "renders the password fields with their ids and password type" do - expect(page).to have_field("user[password]", type: "password", visible: :all) - expect(page).to have_field("user[password_confirmation]", type: "password", visible: :all) - expect(page).to have_css("#user_password", visible: :all) - end - - it "renders the unscoped send_information checkbox and the scoped flags" do - expect(page).to have_field("send_information", visible: :all) - expect(page).to have_css("#send_information", visible: :all) - expect(page).to have_field("user[assign_random_password]", visible: :all) - expect(page).to have_field("user[force_password_change]", visible: :all) - end -end From 7d94cfe2e490a88eb92410b3b5df1cfc07f1a0b8 Mon Sep 17 00:00:00 2001 From: David F Date: Thu, 18 Jun 2026 17:35:49 +0200 Subject: [PATCH 07/14] Address remaining user form review comments. OP-19485 - Guard the external identity URL display against a nil value. - Stop the avatar hint from mentioning Gravatar when it is disabled. - Show the user's status as a read-only line in the Account section instead of folding it into the section title. - Reword the untitled custom-field section label to "User attributes". - Drop the redundant "Create and continue" button (and the now-dead continue redirect in the controller). Co-Authored-By: Claude Opus 4.8 (1M context) --- app/components/users/form_component.html.erb | 3 --- app/controllers/users_controller.rb | 4 ++-- app/forms/users/form/attributes_form.rb | 19 +++++++++++-------- .../form/authentication/_external.html.erb | 2 +- config/locales/en.yml | 2 +- .../avatars/users/_avatar_fields.html.erb | 2 +- modules/avatars/config/locales/en.yml | 2 ++ spec/components/users/form_component_spec.rb | 4 ++-- 8 files changed, 20 insertions(+), 18 deletions(-) diff --git a/app/components/users/form_component.html.erb b/app/components/users/form_component.html.erb index 16c0843af05f..16673965ae5c 100644 --- a/app/components/users/form_component.html.erb +++ b/app/components/users/form_component.html.erb @@ -13,6 +13,3 @@ <% end %> <%= render(Primer::Beta::Button.new(type: :submit, scheme: :primary, name: "submit")) { creating? ? I18n.t(:button_create) : I18n.t(:button_save) } %> -<% if creating? %> - <%= render(Primer::Beta::Button.new(type: :submit, name: "continue")) { I18n.t(:button_create_and_continue) } %> -<% end %> diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 499acc91fb04..86f3ec91042d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -115,7 +115,7 @@ def edit prepare_views_for_tab end - def create # rubocop:disable Metrics/AbcSize + def create call = Users::CreateService .new(user: current_user) .call(create_params) @@ -124,7 +124,7 @@ def create # rubocop:disable Metrics/AbcSize if call.success? flash[:notice] = I18n.t(:notice_successful_create) - redirect_to(params[:continue] ? new_user_path : helpers.allowed_management_user_profile_path(@user)) + redirect_to helpers.allowed_management_user_profile_path(@user) else @contract = Users::CreateContract.new(@user, current_user) render action: :new, status: :unprocessable_entity diff --git a/app/forms/users/form/attributes_form.rb b/app/forms/users/form/attributes_form.rb index 1b8b6d482940..de8da3caf2b8 100644 --- a/app/forms/users/form/attributes_form.rb +++ b/app/forms/users/form/attributes_form.rb @@ -54,12 +54,13 @@ def custom_fields # Built-in account attributes grouped under their own section so they read # as account settings rather than as part of the avatar block rendered - # above the form. The current status is folded into the section title - # (e.g. "Account: active") for persisted users. + # above the form. For persisted users the current status is shown as a + # read-only line at the top of the section. def account_section(form) return unless show_account_section? - form.fieldset_group(title: account_title) do |group| + form.fieldset_group(title: I18n.t(:label_account)) do |group| + account_status(group) if @user.persisted? admin_flag(group) if User.current.admin? end end @@ -68,11 +69,13 @@ def show_account_section? @user.persisted? || User.current.admin? end - def account_title - title = I18n.t(:label_account) - return title unless @user.persisted? - - "#{title}: #{helpers.full_user_status(@user, true)}" + # The current status (e.g. active, locked) as a read-only line rather than + # folded into the section title. + def account_status(group) + status = "#{User.human_attribute_name(:status)}: #{helpers.full_user_status(@user, true)}" + group.html_content do + render(Primer::Beta::Text.new(tag: :p)) { status } + end end def admin_flag(group) diff --git a/app/views/users/form/authentication/_external.html.erb b/app/views/users/form/authentication/_external.html.erb index da67e645f0df..6c872fc9f8f1 100644 --- a/app/views/users/form/authentication/_external.html.erb +++ b/app/views/users/form/authentication/_external.html.erb @@ -9,7 +9,7 @@ <%= render(Primer::Box.new) do %> <%= render(Primer::Beta::Text.new(tag: :div, font_weight: :bold)) { User.human_attribute_name(:identity_url) } %> - <%= render(Primer::Beta::Text.new(tag: :div)) { user.identity_url.split(":", 2).last } %> + <%= render(Primer::Beta::Text.new(tag: :div)) { user.identity_url&.split(":", 2)&.last } %> <%= render(Primer::Beta::Text.new(tag: :div, color: :muted, font_size: :small)) do %> <%= I18n.t("user.identity_url_text") %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index b6081df4d241..d918b99941af 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -5809,7 +5809,7 @@ en: heading: "User attributes" label_new_attribute: "User attribute" label_new_section: "Section" - label_untitled_section: "Untitled section" + label_untitled_section: "User attributes" new: heading: "New attribute" working_days: diff --git a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb index 168a436940cc..6969420c72b3 100644 --- a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb +++ b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb @@ -38,7 +38,7 @@ <%= t("avatars.text_avatar_gravatar_html", gravatar_url: link_to("gravatar.com", "https://gravatar.com")) %> <% end %> <% if manager.local_avatars_enabled? %> - <%= t("avatars.text_avatar_local") %> + <%= manager.gravatar_enabled? ? t("avatars.text_avatar_local") : t("avatars.text_avatar_local_only") %> <% end %>
diff --git a/modules/avatars/config/locales/en.yml b/modules/avatars/config/locales/en.yml index d240c3c83b9a..cb541cc51c09 100644 --- a/modules/avatars/config/locales/en.yml +++ b/modules/avatars/config/locales/en.yml @@ -28,6 +28,8 @@ en: OpenProject uses your Gravatar if you registered one at %{gravatar_url}, or a default image or icon if one exists. text_avatar_local: >- You can also upload a custom avatar that overrides the Gravatar by clicking the image. We recommend an image of 128 by 128 pixels. + text_avatar_local_only: >- + You can upload a custom avatar by clicking the image. We recommend an image of 128 by 128 pixels. settings: enable_gravatars: 'Enable user gravatars' gravatar_default: "Default Gravatar image" diff --git a/spec/components/users/form_component_spec.rb b/spec/components/users/form_component_spec.rb index f6dcdd9df58f..963727794856 100644 --- a/spec/components/users/form_component_spec.rb +++ b/spec/components/users/form_component_spec.rb @@ -47,12 +47,12 @@ def render_component(user:) end context "for a new user" do - it "renders the attributes and the create buttons, but no password block" do + it "renders the attributes and a single create button, but no password block" do render_component(user: User.new) expect(page).to have_field("user[firstname]") expect(page).to have_button(I18n.t(:button_create)) - expect(page).to have_button(I18n.t(:button_create_and_continue)) + expect(page).to have_no_button(I18n.t(:button_create_and_continue)) expect(page).to have_no_css("[data-admin--users-target='passwordFields']", visible: :all) end end From d3124f55bbb2e5c9e97d2b06467c761214d6e43b Mon Sep 17 00:00:00 2001 From: David F Date: Fri, 19 Jun 2026 08:52:57 +0200 Subject: [PATCH 08/14] Use standard sr-only class for avatar file upload. OP-19485 --- .../src/global_styles/content/modules/_avatars.sass | 10 ---------- .../module/avatar-upload-form/avatar-upload-form.html | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/frontend/src/global_styles/content/modules/_avatars.sass b/frontend/src/global_styles/content/modules/_avatars.sass index e64e2cfa50a8..30a05d7370b8 100644 --- a/frontend/src/global_styles/content/modules/_avatars.sass +++ b/frontend/src/global_styles/content/modules/_avatars.sass @@ -28,16 +28,6 @@ &:hover opacity: 0.8 -.avatars--file-input - position: absolute - width: 1px - height: 1px - padding: 0 - margin: -1px - overflow: hidden - clip: rect(0, 0, 0, 0) - border: 0 - .avatars--error-pane color: var(--content-form-error-color) font-weight: var(--base-text-weight-bold) diff --git a/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html b/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html index 8df6262bb865..83e1f88ae6e1 100644 --- a/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html +++ b/modules/avatars/frontend/module/avatar-upload-form/avatar-upload-form.html @@ -3,7 +3,7 @@ accept="image/jpeg,image/png,image/gif" id="avatar_file_input" name="avatar_file_input" - class="avatars--file-input" + class="sr-only" [attr.aria-label]="text.label_choose_avatar" [attr.disabled]="busy || undefined" (change)="onFilePickerChanged($event)" /> From ff169eda9402253b648b8a5a63d2684068d0d8e7 Mon Sep 17 00:00:00 2001 From: David F Date: Tue, 23 Jun 2026 09:27:45 +0200 Subject: [PATCH 09/14] Make user form section spacing consistent. OP-19485 --- app/components/users/form_component.html.erb | 14 ++-- app/components/users/form_component.rb | 1 + app/forms/users/form/consent_form.rb | 65 +++++++++++++++++++ app/views/my/account.html.erb | 5 +- app/views/users/_consent.html.erb | 16 ----- app/views/users/_general.html.erb | 5 +- .../views/avatars/my/_avatar_section.html.erb | 2 +- .../avatars/users/_avatar_section.html.erb | 2 +- 8 files changed, 81 insertions(+), 29 deletions(-) create mode 100644 app/forms/users/form/consent_form.rb delete mode 100644 app/views/users/_consent.html.erb diff --git a/app/components/users/form_component.html.erb b/app/components/users/form_component.html.erb index 16673965ae5c..70d55e3e0e6a 100644 --- a/app/components/users/form_component.html.erb +++ b/app/components/users/form_component.html.erb @@ -2,14 +2,14 @@ <%= helpers.render_user_form_hooks(user: @user, form: @builder) %> -<% if show_consent? %> - <%= render("users/consent", user: @user) %> -<% end %> - <% if show_preferences? %> - <%= fields_for(:pref, @user.pref, builder: @builder.class) do |pref_f| %> - <%= render(Users::Form::PreferencesForm.new(pref_f)) %> + <%# Outside the FormList because of its :pref scope, so it needs the gap the + list adds between its own sections (the section margin already supplies the rest). %> + <%= render(Primer::Box.new(mt: 3)) do %> + <%= fields_for(:pref, @user.pref, builder: @builder.class) do |pref_f| %> + <%= render(Users::Form::PreferencesForm.new(pref_f)) %> + <% end %> <% end %> <% end %> -<%= render(Primer::Beta::Button.new(type: :submit, scheme: :primary, name: "submit")) { creating? ? I18n.t(:button_create) : I18n.t(:button_save) } %> +<%= render(Primer::Beta::Button.new(type: :submit, scheme: :primary, name: "submit", mt: 3)) { creating? ? I18n.t(:button_create) : I18n.t(:button_save) } %> diff --git a/app/components/users/form_component.rb b/app/components/users/form_component.rb index 46d2ffcdba6d..4b9879a9f51b 100644 --- a/app/components/users/form_component.rb +++ b/app/components/users/form_component.rb @@ -62,6 +62,7 @@ def input_forms render_external_auth: show_external_auth?, assign_random_password_checked: assign_random_password_checked?) end + forms << Users::Form::ConsentForm.new(@builder, user: @user) if show_consent? forms end diff --git a/app/forms/users/form/consent_form.rb b/app/forms/users/form/consent_form.rb new file mode 100644 index 000000000000..7da59f26684b --- /dev/null +++ b/app/forms/users/form/consent_form.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users + module Form + # The read-only consent status as a regular form section, so it shares the + # fieldset heading and spacing of the surrounding administration user form. + class ConsentForm < ApplicationForm + form do |f| + f.fieldset_group(title: I18n.t("consent.title")) do |group| + group.html_content do + render(consent_status_component) + end + end + end + + def initialize(user:) + super() + @user = user + end + + private + + def consent_status_component + consent_link = helpers.admin_settings_users_path(anchor: "consent_settings") + Components::OnOffStatusComponent.new( + { + is_on: @user.consented_at.present?, + on_text: helpers.format_time(@user.consented_at), + on_description: helpers.link_translate("consent.user_has_consented", links: { consent_settings: consent_link }), + off_text: I18n.t(:label_never), + off_description: helpers.link_translate("consent.not_yet_consented", links: { consent_settings: consent_link }) + } + ) + end + end + end +end diff --git a/app/views/my/account.html.erb b/app/views/my/account.html.erb index 917b27ceed0b..6887d48e7855 100644 --- a/app/views/my/account.html.erb +++ b/app/views/my/account.html.erb @@ -57,8 +57,9 @@ See COPYRIGHT and LICENSE files for more details. end %> -<%# Constrain hook content (the avatar section) to the same width as the form below. %> -<%= content_tag(:div, class: "op-admin-settings-form-wrapper") do %> +<%# Constrain hook content (the avatar section) to the same width as the form below, + and separate it from the form with the same gap as sits between the form sections (mb-6). %> +<%= content_tag(:div, class: "op-admin-settings-form-wrapper mb-6") do %> <%= call_hook(:view_my_account_top, user: current_user) %> <% end %> diff --git a/app/views/users/_consent.html.erb b/app/views/users/_consent.html.erb deleted file mode 100644 index 64c63669fde8..000000000000 --- a/app/views/users/_consent.html.erb +++ /dev/null @@ -1,16 +0,0 @@ -<%# locals: (user:) -%> -<% consent_link = admin_settings_users_path(anchor: "consent_settings") %> -<%= render(Primer::Box.new(tag: :section, mb: 4)) do %> - <%= render(Primer::Beta::Subhead.new) do |subhead| %> - <% subhead.with_heading(tag: :h3, size: :medium).with_content(t("consent.title")) %> - <% end %> - <%= render ::Components::OnOffStatusComponent.new( - { - is_on: user.consented_at.present?, - on_text: format_time(user.consented_at), - on_description: link_translate("consent.user_has_consented", links: { consent_settings: consent_link }), - off_text: t(:label_never), - off_description: link_translate("consent.not_yet_consented", links: { consent_settings: consent_link }) - } - ) %> -<% end %> diff --git a/app/views/users/_general.html.erb b/app/views/users/_general.html.erb index 27cc4c9130ba..4890338b08f3 100644 --- a/app/views/users/_general.html.erb +++ b/app/views/users/_general.html.erb @@ -27,8 +27,9 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<%# Constrain hook content (e.g. the avatar section) to the same width as the form below. %> -<%= content_tag(:div, class: "op-admin-settings-form-wrapper") do %> +<%# Constrain hook content (e.g. the avatar section) to the same width as the form below, + and separate it from the form with the same gap as sits between the form sections (mb-6). %> +<%= content_tag(:div, class: "op-admin-settings-form-wrapper mb-6") do %> <%= call_hook(:view_users_general_top, user: @user) %> <% end %> diff --git a/modules/avatars/app/views/avatars/my/_avatar_section.html.erb b/modules/avatars/app/views/avatars/my/_avatar_section.html.erb index c536d252f4b4..22db61596f8a 100644 --- a/modules/avatars/app/views/avatars/my/_avatar_section.html.erb +++ b/modules/avatars/app/views/avatars/my/_avatar_section.html.erb @@ -1,7 +1,7 @@ <% manager = ::OpenProject::Avatars::AvatarManager %> <% if manager.avatars_enabled? %> <%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> - <% subhead.with_heading(tag: :h2) { t("avatars.label_avatar") } %> + <% subhead.with_heading(tag: :h3, size: :medium) { t("avatars.label_avatar") } %> <% end %> <%= render "avatars/users/avatar_fields", user:, target_avatar_path: edit_my_avatar_path %> diff --git a/modules/avatars/app/views/avatars/users/_avatar_section.html.erb b/modules/avatars/app/views/avatars/users/_avatar_section.html.erb index 492b48f111b7..37357a488ed8 100644 --- a/modules/avatars/app/views/avatars/users/_avatar_section.html.erb +++ b/modules/avatars/app/views/avatars/users/_avatar_section.html.erb @@ -1,7 +1,7 @@ <% manager = ::OpenProject::Avatars::AvatarManager %> <% if User.current.admin? && manager.avatars_enabled? %> <%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> - <% subhead.with_heading(tag: :h2) { t("avatars.label_avatar") } %> + <% subhead.with_heading(tag: :h3, size: :medium) { t("avatars.label_avatar") } %> <% end %> <%= render "avatars/users/avatar_fields", user:, target_avatar_path: edit_user_avatar_path(user) %> From 34d635ce18275073dd83d93a7e507400c42310f3 Mon Sep 17 00:00:00 2001 From: David F Date: Tue, 23 Jun 2026 13:18:35 +0200 Subject: [PATCH 10/14] Re-arrange avatar form section (with proper view component). OP-19485 --- .../content/modules/_avatars.sass | 12 +- .../avatars/form_section_component.html.erb | 35 ++++++ .../avatars/form_section_component.rb | 61 ++++++++++ .../views/avatars/my/_avatar_section.html.erb | 9 +- .../avatars/users/_avatar_fields.html.erb | 44 ------- .../avatars/users/_avatar_section.html.erb | 9 +- .../avatars/form_section_component_spec.rb | 80 +++++++++++++ .../avatars/spec/features/my_avatar_spec.rb | 110 +++++------------- .../avatars/spec/features/user_avatar_spec.rb | 93 +++------------ 9 files changed, 226 insertions(+), 227 deletions(-) create mode 100644 modules/avatars/app/components/avatars/form_section_component.html.erb create mode 100644 modules/avatars/app/components/avatars/form_section_component.rb delete mode 100644 modules/avatars/app/views/avatars/users/_avatar_fields.html.erb create mode 100644 modules/avatars/spec/components/avatars/form_section_component_spec.rb diff --git a/frontend/src/global_styles/content/modules/_avatars.sass b/frontend/src/global_styles/content/modules/_avatars.sass index 30a05d7370b8..4ac224634b86 100644 --- a/frontend/src/global_styles/content/modules/_avatars.sass +++ b/frontend/src/global_styles/content/modules/_avatars.sass @@ -1,18 +1,8 @@ -.avatars--layout - display: flex - align-items: flex-start - gap: 1.5rem - margin-bottom: 1rem - .avatars--media - display: flex + display: inline-flex flex-direction: column align-items: center gap: 0.25rem - flex-shrink: 0 - -.avatars--description - flex: 1 .avatars--current-avatar display: inline-block diff --git a/modules/avatars/app/components/avatars/form_section_component.html.erb b/modules/avatars/app/components/avatars/form_section_component.html.erb new file mode 100644 index 000000000000..a0da80ae064f --- /dev/null +++ b/modules/avatars/app/components/avatars/form_section_component.html.erb @@ -0,0 +1,35 @@ +<%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> + <% subhead.with_heading(tag: :h3, size: :medium) { t("avatars.label_avatar") } %> + <% subhead.with_description { description } %> +<% end %> + +
+ <% if manager.local_avatars_enabled? %> + <%# Clicking the avatar activates the hidden file input rendered by the + upload form below; the upload then fires as soon as a file is selected. %> + + + <% if helpers.local_avatar?(@user) %> + <%= form_with( + url: @target_avatar_path, + method: :delete, + data: { turbo_confirm: t(:are_you_sure_delete_avatar) } + ) do %> + <%= render( + Primer::Beta::Button.new( + type: :submit, + scheme: :link, + test_selector: "avatar-delete-link" + ) + ) { t(:button_delete) } %> + <% end %> + <% end %> + <% else %> +
+ <%= helpers.avatar(@user, hover_card: { active: false }) %> +
+ <% end %> +
diff --git a/modules/avatars/app/components/avatars/form_section_component.rb b/modules/avatars/app/components/avatars/form_section_component.rb new file mode 100644 index 000000000000..197e33536dc4 --- /dev/null +++ b/modules/avatars/app/components/avatars/form_section_component.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Avatars + class FormSectionComponent < ApplicationComponent + def initialize(user:, target_avatar_path:) + super() + @user = user + @target_avatar_path = target_avatar_path + end + + def render? + manager.avatars_enabled? + end + + def description + parts = [] + if manager.gravatar_enabled? + gravatar_link = link_to("gravatar.com", "https://gravatar.com") + parts << t("avatars.text_avatar_gravatar_html", gravatar_url: gravatar_link) + end + if manager.local_avatars_enabled? + parts << (manager.gravatar_enabled? ? t("avatars.text_avatar_local") : t("avatars.text_avatar_local_only")) + end + helpers.safe_join(parts, " ") + end + + private + + def manager + ::OpenProject::Avatars::AvatarManager + end + end +end diff --git a/modules/avatars/app/views/avatars/my/_avatar_section.html.erb b/modules/avatars/app/views/avatars/my/_avatar_section.html.erb index 22db61596f8a..479c40a8bac0 100644 --- a/modules/avatars/app/views/avatars/my/_avatar_section.html.erb +++ b/modules/avatars/app/views/avatars/my/_avatar_section.html.erb @@ -1,8 +1 @@ -<% manager = ::OpenProject::Avatars::AvatarManager %> -<% if manager.avatars_enabled? %> - <%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> - <% subhead.with_heading(tag: :h3, size: :medium) { t("avatars.label_avatar") } %> - <% end %> - - <%= render "avatars/users/avatar_fields", user:, target_avatar_path: edit_my_avatar_path %> -<% end %> +<%= render(Avatars::FormSectionComponent.new(user:, target_avatar_path: edit_my_avatar_path)) %> diff --git a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb b/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb deleted file mode 100644 index 6969420c72b3..000000000000 --- a/modules/avatars/app/views/avatars/users/_avatar_fields.html.erb +++ /dev/null @@ -1,44 +0,0 @@ -<%# locals: (user:, target_avatar_path:) -%> -<% manager = ::OpenProject::Avatars::AvatarManager %> - -
-
- <% if manager.local_avatars_enabled? %> - <%# Clicking the avatar activates the hidden file input rendered by the - component below; the upload then fires as soon as a file is selected. %> - - - <% if local_avatar?(user) %> - <%= form_with( - url: target_avatar_path, - method: :delete, - data: { turbo_confirm: t(:are_you_sure_delete_avatar) } - ) do %> - <%= render( - Primer::Beta::Button.new( - type: :submit, - scheme: :link, - test_selector: "avatar-delete-link" - ) - ) { t(:button_delete) } %> - <% end %> - <% end %> - <% else %> -
- <%= avatar(user, hover_card: { active: false }) %> -
- <% end %> -
- -
- <% if manager.gravatar_enabled? %> - <%= t("avatars.text_avatar_gravatar_html", gravatar_url: link_to("gravatar.com", "https://gravatar.com")) %> - <% end %> - <% if manager.local_avatars_enabled? %> - <%= manager.gravatar_enabled? ? t("avatars.text_avatar_local") : t("avatars.text_avatar_local_only") %> - <% end %> -
-
diff --git a/modules/avatars/app/views/avatars/users/_avatar_section.html.erb b/modules/avatars/app/views/avatars/users/_avatar_section.html.erb index 37357a488ed8..5e0da19066ba 100644 --- a/modules/avatars/app/views/avatars/users/_avatar_section.html.erb +++ b/modules/avatars/app/views/avatars/users/_avatar_section.html.erb @@ -1,8 +1,3 @@ -<% manager = ::OpenProject::Avatars::AvatarManager %> -<% if User.current.admin? && manager.avatars_enabled? %> - <%= render(Primer::Beta::Subhead.new(mb: 2)) do |subhead| %> - <% subhead.with_heading(tag: :h3, size: :medium) { t("avatars.label_avatar") } %> - <% end %> - - <%= render "avatars/users/avatar_fields", user:, target_avatar_path: edit_user_avatar_path(user) %> +<% if User.current.admin? %> + <%= render(Avatars::FormSectionComponent.new(user:, target_avatar_path: edit_user_avatar_path(user))) %> <% end %> diff --git a/modules/avatars/spec/components/avatars/form_section_component_spec.rb b/modules/avatars/spec/components/avatars/form_section_component_spec.rb new file mode 100644 index 000000000000..3462d843ad48 --- /dev/null +++ b/modules/avatars/spec/components/avatars/form_section_component_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Avatars::FormSectionComponent, type: :component do + let(:user) { build_stubbed(:user) } + let(:enable_gravatars) { false } + let(:enable_local_avatars) { false } + + before do + allow(Setting) + .to receive(:plugin_openproject_avatars) + .and_return("enable_gravatars" => enable_gravatars, "enable_local_avatars" => enable_local_avatars) + end + + def render_section + render_inline(described_class.new(user:, target_avatar_path: "/users/1/avatar")) + end + + context "when avatars are disabled" do + it "renders nothing" do + render_section + + expect(page).to have_no_css("h3", text: "Avatar") + end + end + + context "when only gravatars are enabled" do + let(:enable_gravatars) { true } + + it "renders the heading and the Gravatar hint, but no upload trigger" do + render_section + + expect(page).to have_css("h3", text: "Avatar") + expect(page).to have_css(".avatars--current-avatar") + expect(page).to have_css(".Subhead-description", text: "Gravatar") + expect(page).to have_link("gravatar.com") + expect(page).to have_no_css(".avatars--upload-trigger") + expect(page).to have_no_css("opce-avatar-upload-form") + end + end + + context "when only local avatars are enabled" do + let(:enable_local_avatars) { true } + + it "renders the upload trigger pointed at the target path and the local-only hint" do + render_section + + expect(page).to have_css(".avatars--upload-trigger") + expect(page).to have_css("opce-avatar-upload-form[target='/users/1/avatar']") + expect(page).to have_css(".Subhead-description", text: I18n.t("avatars.text_avatar_local_only")) + expect(page).to have_no_link("gravatar.com") + end + end + + context "when both gravatars and local avatars are enabled" do + let(:enable_gravatars) { true } + let(:enable_local_avatars) { true } + + it "renders both the Gravatar hint and the upload trigger with the override hint" do + render_section + + expect(page).to have_link("gravatar.com") + expect(page).to have_css(".avatars--upload-trigger") + expect(page).to have_css(".Subhead-description", text: I18n.t("avatars.text_avatar_local")) + end + end + + context "when the user already has a local avatar" do + let(:enable_local_avatars) { true } + + before { allow(user).to receive(:local_avatar_attachment).and_return(true) } + + it "renders the delete control" do + render_section + + expect(page).to have_css("[data-test-selector='avatar-delete-link']") + end + end +end diff --git a/modules/avatars/spec/features/my_avatar_spec.rb b/modules/avatars/spec/features/my_avatar_spec.rb index 9d69cb48cdf9..d3c3a0dc83c5 100644 --- a/modules/avatars/spec/features/my_avatar_spec.rb +++ b/modules/avatars/spec/features/my_avatar_spec.rb @@ -3,6 +3,10 @@ require "spec_helper" require "mini_magick" +# The avatar section's rendering across the gravatar/local/both/none settings is +# covered by Avatars::FormSectionComponent's component spec. This feature spec only +# exercises the self-service round-trip that needs a real browser and the my-avatar +# controller: client-side validation, upload, server-side resize, and deletion. RSpec.describe "My avatar management", :js do include Rails.application.routes.url_helpers @@ -10,100 +14,44 @@ let(:user) { create(:user) } let(:avatar_management_path) { my_account_path } - let(:enable_gravatars) { false } - let(:enable_local_avatars) { false } - let(:plugin_settings) do - { - "enable_gravatars" => enable_gravatars, - "enable_local_avatars" => enable_local_avatars - } - end - before do login_as user allow(Setting) .to receive(:plugin_openproject_avatars) - .and_return(plugin_settings) - end - - describe "only gravatars enabled" do - let(:enable_gravatars) { true } - - it "shows the Gravatar hint and the current avatar but no upload field" do - visit avatar_management_path - - expect(page).to have_css(".avatars--current-avatar") - expect(page).to have_link("gravatar.com") - expect(page).to have_no_field("avatar_file_input") - end + .and_return("enable_gravatars" => false, "enable_local_avatars" => true) end - describe "only local avatars enabled" do - let(:enable_local_avatars) { true } - - it "can upload and delete a custom avatar" do - visit avatar_management_path - expect(page).to have_css(".avatars--upload-trigger") - - # Gravatar hint is not rendered - expect(page).to have_no_link("gravatar.com") - - # Attach a new invalid image - attach_file("avatar_file_input", - UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path, - make_visible: true) + it "validates the format, then uploads, resizes and deletes a custom avatar" do + visit avatar_management_path + expect(page).to have_css(".avatars--upload-trigger") - # Expect error - expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") + # An invalid file is rejected client-side + attach_file("avatar_file_input", + UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path, + make_visible: true) - # Attach a valid image; it is uploaded as soon as it is selected - visit avatar_management_path - attach_file("avatar_file_input", - UploadedFile.load_from(File.join(image_base_path, "too_big.jpg")).path, - make_visible: true) + expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") - # Expect the avatar to be uploaded and resized - expect(page).to have_test_selector("avatar-delete-link", wait: 20) - avatar_path = user.local_avatar_attachment.file.path - content_type = OpenProject::ContentTypeDetector.new(avatar_path).detect - image = MiniMagick::Image.open(avatar_path) + # A valid image is uploaded as soon as it is selected + visit avatar_management_path + attach_file("avatar_file_input", + UploadedFile.load_from(File.join(image_base_path, "too_big.jpg")).path, + make_visible: true) - expect(image.dimensions).to eq [128, 128] - expect(content_type).to eq("image/jpeg") + # It is uploaded and resized to the avatar dimensions + expect(page).to have_test_selector("avatar-delete-link", wait: 20) + avatar_path = user.local_avatar_attachment.file.path + content_type = OpenProject::ContentTypeDetector.new(avatar_path).detect + image = MiniMagick::Image.open(avatar_path) - # Delete the avatar - accept_alert do - find_test_selector("avatar-delete-link").click - end + expect(image.dimensions).to eq [128, 128] + expect(content_type).to eq("image/jpeg") - expect(page).to have_no_test_selector("avatar-delete-link", wait: 20) - end - end - - describe "both gravatars and local avatars enabled" do - let(:enable_gravatars) { true } - let(:enable_local_avatars) { true } - - it "renders the Gravatar hint and the upload trigger in a single section" do - visit avatar_management_path - - expect(page).to have_link("gravatar.com") - expect(page).to have_css(".avatars--upload-trigger") - end - end - - describe "none enabled" do - before do - allow(Setting) - .to receive(:plugin_openproject_avatars) - .and_return({}) + # And can be deleted again + accept_alert do + find_test_selector("avatar-delete-link").click end - it "does not render the avatar section on the account page" do - visit my_account_path - - expect(page).to have_text(I18n.t(:label_my_account)) - expect(page).to have_no_css(".avatars--current-avatar") - end + expect(page).to have_no_test_selector("avatar-delete-link", wait: 20) end end diff --git a/modules/avatars/spec/features/user_avatar_spec.rb b/modules/avatars/spec/features/user_avatar_spec.rb index 1262d1d00f00..fcc15a06889d 100644 --- a/modules/avatars/spec/features/user_avatar_spec.rb +++ b/modules/avatars/spec/features/user_avatar_spec.rb @@ -2,106 +2,47 @@ require "spec_helper" +# The avatar section's rendering across the gravatar/local/both/none settings is +# covered exhaustively by Avatars::FormSectionComponent's component spec. These +# feature specs only exercise what needs a real browser and the users avatar +# controller: client-side file validation, the delete round-trip, and access control. RSpec.describe "User avatar management", :js do include Rails.application.routes.url_helpers let(:image_base_path) { File.expand_path("#{File.dirname(__FILE__)}/../fixtures/") } let(:avatar_management_path) { edit_user_path(target_user) } - let(:enable_gravatars) { false } - let(:enable_local_avatars) { false } - let(:plugin_settings) do - { - "enable_gravatars" => enable_gravatars, - "enable_local_avatars" => enable_local_avatars - } - end - before do login_as user allow(Setting) .to receive(:plugin_openproject_avatars) - .and_return(plugin_settings) + .and_return("enable_gravatars" => false, "enable_local_avatars" => true) end context "when user is admin" do let(:user) { create(:admin) } let(:target_user) { create(:user) } - describe "only gravatars enabled" do - let(:enable_gravatars) { true } + it "rejects a file with an invalid format" do + visit avatar_management_path - it "shows the avatar section with the Gravatar hint but no upload field" do - visit avatar_management_path + attach_file("avatar_file_input", + UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path, + make_visible: true) - expect(page).to have_css("h2", text: "Avatar") - expect(page).to have_css(".avatars--current-avatar") - expect(page).to have_css(".avatars--description", text: "Gravatar") - expect(page).to have_no_field("avatar_file_input") - end + expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") end - describe "only local avatars enabled" do - let(:enable_local_avatars) { true } - - it "exposes the avatar as an upload trigger and validates the file format" do - visit avatar_management_path - expect(page).to have_css("h2", text: "Avatar") - expect(page).to have_css(".avatars--upload-trigger") + it "deletes an existing custom avatar" do + target_user.attachments = [build(:avatar_attachment, author: target_user)] - # Gravatar hint is not rendered - expect(page).to have_no_link("gravatar.com") + visit avatar_management_path - # The upload component is active on this tab and rejects invalid files - attach_file("avatar_file_input", - UploadedFile.load_from(File.join(image_base_path, "invalid.txt")).path, - make_visible: true) - - expect(page).to have_css(".avatars--error-pane", text: "Allowed formats are jpg, png, gif") + accept_alert do + find_test_selector("avatar-delete-link").click end - it "offers to delete an existing custom avatar" do - target_user.attachments = [build(:avatar_attachment, author: target_user)] - - visit avatar_management_path - - accept_alert do - find_test_selector("avatar-delete-link").click - end - - expect(page).to have_no_test_selector("avatar-delete-link", wait: 20) - end - end - - describe "both gravatars and local avatars enabled" do - let(:enable_gravatars) { true } - let(:enable_local_avatars) { true } - - it "renders the Gravatar hint and the upload trigger in a single section" do - visit avatar_management_path - - expect(page).to have_css("h2", text: "Avatar") - expect(page).to have_css(".avatars--description", text: "Gravatar") - expect(page).to have_css(".avatars--upload-trigger") - end - end - - describe "none enabled" do - it "does not render the avatar section" do - visit avatar_management_path - expect(page).to have_button(I18n.t(:button_save)) - expect(page).to have_no_css("h2", text: "Avatar") - end - end - end - - context "when user is self" do - let(:user) { create(:user) } - let(:target_user) { user } - - it "forbids the user to access" do - visit edit_user_path(target_user) - expect(page).to have_text("[Error 403]") + expect(page).to have_no_test_selector("avatar-delete-link", wait: 20) end end From e01844016716088892afd3ca9bcc37159f2b508e Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 23 Jun 2026 17:08:59 +0200 Subject: [PATCH 11/14] Consequently use mb-3 spacing --- app/forms/users/form/attributes_form.rb | 2 +- app/views/my/account.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/forms/users/form/attributes_form.rb b/app/forms/users/form/attributes_form.rb index de8da3caf2b8..5cd91fd1b790 100644 --- a/app/forms/users/form/attributes_form.rb +++ b/app/forms/users/form/attributes_form.rb @@ -93,7 +93,7 @@ def user_sections(form) def render_section(form, section) visible_cfs_by_key = section.custom_fields.visible(User.current).index_by(&:column_name) - form.fieldset_group(title: section_title(section)) do |group| + form.fieldset_group(title: section_title(section), mb: 3) do |group| section.attribute_order.each do |key| if UserCustomFieldSection::BUILT_IN_ATTRIBUTES.include?(key) render_built_in(group, key) diff --git a/app/views/my/account.html.erb b/app/views/my/account.html.erb index 6887d48e7855..cfe54c27176e 100644 --- a/app/views/my/account.html.erb +++ b/app/views/my/account.html.erb @@ -59,7 +59,7 @@ See COPYRIGHT and LICENSE files for more details. <%# Constrain hook content (the avatar section) to the same width as the form below, and separate it from the form with the same gap as sits between the form sections (mb-6). %> -<%= content_tag(:div, class: "op-admin-settings-form-wrapper mb-6") do %> +<%= render(Primer::BaseComponent.new(tag: :div, classes: "op-admin-settings-form-wrapper", mb: 3)) do %> <%= call_hook(:view_my_account_top, user: current_user) %> <% end %> From 47a85a1c86cf73a51fd24674608c5b1804a341c5 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 23 Jun 2026 17:19:19 +0200 Subject: [PATCH 12/14] Use a date picker instead of a date field for date CFs --- app/forms/custom_fields/inputs/date.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/forms/custom_fields/inputs/date.rb b/app/forms/custom_fields/inputs/date.rb index 381b58ba6061..8e6b561b1edd 100644 --- a/app/forms/custom_fields/inputs/date.rb +++ b/app/forms/custom_fields/inputs/date.rb @@ -30,10 +30,14 @@ class CustomFields::Inputs::Date < CustomFields::Inputs::Base::Input form do |custom_value_form| - custom_value_form.text_field(**input_attributes) + custom_value_form.single_date_picker(**input_attributes) end def input_attributes - super.merge({ input_width: :xsmall, type: "date" }) + super.merge({ input_width: :medium, leading_visual: { icon: :calendar } }) + end + + def value + custom_value.typed_value&.iso8601 end end From d7a645cf470d3821439a50e353204b4b711d417f Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 23 Jun 2026 17:23:48 +0200 Subject: [PATCH 13/14] Consistent spacing in the user edit admin page --- app/forms/users/form/authentication_form.rb | 2 +- app/forms/users/form/consent_form.rb | 2 +- app/forms/users/form/preferences_form.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/forms/users/form/authentication_form.rb b/app/forms/users/form/authentication_form.rb index a566d206f73c..469286715813 100644 --- a/app/forms/users/form/authentication_form.rb +++ b/app/forms/users/form/authentication_form.rb @@ -44,7 +44,7 @@ module Form # - a notice that the user cannot log in when password login is disabled. class AuthenticationForm < ApplicationForm form do |f| - f.fieldset_group(title: I18n.t(:label_authentication)) do |group| + f.fieldset_group(title: I18n.t(:label_authentication), mb: 3) do |group| if @render_external_auth external_authentication(group) else diff --git a/app/forms/users/form/consent_form.rb b/app/forms/users/form/consent_form.rb index 7da59f26684b..3e3b16d6c297 100644 --- a/app/forms/users/form/consent_form.rb +++ b/app/forms/users/form/consent_form.rb @@ -34,7 +34,7 @@ module Form # fieldset heading and spacing of the surrounding administration user form. class ConsentForm < ApplicationForm form do |f| - f.fieldset_group(title: I18n.t("consent.title")) do |group| + f.fieldset_group(title: I18n.t("consent.title"), mb: 3) do |group| group.html_content do render(consent_status_component) end diff --git a/app/forms/users/form/preferences_form.rb b/app/forms/users/form/preferences_form.rb index 019c787f7829..aee37462e256 100644 --- a/app/forms/users/form/preferences_form.rb +++ b/app/forms/users/form/preferences_form.rb @@ -38,7 +38,7 @@ module Form # option building mirrors My::TimeZoneForm. class PreferencesForm < ApplicationForm form do |f| - f.fieldset_group(title: I18n.t(:label_preferences)) do |group| + f.fieldset_group(title: I18n.t(:label_preferences), mb: 3) do |group| group.select_list( name: :time_zone, label: UserPreference.human_attribute_name(:time_zone), From 88505b28fad6fbbed7683894a2d4884437b1c0e7 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 23 Jun 2026 17:35:47 +0200 Subject: [PATCH 14/14] Revert "Use a date picker instead of a date field for date CFs" This reverts commit 47a85a1c86cf73a51fd24674608c5b1804a341c5. --- app/forms/custom_fields/inputs/date.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/forms/custom_fields/inputs/date.rb b/app/forms/custom_fields/inputs/date.rb index 8e6b561b1edd..381b58ba6061 100644 --- a/app/forms/custom_fields/inputs/date.rb +++ b/app/forms/custom_fields/inputs/date.rb @@ -30,14 +30,10 @@ class CustomFields::Inputs::Date < CustomFields::Inputs::Base::Input form do |custom_value_form| - custom_value_form.single_date_picker(**input_attributes) + custom_value_form.text_field(**input_attributes) end def input_attributes - super.merge({ input_width: :medium, leading_visual: { icon: :calendar } }) - end - - def value - custom_value.typed_value&.iso8601 + super.merge({ input_width: :xsmall, type: "date" }) end end