diff --git a/app/contracts/settings/working_days_params_contract.rb b/app/contracts/settings/working_days_params_contract.rb index df8ed0099cf..24b01cb0381 100644 --- a/app/contracts/settings/working_days_params_contract.rb +++ b/app/contracts/settings/working_days_params_contract.rb @@ -36,7 +36,7 @@ module Settings protected def working_days_are_present - if working_days.empty? + if working_days.blank? errors.add :base, :working_days_are_missing end end diff --git a/app/controllers/admin/settings/working_days_settings_controller.rb b/app/controllers/admin/settings/working_days_settings_controller.rb index 4a9842ced3c..c20146f0568 100644 --- a/app/controllers/admin/settings/working_days_settings_controller.rb +++ b/app/controllers/admin/settings/working_days_settings_controller.rb @@ -12,14 +12,34 @@ module Admin::Settings true end + def failure_callback(call) + @modified_non_working_days = call.result + flash[:error] = call.message || I18n.t(:notice_internal_server_error) + render action: 'show', tab: params[:tab] + end + + protected + def settings_params settings = super - settings[:working_days] = settings[:working_days].compact_blank.map(&:to_i).uniq + settings[:working_days] = working_days_params(settings) + settings[:non_working_days] = non_working_days_params settings end - def contract_options - { params_contract: Settings::WorkingDaysParamsContract } + def update_service + ::Settings::WorkingDaysUpdateService + end + + private + + def working_days_params(settings) + settings[:working_days] ? settings[:working_days].compact_blank.map(&:to_i).uniq : [] + end + + def non_working_days_params + non_working_days = params[:settings].to_unsafe_hash[:non_working_days] || {} + non_working_days.to_h.values end end end diff --git a/app/controllers/admin/settings_controller.rb b/app/controllers/admin/settings_controller.rb index 5a69f1ca603..7bee5fdd5c2 100644 --- a/app/controllers/admin/settings_controller.rb +++ b/app/controllers/admin/settings_controller.rb @@ -50,13 +50,12 @@ module Admin def update return unless params[:settings] - call = ::Settings::UpdateService - .new(user: current_user, contract_options:) + call = update_service + .new(user: current_user) .call(settings_params) - call.on_success { flash[:notice] = t(:notice_successful_update) } - call.on_failure { flash[:error] = call.message || I18n.t(:notice_internal_server_error) } - redirect_to action: 'show', tab: params[:tab] + call.on_success { success_callback(call) } + call.on_failure { failure_callback(call) } end def show_plugin @@ -94,8 +93,18 @@ module Admin permitted_params.settings.to_h end - def contract_options - {} + def update_service + ::Settings::UpdateService + end + + def success_callback(_call) + flash[:notice] = t(:notice_successful_update) + redirect_to action: 'show', tab: params[:tab] + end + + def failure_callback(call) + flash[:error] = call.message || I18n.t(:notice_internal_server_error) + redirect_to action: 'show', tab: params[:tab] end end end diff --git a/app/services/settings/update_service.rb b/app/services/settings/update_service.rb index e48fd7eb43a..66a2650069d 100644 --- a/app/services/settings/update_service.rb +++ b/app/services/settings/update_service.rb @@ -26,30 +26,23 @@ # See COPYRIGHT and LICENSE files for more details. #++ -class Settings::UpdateService < ::BaseServices::BaseContracted - def initialize(user:, contract_options: {}) +class Settings::UpdateService < BaseServices::BaseContracted + def initialize(user:) super user:, - contract_options:, contract_class: Settings::UpdateContract end - def validate_params(params) - if contract_options[:params_contract] - contract = contract_options[:params_contract].new(model, user, params:) - ServiceResult.new success: contract.valid?, - errors: contract.errors, - result: model - else - super - end + def after_validate(params, call) + params.keys.each(&method(:remember_previous_value)) + call end - def after_validate(params, call) + # We will have a problem with error handling on the form. + # How can we still display the user changed values in case the form is not successfully saved? + def persist(call) params.each do |name, value| - remember_previous_value(name) set_setting_value(name, value) end - call end diff --git a/app/services/settings/working_days_update_service.rb b/app/services/settings/working_days_update_service.rb new file mode 100644 index 00000000000..dfc94ca927f --- /dev/null +++ b/app/services/settings/working_days_update_service.rb @@ -0,0 +1,98 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 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 Settings::WorkingDaysUpdateService < Settings::UpdateService + def call(params) + params = params.to_h.deep_symbolize_keys + self.non_working_days_params = params.delete(:non_working_days) || [] + super + end + + def validate_params(params) + contract = Settings::WorkingDaysParamsContract.new(model, user, params:) + ServiceResult.new success: contract.valid?, + errors: contract.errors, + result: model + end + + def persist(call) + results = call + ActiveRecord::Base.transaction do + # The order of merging the service is important to preserve + # the errors model's base object, which is a NonWorkingDay + results = persist_non_working_days + results.merge!(super) if results.success? + + raise ActiveRecord::Rollback if results.failure? + end + + results + end + + private + + attr_accessor :non_working_days_params + + def persist_non_working_days + # We don't support update for now + to_create, to_delete = attributes_to_create_and_delete + results = destroy_records(to_delete) + create_results = create_records(to_create) + results.merge!(create_results) + results.result = Array(results.result) + Array(create_results.result) + results + end + + def attributes_to_create_and_delete + non_working_days_params.reduce([[], []]) do |results, nwd| + results.first << nwd if !nwd[:id] + results.last << nwd[:id] if nwd[:_destroy] && nwd[:id] + results + end + end + + def create_records(attributes) + wrap_result(attributes.map { |attrs| NonWorkingDay.create(attrs) }) + end + + def destroy_records(ids) + wrap_result NonWorkingDay.where(id: ids).destroy_all + end + + def wrap_result(result) + model = NonWorkingDay.new + errors = model.errors.tap do |err| + result.each do |r| + err.merge!(r.errors) + end + end + success = model.errors.empty? + + ServiceResult.new(success:, errors:, result:) + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 6a1488e44dd..3ea0f2e97b3 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -757,6 +757,11 @@ en: no_notification_reason: 'cannot be blank as IAN is chosen as a channel.' reason_mail_digest: no_notification_reason: 'cannot be blank as mail digest is chosen as a channel.' + non_working_day: + attributes: + date: + taken: "A non-working day already exists for %{value}." + format: "%{message}" parse_schema_filter_params_service: attributes: base: diff --git a/spec/controllers/admin/settings/working_days_settings_controller_spec.rb b/spec/controllers/admin/settings/working_days_settings_controller_spec.rb new file mode 100644 index 00000000000..6a2f6f46cfb --- /dev/null +++ b/spec/controllers/admin/settings/working_days_settings_controller_spec.rb @@ -0,0 +1,117 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2022 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' + +describe Admin::Settings::WorkingDaysSettingsController do + shared_let(:user) { create(:admin) } + + current_user { user } + + describe 'show' do + describe 'permissions' do + let(:fetch) { get 'show' } + + it_behaves_like 'a controller action with require_admin' + end + + it 'contains check boxes for the working days' do + get 'show' + + expect(response).to be_successful + expect(response).to render_template 'admin/settings/working_days_settings/show' + end + end + + describe 'update' do + let(:working_days) { [*'1'..'7'] } + let(:non_working_days) { {} } + let(:params) do + { settings: { working_days:, non_working_days: } } + end + + subject { patch 'update', params: } + + it 'succeeds' do + subject + + expect(response).to redirect_to action: 'show' + expect(flash[:notice]).to eq I18n.t(:notice_successful_update) + end + + context 'with non_working_days' do + let(:non_working_days) do + { '0' => { 'name' => 'Christmas Eve', 'date' => '2022-12-24' } } + end + + it 'succeeds' do + subject + + expect(response).to redirect_to action: 'show' + expect(flash[:notice]).to eq I18n.t(:notice_successful_update) + end + + it 'creates the non_working_days' do + expect { subject }.to change(NonWorkingDay, :count).by(1) + expect(NonWorkingDay.first).to have_attributes(name: 'Christmas Eve', date: Date.parse('2022-12-24')) + end + end + + context 'when fails with a duplicate entry' do + let(:nwd_to_delete) { create(:non_working_day, name: 'NWD to delete') } + let(:non_working_days) do + { + '0' => { 'name' => 'Christmas Eve', 'date' => '2022-12-24' }, + '1' => { 'name' => 'Christmas Eve2', 'date' => '2022-12-24' }, + '2' => { 'id' => nwd_to_delete.id, '_destroy' => true } + } + end + + it 'displays the error message' do + subject + + expect(response).to render_template :show + expect(flash[:error]).to eq 'A non-working day already exists for 2022-12-24.' + end + + it 'sets the @modified_non_working_days variable' do + subject + expect(assigns(:modified_non_working_days)).to contain_exactly( + have_attributes(name: 'Christmas Eve', date: Date.parse('2022-12-24')), + have_attributes(name: 'Christmas Eve2', date: Date.parse('2022-12-24')), + have_attributes(nwd_to_delete.slice(:id, :name, :date)) + ) + end + + it 'does not destroys other records' do + subject + expect { nwd_to_delete.reload }.not_to raise_error + end + end + end +end diff --git a/spec/services/settings/shared/shared_call_examples.rb b/spec/services/settings/shared/shared_call_examples.rb new file mode 100644 index 00000000000..8fd56250104 --- /dev/null +++ b/spec/services/settings/shared/shared_call_examples.rb @@ -0,0 +1,56 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 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.shared_examples 'successful call' do + it 'is successful' do + expect(subject) + .to be_success + end + + it 'sets the setting value' do + subject + + expect(Setting) + .to have_received(:[]=) + .with(setting_name, new_setting_value) + end +end + +RSpec.shared_examples 'unsuccessful call' do + it 'is not successful' do + expect(subject) + .not_to be_success + end + + it 'does not set the setting value' do + subject + + expect(Setting) + .not_to have_received(:[]=) + end +end diff --git a/spec/services/settings/update_service_spec.rb b/spec/services/settings/update_service_spec.rb index fa907703c7c..f357e626897 100644 --- a/spec/services/settings/update_service_spec.rb +++ b/spec/services/settings/update_service_spec.rb @@ -25,13 +25,13 @@ # See COPYRIGHT and LICENSE files for more details. require 'spec_helper' +require_relative 'shared/shared_call_examples' describe Settings::UpdateService do let(:instance) do - described_class.new(user:, contract_options:) + described_class.new(user:) end let(:user) { build_stubbed(:user) } - let(:contract_options) { {} } let(:contract) do instance_double(Settings::UpdateContract, validate: contract_success, @@ -76,81 +76,27 @@ describe Settings::UpdateService do end describe '#call' do - shared_examples_for 'successful call' do - it 'is successful' do - expect(instance.call(params)) - .to be_success - end - - it 'sets the setting value' do - instance.call(params) - - expect(Setting) - .to have_received(:[]=) - .with(setting_name, new_setting_value) - end - - it 'calls the on_change handler' do - instance.call(params) - - expect(definition_on_change) - .to have_received(:call).with(previous_setting_value) - end - end - - shared_examples_for 'unsuccessful call' do - it 'is not successful' do - expect(instance.call(params)) - .not_to be_success - end - - it 'does not set the setting value' do - instance.call(params) - - expect(Setting) - .not_to have_received(:[]=) - end - - it 'does not call the on_change handler' do - instance.call(params) - - expect(definition_on_change) - .not_to have_received(:call) - end - end + subject { instance.call(params) } include_examples 'successful call' + it 'calls the on_change handler' do + subject + + expect(definition_on_change) + .to have_received(:call).with(previous_setting_value) + end + context 'when the contract is not successfully validated' do let(:contract_success) { false } include_examples 'unsuccessful call' - end - context 'with a provided params_contract' do - let(:contract_options) { { params_contract: ParamsContract } } - let(:params_contract) do - instance_double(ParamsContract, - valid?: params_contract_success, - errors: instance_double(ActiveModel::Error)) - end + it 'does not call the on_change handler' do + subject - before do - allow(ParamsContract) - .to receive(:new) - .and_return(params_contract) - end - - context 'with a provided params_contract that is successfully validated' do - let(:params_contract_success) { true } - - include_examples 'successful call' - end - - context 'with a provided params_contract that fails validation' do - let(:params_contract_success) { false } - - include_examples 'unsuccessful call' + expect(definition_on_change) + .not_to have_received(:call) end end end diff --git a/spec/services/settings/working_days_update_service_spec.rb b/spec/services/settings/working_days_update_service_spec.rb new file mode 100644 index 00000000000..ba69682fb52 --- /dev/null +++ b/spec/services/settings/working_days_update_service_spec.rb @@ -0,0 +1,169 @@ +# OpenProject is an open source project management software. +# Copyright (C) 2010-2022 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' +require_relative 'shared/shared_call_examples' + +describe Settings::WorkingDaysUpdateService do + let(:instance) do + described_class.new(user:) + end + let(:user) { build_stubbed(:user) } + let(:contract) do + instance_double(Settings::UpdateContract, + validate: contract_success, + errors: instance_double(ActiveModel::Error)) + end + let(:contract_success) { true } + let(:params_contract) do + instance_double(Settings::WorkingDaysParamsContract, + valid?: params_contract_success, + errors: instance_double(ActiveModel::Error)) + end + let(:params_contract_success) { true } + let(:setting_name) { :a_setting_name } + let(:new_setting_value) { 'a_new_setting_value' } + let(:previous_setting_value) { 'the_previous_setting_value' } + let(:setting_params) { { setting_name => new_setting_value } } + let(:non_working_days_params) { {} } + let(:params) { setting_params.merge(non_working_days: non_working_days_params) } + + before do + # stub a setting definition + allow(Setting) + .to receive(:[]) + .and_call_original + allow(Setting) + .to receive(:[]).with(setting_name) + .and_return(previous_setting_value) + allow(Setting) + .to receive(:[]=) + + # stub contract + allow(Settings::UpdateContract) + .to receive(:new) + .and_return(contract) + allow(Settings::WorkingDaysParamsContract) + .to receive(:new) + .and_return(params_contract) + end + + describe '#call' do + subject { instance.call(params) } + + shared_examples 'unsuccessful working days settings call' do + include_examples 'unsuccessful call' + + it 'does not persists the non working days' do + expect { subject }.not_to change(NonWorkingDay, :count) + end + end + + include_examples 'successful call' + + context 'when non working days are present' do + let!(:existing_nwd) { create(:non_working_day, name: 'Existing NWD') } + let!(:nwd_to_delete) { create(:non_working_day, name: 'NWD to delete') } + let(:non_working_days_params) do + [ + { 'name' => 'Christmas Eve', 'date' => '2022-12-24' }, + { 'name' => 'NYE', 'date' => '2022-12-31' }, + { 'id' => existing_nwd.id }, + { 'id' => nwd_to_delete.id, '_destroy' => true } + ] + end + + include_examples 'successful call' + + it 'persists (create/delete) the non working days' do + expect { subject }.to change(NonWorkingDay, :count).by(1) + + expect { nwd_to_delete.reload }.to raise_error(ActiveRecord::RecordNotFound) + + expect(NonWorkingDay.all).to contain_exactly( + have_attributes(name: 'Christmas Eve', date: Date.parse('2022-12-24')), + have_attributes(name: 'NYE', date: Date.parse('2022-12-31')), + have_attributes(existing_nwd.slice(:id, :name, :date)) + ) + end + + context 'when there are duplicates' do + context 'with both within the params' do + let(:non_working_days_params) do + [ + { 'name' => 'Christmas Eve', 'date' => '2022-12-24' }, + { 'name' => 'Christmas Eve', 'date' => '2022-12-24' } + ] + end + + include_examples 'unsuccessful working days settings call' + end + + context 'with one saved in the database' do + let(:non_working_days_params) do + [existing_nwd.slice(:name, :date)] + end + + include_examples 'unsuccessful working days settings call' + + context 'when deleting and re-creating the duplicate non-working day' do + let(:non_working_days_params) do + [ + existing_nwd.slice(:id, :name, :date).merge('_destroy' => true), + existing_nwd.slice(:name, :date) + ] + end + + include_examples 'successful call' + end + end + end + end + + context 'when the params contract is not successfully validated' do + let(:params_contract_success) { false } + + include_examples 'unsuccessful working days settings call' + end + + context 'when the contract is not successfully validated' do + let(:contract_success) { false } + + include_examples 'unsuccessful working days settings call' + + context 'when non working days are present' do + let(:non_working_days_params) do + [ + { 'name' => 'Christmas Eve', 'date' => '2022-12-24' }, + { 'name' => 'NYE', 'date' => '2022-12-31' } + ] + end + + include_examples 'unsuccessful working days settings call' + end + end + end +end