[73372] Wrong icon used when changing non working days (#23292)

* Create a new dialog component for non-working days

* Change the non-working days component

* Add feature spec

* Fix reload after canceling the action

* preserve submitted form data for confirmation, and simplify cancel handling

* Change header text

* Remove the typescript unnecessary codes and listening to a form submit and call update on confirm changes

app/components/admin/settings/working_days/confirm_dialog_component.html.erb

@@ -0,0 +1,39 @@
+<%=
+  render(
+    Primer::OpenProject::DangerDialog.new(
+      id: DIALOG_ID,
+      title: t("working_days.dialog.title"),
+      confirm_button_text: t("working_days.dialog.confirm_button"),
+      form_arguments:,
+      size: :medium_portrait
+    )
+  ) do |dialog|
+    dialog.with_confirmation_message do |message|
+      message.with_heading(tag: :h2) { t("working_days.dialog.heading") }
+      message.with_description_content(t("working_days.dialog.description"))
+    end
+
+    dialog.with_additional_details do
+      concat hidden_settings_fields
+      concat(
+        tag.div do
+          if removed_non_working_days.any?
+            concat tag.p(t("working_days.dialog.removed_title"))
+            concat(
+              render(
+                Primer::BaseComponent.new(
+                  tag: :ul,
+                  mb: 3
+                )
+              ) do
+                safe_join(removed_non_working_days.map { |non_working_day| tag.li(non_working_day) })
+              end
+            )
+          end
+
+          concat tag.p(t("working_days.dialog.warning"))
+        end
+      )
+    end
+  end
+%>

app/components/admin/settings/working_days/confirm_dialog_component.rb

@@ -0,0 +1,78 @@
+# 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 Admin
+  module Settings
+    module WorkingDays
+      class ConfirmDialogComponent < ApplicationComponent
+        include OpTurbo::Streamable
+
+        DIALOG_ID = "working-days-change-dialog"
+
+        attr_reader :form_values, :removed_non_working_days
+
+        def initialize(form_values: {}, removed_non_working_days: [])
+          super
+          @form_values = form_values
+          @removed_non_working_days = removed_non_working_days
+        end
+
+        private
+
+        def form_arguments
+          {
+            action: helpers.admin_settings_working_days_and_hours_path,
+            method: :patch,
+            data: { turbo: false }
+          }
+        end
+
+        def hidden_settings_fields
+          hidden_field_tags_for("settings", form_values)
+        end
+
+        def hidden_field_tags_for(name, value)
+          case value
+          when Hash
+            safe_join(
+              value.flat_map do |key, nested_value|
+                hidden_field_tags_for("#{name}[#{key}]", nested_value)
+              end
+            )
+          when Array
+            safe_join(value.map { |array_value| hidden_field_tag("#{name}[]", array_value) })
+          else
+            hidden_field_tag(name, value)
+          end
+        end
+      end
+    end
+  end
+end

app/controllers/admin/settings/working_days_and_hours_settings_controller.rb

@@ -30,8 +30,25 @@
 
 module Admin::Settings
   class WorkingDaysAndHoursSettingsController < ::Admin::SettingsController
+    include OpTurbo::ComponentStream
+
     menu_item :working_days_and_hours
 
+    def confirm_changes
+      return update unless working_days_changed? || non_working_days_changed?
+
+      removed_days = non_working_days_params
+        .select { |nwd| nwd["_destroy"].present? }
+        .filter_map { |nwd| removed_non_working_day_date(nwd) }
+
+      component = Admin::Settings::WorkingDays::ConfirmDialogComponent.new(
+        form_values: params.expect(settings: {}).to_h,
+        removed_non_working_days: removed_days
+      )
+
+      respond_with_dialog(component)
+    end
+
     def failure_callback(call)
       @modified_non_working_days = modified_non_working_days_for(call.result)
       flash[:error] = call.message || I18n.t(:notice_internal_server_error)
@@ -53,15 +70,31 @@ module Admin::Settings
 
     private
 
+    def working_days_changed?
+      working_days_params(params.expect(settings: {})) != Setting.working_days.map(&:to_i)
+    end
+
+    def non_working_days_changed?
+      non_working_days_params.any?
+    end
+
     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_attributes] || {}
+      non_working_days = params.expect(settings: {})[:non_working_days_attributes] || {}
       non_working_days.to_h.values
     end
 
+    def removed_non_working_day_date(non_working_day_params)
+      date = NonWorkingDay.find_by(id: non_working_day_params["id"])&.date || non_working_day_params["date"]
+
+      I18n.l(date.to_date, format: :long)
+    rescue Date::Error, NoMethodError
+      nil
+    end
+
     def modified_non_working_days_for(result)
       return if result.nil?
 

app/views/admin/settings/working_days_and_hours_settings/show.html.erb

@@ -41,9 +41,9 @@ See COPYRIGHT and LICENSE files for more details.
 %>
 
 <%= styled_form_tag(
-      admin_settings_working_days_and_hours_path,
-      method: :patch,
-      class: "op-working-days-admin-settings"
+      confirm_changes_admin_settings_working_days_and_hours_path,
+      class: "op-working-days-admin-settings",
+      data: { turbo_stream: true }
     ) do %>
 
   <section class="form--section">

config/locales/en.yml

@@ -6201,6 +6201,17 @@ en:
     warning: >
       Changing which days of the week are considered working days or non-working days
       can affect the start and finish days of all work packages and life cycles in all projects in this instance.
+    dialog:
+      title: "Change working days"
+      description: >
+        Changing which days of the week are considered working days or non-working days
+        can affect the start and finish days of all work packages and life cycles in all projects in this instance.
+      removed_title: "You will remove the following days from the non-working days list:"
+      warning: >
+        The changes might take some time to take effect. You will be notified when all relevant work
+        packages and project life cycles have been updated.
+      heading: "Change the working days?"
+      confirm_button: "Save and reschedule"
     journal_note:
       changed: _**Working days** changed (%{changes})._
       days:

config/locales/js-en.yml

@@ -228,15 +228,6 @@ en:
           new_date: "(new)"
         add_non_working_day: "Non-working day"
         already_added_error: "A non-working day for this date exists already. There can only be one non-working day created for each unique date."
-        change_button: "Save and reschedule"
-        change_title: "Change working days"
-        removed_title: "You will remove the following days from the non-working days list:"
-        change_description: "Changing which days of the week are considered working days or non-working days can affect the start and finish days of all work packages and life cycles in all projects in this instance."
-        warning: >
-          The changes might take some time to take effect. You will be notified when all relevant work packages and project life cycles have been updated.
-
-
-          Are you sure you want to continue?
 
       work_packages_settings:
         warning_progress_calculation_mode_change_from_status_to_field_html: >-

config/routes.rb

@@ -750,7 +750,9 @@ Rails.application.routes.draw do
           get :new_link
         end
       end
-      resource :working_days_and_hours, controller: "/admin/settings/working_days_and_hours_settings", only: %i[show update]
+      resource :working_days_and_hours, controller: "/admin/settings/working_days_and_hours_settings", only: %i[show update] do
+        post :confirm_changes
+      end
       resource :users, controller: "/admin/settings/users_settings", only: %i[show update]
       resource :date_format, controller: "/admin/settings/date_format_settings", only: %i[show update]
       resource :icalendar, controller: "/admin/settings/icalendar_settings", only: %i[show update]

frontend/src/app/shared/components/op-non-working-days-list/op-non-working-days-list.component.ts

@@ -1,21 +1,14 @@
-import { AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, ElementRef, HostBinding, Injector, Input, OnInit, ViewChild, ViewEncapsulation, inject } from '@angular/core';
-import { BannersService } from 'core-app/core/enterprise/banners.service';
+import { ChangeDetectionStrategy, ChangeDetectorRef, Component, ElementRef, HostBinding, Input, OnInit, ViewChild, ViewEncapsulation, inject } from '@angular/core';
 import { I18nService } from 'core-app/core/i18n/i18n.service';
-import { OpModalService } from '../modal/modal.service';
-import { PathHelperService } from 'core-app/core/path-helper/path-helper.service';
 import { populateInputsFromDataset } from 'core-app/shared/components/dataset-inputs';
 import { FullCalendarComponent } from '@fullcalendar/angular';
 import { CalendarOptions, EventInput, EventSourceFuncArg } from '@fullcalendar/core';
 import listPlugin from '@fullcalendar/list';
-import { ApiV3Service } from 'core-app/core/apiv3/api-v3.service';
 import { DayResourceService } from 'core-app/core/state/days/day.service';
 import { IDay } from 'core-app/core/state/days/day.model';
 import { CalendarViewEvent } from 'core-app/features/calendar/op-work-packages-calendar.service';
 import { opIconElement } from 'core-app/shared/helpers/op-icon-builder';
-import { ConfirmDialogService } from 'core-app/shared/components/modals/confirm-dialog/confirm-dialog.service';
-import { ConfirmDialogOptions } from '../modals/confirm-dialog/confirm-dialog.modal';
 import { ToastService } from 'core-app/shared/components/toaster/toast.service';
-import moment from 'moment-timezone';
 import allLocales from '@fullcalendar/core/locales-all';
 
 
@@ -34,16 +27,10 @@ export interface INonWorkingDay {
   templateUrl: './op-non-working-days-list.component.html',
   standalone: false,
 })
-export class OpNonWorkingDaysListComponent implements OnInit, AfterViewInit {
+export class OpNonWorkingDaysListComponent implements OnInit {
   readonly elementRef = inject<ElementRef<HTMLElement>>(ElementRef);
   protected I18n = inject(I18nService);
-  readonly bannersService = inject(BannersService);
-  readonly opModalService = inject(OpModalService);
-  readonly injector = inject(Injector);
-  readonly pathHelper = inject(PathHelperService);
-  readonly apiV3Service = inject(ApiV3Service);
   readonly dayService = inject(DayResourceService);
-  readonly confirmDialogService = inject(ConfirmDialogService);
   readonly toast = inject(ToastService);
   readonly cdRef = inject(ChangeDetectorRef);
 
@@ -59,22 +46,13 @@ export class OpNonWorkingDaysListComponent implements OnInit, AfterViewInit {
     already_added_error: this.I18n.t('js.admin.working_days.already_added_error'),
     new_date: this.I18n.t('js.admin.working_days.calendar.new_date'),
     add_non_working_day: this.I18n.t('js.admin.working_days.add_non_working_day'),
-    change_description: this.I18n.t('js.admin.working_days.change_description'),
-    warning: this.I18n.t('js.admin.working_days.warning'),
-    change_button: this.I18n.t('js.admin.working_days.change_button'),
-    change_title: this.I18n.t('js.admin.working_days.change_title'),
-    removed_title: this.I18n.t('js.admin.working_days.removed_title'),
     non_working_day_name: this.I18n.t('js.modals.label_name'),
     add: this.I18n.t('js.button_add'),
   };
 
-  form_submitted = false;
-
   originalNonWorkingDays:INonWorkingDay[] = [];
   nonWorkingDays:INonWorkingDay[] = [];
 
-  originalWorkingDays:string[] = [];
-
   datepickerOpened = false;
 
   selectedNonWorkingDayName = '';
@@ -100,15 +78,10 @@ export class OpNonWorkingDaysListComponent implements OnInit, AfterViewInit {
       anchor.classList.add('fc-list-day-side-text', 'op-non-working-days-list--delete-icon');
       anchor.appendChild(opIconElement('icon', 'icon-delete'));
 
-      anchor.addEventListener('click', () => {
-        // Create 4 hidden inputs(id, name, date, _destroy) for the deleted NWD
-        this.nonWorkingDays = this.nonWorkingDays.map((item) => {
-          if (item.date === event.id) {
-            return { ...item, _destroy: true };
-          }
+      anchor.addEventListener('click', (clickEvent:Event) => {
+        clickEvent.preventDefault();
 
-          return item;
-        });
+        this.markNonWorkingDayForRemoval(event.id);
         event.remove();
         this.cdRef.detectChanges();
       });
@@ -121,36 +94,15 @@ export class OpNonWorkingDaysListComponent implements OnInit, AfterViewInit {
 
   constructor() {
     populateInputsFromDataset(this);
-    this.listenToFormSubmit();
   }
 
-  private listenToFormSubmit() {
-    const form = this.elementRef.nativeElement.closest('form')!;
-    form.addEventListener('submit', (evt:Event) => {
-      if (!this.form_submitted
-        && (this.nonWorkingDaysModified() || this.workingDaysModified())) {
-        this.form_submitted = true;
-        const target = evt.target as HTMLFormElement;
-        const options:ConfirmDialogOptions = {
-          text: {
-            text: this.text.change_description,
-            title: this.text.change_title,
-            button_continue: this.text.change_button,
-          },
-          dangerHighlighting: true,
-          divideContent: true,
-          refreshOnCancel: true,
-          showListData: this.removedNonWorkingDays.length > 0,
-          warningText: this.text.warning,
-          passedData: this.removedNonWorkingDays,
-          listTitle: this.text.removed_title,
-        };
-        evt.preventDefault();
-        void this.confirmDialogService.confirm(options).then(() => {
-          this.form_submitted = false;
-          target.submit();
-        });
+  private markNonWorkingDayForRemoval(date:string):void {
+    this.nonWorkingDays = this.nonWorkingDays.map((item) => {
+      if (item.date === date) {
+        return { ...item, _destroy: true };
       }
+
+      return item;
     });
   }
 
@@ -163,23 +115,6 @@ export class OpNonWorkingDaysListComponent implements OnInit, AfterViewInit {
     this.cdRef.detectChanges();
   }
 
-  ngAfterViewInit():void {
-    const form = this.elementRef.nativeElement.closest('form')!;
-    const workingDayCheckboxes = Array.from(form.querySelectorAll('input[name="settings[working_days][]"]'));
-    workingDayCheckboxes.forEach((checkbox:HTMLInputElement) => {
-      if (checkbox.checked) {
-        this.originalWorkingDays.push(checkbox.value);
-      }
-    });
-  }
-
-  public get removedNonWorkingDays():string[] {
-    return this
-      .nonWorkingDays
-      .filter((el) => el._destroy)
-      .map((el) => moment(el.date).format('MMMM DD, YYYY'));
-  }
-
   // Initializes nonWorkingDays from the API
   public calendarEventsFunction(
     fetchInfo:EventSourceFuncArg,
@@ -238,28 +173,4 @@ export class OpNonWorkingDaysListComponent implements OnInit, AfterViewInit {
     api.addEvent({ ...day, id: date });
   }
 
-  private get workingDays():string[] {
-    const workingDays:string[] = [];
-
-    const form = this.elementRef.nativeElement.closest('form')!;
-    const workingDayCheckboxes = Array.from(form.querySelectorAll('input[name="settings[working_days][]"]'));
-    workingDayCheckboxes.forEach((checkbox:HTMLInputElement) => {
-      if (checkbox.checked) {
-        workingDays.push(checkbox.value);
-      }
-    });
-
-    return workingDays;
-  }
-
-  private nonWorkingDaysModified():boolean {
-    return this.removedNonWorkingDays.length > 0
-      || this.modifiedNonWorkingDays.length > 0
-      || this.nonWorkingDays.length > this.originalNonWorkingDays.length;
-  }
-
-  private workingDaysModified():boolean {
-    return _.difference(this.workingDays, this.originalWorkingDays).length > 0
-      || _.difference(this.originalWorkingDays, this.workingDays).length > 0;
-  }
 }

spec/components/admin/settings/working_days/confirm_dialog_component_spec.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Admin::Settings::WorkingDays::ConfirmDialogComponent, type: :component do
+  let(:form_values) do
+    {
+      "working_days" => ["", "1", "2", "3"],
+      "hours_per_day" => "8",
+      "non_working_days_attributes" => {
+        "12" => {
+          "id" => "12",
+          "_destroy" => "true"
+        },
+        "2026-06-10" => {
+          "date" => "2026-06-10",
+          "name" => "My holiday"
+        }
+      }
+    }
+  end
+
+  it "renders the danger dialog with no button icon and keeps the confirmation texts" do
+    render_inline(described_class.new(removed_non_working_days: ["June 10, 2026"]))
+
+    expect(page).to have_css(".DangerDialog")
+    expect(page).to have_css(".Button--danger", text: "Save and reschedule")
+    expect(page).to have_no_css(".Button--danger svg")
+
+    expect(page).to have_text("Change working days")
+    expect(page).to have_css("h2", text: "Change the working days?")
+    expect(page).to have_text("You will remove the following days from the non-working days list:")
+    expect(page).to have_text("June 10, 2026")
+    expect(page).to have_css("ul", text: "June 10, 2026")
+  end
+
+  it "submits the captured working days settings through the dialog form" do
+    render_inline(described_class.new(form_values:))
+
+    expect(page).to have_css('input[type="hidden"][name="_method"][value="patch"]', visible: :hidden)
+    expect(page).to have_css('input[type="hidden"][name="settings[hours_per_day]"][value="8"]', visible: :hidden)
+    expect(page).to have_css('input[type="hidden"][name="settings[working_days][]"][value="1"]', visible: :hidden)
+    expect(page).to have_css('input[type="hidden"][name="settings[working_days][]"][value="2"]', visible: :hidden)
+    expect(page).to have_css('input[type="hidden"][name="settings[working_days][]"][value="3"]', visible: :hidden)
+  end
+
+  it "submits captured non-working day changes through the dialog form" do
+    render_inline(described_class.new(form_values:))
+
+    expect(page).to have_css(
+      'input[type="hidden"][name="settings[non_working_days_attributes][12][id]"][value="12"]',
+      visible: :hidden
+    )
+    expect(page).to have_css(
+      'input[type="hidden"][name="settings[non_working_days_attributes][12][_destroy]"][value="true"]',
+      visible: :hidden
+    )
+    expect(page).to have_css(
+      'input[type="hidden"][name="settings[non_working_days_attributes][2026-06-10][date]"][value="2026-06-10"]',
+      visible: :hidden
+    )
+    expect(page).to have_css(
+      'input[type="hidden"][name="settings[non_working_days_attributes][2026-06-10][name]"][value="My holiday"]',
+      visible: :hidden
+    )
+  end
+end

spec/features/admin/working_days_spec.rb

@@ -70,7 +70,6 @@ RSpec.describe "Working Days", :js do
     follower              |          XXX   | automatic       | follows earliest_work_package, follows second_work_package
   TABLE
 
-  let(:dialog) { Components::ConfirmationDialog.new }
   let(:datepicker) { Components::DatepickerModal.new }
   let(:project_activity_page) { Pages::Projects::Activity.new(project) }
 
@@ -106,7 +105,7 @@ RSpec.describe "Working Days", :js do
       click_on "Apply changes"
 
       perform_enqueued_jobs do
-        dialog.cancel
+        within_dialog("Change working days") { click_button "Cancel" }
       end
 
       expect(page).to have_no_css(".op-toast.-success")
@@ -130,7 +129,7 @@ RSpec.describe "Working Days", :js do
       click_on "Apply changes"
 
       perform_enqueued_jobs do
-        dialog.confirm
+        within_dialog("Change working days") { click_button "Save and reschedule" }
       end
 
       expect_flash(message: "Successful update.")
@@ -171,7 +170,7 @@ RSpec.describe "Working Days", :js do
       click_on "Apply changes"
 
       perform_enqueued_jobs do
-        dialog.confirm
+        within_dialog("Change working days") { click_button "Save and reschedule" }
       end
 
       expect_flash(type: :error, message: "At least one day of the week must be defined as a working day.")
@@ -202,7 +201,7 @@ RSpec.describe "Working Days", :js do
       click_on "Apply changes"
 
       # Not executing the background jobs
-      dialog.confirm
+      within_dialog("Change working days") { click_button "Save and reschedule" }
 
       expect_flash(type: :error,
                    message: "The previous changes to the working days configuration have not been applied yet.")
@@ -223,7 +222,7 @@ RSpec.describe "Working Days", :js do
       click_on "Apply changes"
 
       perform_enqueued_jobs do
-        dialog.confirm
+        within_dialog("Change working days") { click_button "Save and reschedule" }
       end
 
       expect_flash(message: "Successful update.")
@@ -351,7 +350,7 @@ RSpec.describe "Working Days", :js do
 
       click_on "Apply changes"
 
-      dialog.confirm
+      within_dialog("Change working days") { click_button "Save and reschedule" }
 
       # Remove the first date
       expect(page).to have_no_css("tr", text: non_working_days.first.date.strftime("%B %-d, %Y"))
@@ -373,7 +372,7 @@ RSpec.describe "Working Days", :js do
 
       click_on "Apply changes"
 
-      dialog.confirm
+      within_dialog("Change working days") { click_button "Save and reschedule" }
 
       # Keep the second date hidden
       expect(page).to have_no_css("tr", text: non_working_days.second.date.strftime("%B %-d, %Y"))