From 031c3ce1ccb450a32cd0d6e07fb712460ccb5fbf Mon Sep 17 00:00:00 2001 From: Behrokh Satarnejad <62008897+bsatarnejad@users.noreply.github.com> Date: Fri, 29 May 2026 09:08:48 +0200 Subject: [PATCH] [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 --- .../confirm_dialog_component.html.erb | 39 ++++++ .../working_days/confirm_dialog_component.rb | 78 ++++++++++++ ...king_days_and_hours_settings_controller.rb | 35 +++++- .../show.html.erb | 6 +- config/locales/en.yml | 11 ++ config/locales/js-en.yml | 9 -- config/routes.rb | 4 +- .../op-non-working-days-list.component.ts | 111 ++---------------- .../confirm_dialog_component_spec.rb | 67 +++++++++++ spec/features/admin/working_days_spec.rb | 15 ++- 10 files changed, 253 insertions(+), 122 deletions(-) create mode 100644 app/components/admin/settings/working_days/confirm_dialog_component.html.erb create mode 100644 app/components/admin/settings/working_days/confirm_dialog_component.rb create mode 100644 spec/components/admin/settings/working_days/confirm_dialog_component_spec.rb diff --git a/app/components/admin/settings/working_days/confirm_dialog_component.html.erb b/app/components/admin/settings/working_days/confirm_dialog_component.html.erb new file mode 100644 index 00000000000..67df581f1d8 --- /dev/null +++ b/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 +%> diff --git a/app/components/admin/settings/working_days/confirm_dialog_component.rb b/app/components/admin/settings/working_days/confirm_dialog_component.rb new file mode 100644 index 00000000000..ff380bff316 --- /dev/null +++ b/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 diff --git a/app/controllers/admin/settings/working_days_and_hours_settings_controller.rb b/app/controllers/admin/settings/working_days_and_hours_settings_controller.rb index 4a2d5a94af8..ed64a3b35bd 100644 --- a/app/controllers/admin/settings/working_days_and_hours_settings_controller.rb +++ b/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? diff --git a/app/views/admin/settings/working_days_and_hours_settings/show.html.erb b/app/views/admin/settings/working_days_and_hours_settings/show.html.erb index ddebe53fef9..31f23fcf725 100644 --- a/app/views/admin/settings/working_days_and_hours_settings/show.html.erb +++ b/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 %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 42fcb850b71..fd4937afb45 100644 --- a/config/locales/en.yml +++ b/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: diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index 1b1b683d1cd..9c4cf449afc 100644 --- a/config/locales/js-en.yml +++ b/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: >- diff --git a/config/routes.rb b/config/routes.rb index 9336c99219e..1a5d6bd4dd6 100644 --- a/config/routes.rb +++ b/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] diff --git a/frontend/src/app/shared/components/op-non-working-days-list/op-non-working-days-list.component.ts b/frontend/src/app/shared/components/op-non-working-days-list/op-non-working-days-list.component.ts index c545e4107d5..b3a711704f8 100644 --- a/frontend/src/app/shared/components/op-non-working-days-list/op-non-working-days-list.component.ts +++ b/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); 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; - } } diff --git a/spec/components/admin/settings/working_days/confirm_dialog_component_spec.rb b/spec/components/admin/settings/working_days/confirm_dialog_component_spec.rb new file mode 100644 index 00000000000..f73dc4d7fcc --- /dev/null +++ b/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 diff --git a/spec/features/admin/working_days_spec.rb b/spec/features/admin/working_days_spec.rb index 6604d14642c..003c1ba093b 100644 --- a/spec/features/admin/working_days_spec.rb +++ b/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"))