[#45339] Modify the backend to support saving the non working days

https://community.openproject.org/work_packages/45339

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

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

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

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
 

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

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:

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

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

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

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