From 1f78f5ae4dd6b5cf227d85e9f818ca84ca20f64d Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 2 Apr 2026 09:57:03 +0200 Subject: [PATCH] use preference service inside user update service - for backlogs as well --- app/controllers/my_controller.rb | 5 +- app/models/permitted_params.rb | 4 -- app/services/users/update_service.rb | 5 +- .../backlogs/app/forms/my/backlogs_form.rb | 22 ++------ .../views/shared/_view_my_settings.html.erb | 4 +- modules/backlogs/config/locales/en.yml | 4 +- .../lib/open_project/backlogs/engine.rb | 4 +- .../backlogs/hooks/layout_hook.rb | 5 +- .../backlogs/hooks/user_settings_hook.rb | 51 ------------------- .../patches/permitted_params_patch.rb | 5 -- spec/services/users/update_service_spec.rb | 4 +- 11 files changed, 21 insertions(+), 92 deletions(-) delete mode 100644 modules/backlogs/lib/open_project/backlogs/hooks/user_settings_hook.rb diff --git a/app/controllers/my_controller.rb b/app/controllers/my_controller.rb index 3d3e0f51a32..290fba911ac 100644 --- a/app/controllers/my_controller.rb +++ b/app/controllers/my_controller.rb @@ -158,7 +158,10 @@ class MyController < ApplicationController end def user_params - permitted_params.my_account_settings.to_h + # 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: {})) end def notice_account_updated diff --git a/app/models/permitted_params.rb b/app/models/permitted_params.rb index 7b8ea63e6d1..5870bdd695a 100644 --- a/app/models/permitted_params.rb +++ b/app/models/permitted_params.rb @@ -199,10 +199,6 @@ class PermittedParams params.require(:placeholder_user).permit(*self.class.permitted_attributes[:placeholder_user]) end - def my_account_settings - user.merge(pref:) - end - def user_register_via_omniauth permitted_params = params .require(:user) diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index d16275e148d..cc0fc06d36f 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -45,8 +45,9 @@ module Users def persist(_service_result) service_result = super - if service_result.success? - service_result.success = model.pref.save + if service_result.success? && params[:pref].present? + preference_service = UserPreferences::UpdateService.new(user:, model: model.pref) + service_result.add_dependent!(preference_service.call(params[:pref])) end service_result diff --git a/modules/backlogs/app/forms/my/backlogs_form.rb b/modules/backlogs/app/forms/my/backlogs_form.rb index 4ae5176059d..15579e112f0 100644 --- a/modules/backlogs/app/forms/my/backlogs_form.rb +++ b/modules/backlogs/app/forms/my/backlogs_form.rb @@ -30,16 +30,14 @@ class My::BacklogsForm < ApplicationForm form do |f| - f.text_field name: :task_color, - label: I18n.t("backlogs.task_color"), - value: @color, + f.text_field name: :backlogs_task_color, + label: I18n.t("activerecord.attributes.user_preference.backlogs_task_color"), input_width: :xsmall - f.check_box name: :versions_default_fold_state, + f.check_box name: :backlogs_versions_default_fold_state, value: DEFAULT_FOLD_STATE, unchecked_value: DEFAULT_EXPAND_STATE, - checked: default_fold_state_checked?, - label: I18n.t("backlogs.label_sprints_default_fold_state"), + label: I18n.t("activerecord.attributes.user_preference.backlogs_versions_default_fold_state"), caption: I18n.t("backlogs.caption_sprints_default_fold_state") f.submit(name: :submit, label: I18n.t("backlogs.user_preference.button_update_backlogs"), scheme: :default) @@ -47,16 +45,4 @@ class My::BacklogsForm < ApplicationForm DEFAULT_FOLD_STATE = "closed" DEFAULT_EXPAND_STATE = "open" - - def initialize(color:, versions_default_fold_state:) - super() - @color = color - @versions_default_fold_state = versions_default_fold_state - end - - private - - def default_fold_state_checked? - @versions_default_fold_state == DEFAULT_FOLD_STATE - end end diff --git a/modules/backlogs/app/views/shared/_view_my_settings.html.erb b/modules/backlogs/app/views/shared/_view_my_settings.html.erb index c878554ece4..569bfb700ae 100644 --- a/modules/backlogs/app/views/shared/_view_my_settings.html.erb +++ b/modules/backlogs/app/views/shared/_view_my_settings.html.erb @@ -33,7 +33,7 @@ See COPYRIGHT and LICENSE files for more details. end %> <%= - settings_primer_form_with(model: user.pref, scope: :backlogs, url: { action: "update_settings" }, data: { turbo: false }) do |form| - render(My::BacklogsForm.new(form, color:, versions_default_fold_state:)) + settings_primer_form_with(model: user.pref, scope: :pref, url: { action: "update_settings" }, data: { turbo: false }) do |form| + render(My::BacklogsForm.new(form)) end %> diff --git a/modules/backlogs/config/locales/en.yml b/modules/backlogs/config/locales/en.yml index 1a86d7c1d77..22ac6cb0218 100644 --- a/modules/backlogs/config/locales/en.yml +++ b/modules/backlogs/config/locales/en.yml @@ -48,6 +48,9 @@ en: sprint_sharing: "Sprint sharing" sprint: duration: "Sprint duration" + user_preference: + backlogs_task_color: "Task color" + backlogs_versions_default_fold_state: "Show sprints folded" work_package: backlogs_work_package_type: "Backlog type" position: "Position" @@ -103,7 +106,6 @@ en: sharing_description: "This project can either share its own sprints, receive shared sprints or handle sprints independently (no sharing)." sharing: "Sharing" impediment: "Impediment" - label_sprints_default_fold_state: "Show sprints folded" caption_sprints_default_fold_state: "Sprints will not be expanded by default when viewing the 'Backlog and sprints' page. Each one has to be manually expanded." work_package_is_closed: "Work package is done, when" label_is_done_status: "Status %{status_name} means done" diff --git a/modules/backlogs/lib/open_project/backlogs/engine.rb b/modules/backlogs/lib/open_project/backlogs/engine.rb index 2c67d5012f3..bec225a9c64 100644 --- a/modules/backlogs/lib/open_project/backlogs/engine.rb +++ b/modules/backlogs/lib/open_project/backlogs/engine.rb @@ -179,7 +179,8 @@ module OpenProject::Backlogs "definitions/UserPreferences/properties", { "backlogs_task_color" => { - "type" => "string" + "type" => "string", + "pattern" => "^#[0-9a-fA-F]{6}$" }, "backlogs_versions_default_fold_state" => { "type" => "string", @@ -228,7 +229,6 @@ module OpenProject::Backlogs config.to_prepare do OpenProject::Backlogs::Hooks::LayoutHook - OpenProject::Backlogs::Hooks::UserSettingsHook end initializer "openproject_backlogs.event_subscriptions" do diff --git a/modules/backlogs/lib/open_project/backlogs/hooks/layout_hook.rb b/modules/backlogs/lib/open_project/backlogs/hooks/layout_hook.rb index e4271417473..2aa40f2bb86 100644 --- a/modules/backlogs/lib/open_project/backlogs/hooks/layout_hook.rb +++ b/modules/backlogs/lib/open_project/backlogs/hooks/layout_hook.rb @@ -35,10 +35,7 @@ module OpenProject::Backlogs::Hooks :render_to_string, partial: "shared/view_my_settings", locals: { - user: context[:user], - color: context[:user].backlogs_preference(:task_color), - versions_default_fold_state: - context[:user].backlogs_preference(:versions_default_fold_state) + user: context[:user] } ) end diff --git a/modules/backlogs/lib/open_project/backlogs/hooks/user_settings_hook.rb b/modules/backlogs/lib/open_project/backlogs/hooks/user_settings_hook.rb deleted file mode 100644 index b78dc201f5b..00000000000 --- a/modules/backlogs/lib/open_project/backlogs/hooks/user_settings_hook.rb +++ /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. -#++ - -class OpenProject::Backlogs::Hooks::UserSettingsHook < OpenProject::Hook::ViewListener - # Updates the backlogs settings before saving the user - # - # Context: - # * params => Request parameters - # * permitted_params => whitelisted params - # * user => user being altered - def service_update_user_before_save(context = {}) - params = context[:params] - user = context[:user] - - backlogs_params = params.delete(:backlogs) - return unless backlogs_params - - versions_default_fold_state = backlogs_params[:versions_default_fold_state] || "open" - user.backlogs_preference(:versions_default_fold_state, versions_default_fold_state) - - color = backlogs_params[:task_color] || "" - if color == "" || color.match(/^#[A-Fa-f0-9]{6}$/) - user.backlogs_preference(:task_color, color) - end - end -end diff --git a/modules/backlogs/lib/open_project/backlogs/patches/permitted_params_patch.rb b/modules/backlogs/lib/open_project/backlogs/patches/permitted_params_patch.rb index b5c57ca8d0b..6ac3546d83c 100644 --- a/modules/backlogs/lib/open_project/backlogs/patches/permitted_params_patch.rb +++ b/modules/backlogs/lib/open_project/backlogs/patches/permitted_params_patch.rb @@ -41,11 +41,6 @@ module OpenProject::Backlogs::Patches::PermittedParamsPatch permitted_params end - def my_account_settings - backlogs_params = params.fetch(:backlogs, {}).permit(:task_color, :versions_default_fold_state) - super.merge(backlogs: backlogs_params) - end - def backlogs_admin_settings params .require(:settings) diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index 11baf2b5ff0..52ca7e7c10b 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -43,7 +43,7 @@ RSpec.describe Users::UpdateService do let(:current_user) { create(:admin) } let(:update_user) { create(:user, mail: "correct@example.org") } - subject { instance.call(attributes:) } + subject { instance.call(**attributes) } context "when invalid" do let(:attributes) { { mail: "invalid" } } @@ -97,7 +97,7 @@ RSpec.describe Users::UpdateService do end describe "updating prefs" do - let(:attributes) { {} } + let(:attributes) { { pref: { increase_theme_contrast: 0 } } } before do allow(update_user).to receive(:save).and_return(user_save_result)