From 12b3edceec32953587e791efe1fd80b74a1deaa5 Mon Sep 17 00:00:00 2001 From: Dombi Attila <83396+dombesz@users.noreply.github.com> Date: Wed, 10 Apr 2024 16:49:16 +0300 Subject: [PATCH 01/18] Use morphing on turbo frame re-rendering. --- frontend/package-lock.json | 11 +++++++++++ frontend/package.json | 1 + frontend/src/turbo/setup.ts | 11 +++++++++++ 3 files changed, 23 insertions(+) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 2487a70517f8..99718e1656d1 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -84,6 +84,7 @@ "mime": "^2.5.2", "moment": "^2.29.4", "moment-timezone": "^0.5.43", + "morphdom": "^2.7.2", "mousetrap": "~1.6.3", "ng-dynamic-component": "^10.7.0", "ng2-charts": "^4.1.1", @@ -14923,6 +14924,11 @@ "node": "*" } }, + "node_modules/morphdom": { + "version": "2.7.2", + "resolved": "https://registry.npmjs.org/morphdom/-/morphdom-2.7.2.tgz", + "integrity": "sha512-Dqb/lHFyTi7SZpY0a5R4I/0Edo+iPMbaUexsHHsLAByyixCDiLHPHyVoKVmrpL0THcT7V9Cgev9y21TQYq6wQg==" + }, "node_modules/mousetrap": { "version": "1.6.5", "resolved": "https://registry.npmjs.org/mousetrap/-/mousetrap-1.6.5.tgz", @@ -31730,6 +31736,11 @@ "moment": "^2.29.4" } }, + "morphdom": { + "version": "2.7.2", + "resolved": "https://registry.npmjs.org/morphdom/-/morphdom-2.7.2.tgz", + "integrity": "sha512-Dqb/lHFyTi7SZpY0a5R4I/0Edo+iPMbaUexsHHsLAByyixCDiLHPHyVoKVmrpL0THcT7V9Cgev9y21TQYq6wQg==" + }, "mousetrap": { "version": "1.6.5", "resolved": "https://registry.npmjs.org/mousetrap/-/mousetrap-1.6.5.tgz", diff --git a/frontend/package.json b/frontend/package.json index e6310aa2c9f8..231c15ffe7a9 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -135,6 +135,7 @@ "mime": "^2.5.2", "moment": "^2.29.4", "moment-timezone": "^0.5.43", + "morphdom": "^2.7.2", "mousetrap": "~1.6.3", "ng-dynamic-component": "^10.7.0", "ng2-charts": "^4.1.1", diff --git a/frontend/src/turbo/setup.ts b/frontend/src/turbo/setup.ts index 214cd9140944..8c4d1085c66b 100644 --- a/frontend/src/turbo/setup.ts +++ b/frontend/src/turbo/setup.ts @@ -1,4 +1,5 @@ import * as Turbo from '@hotwired/turbo'; +import morphdom from 'morphdom'; import { ModalDialogElement } from '@openproject/primer-view-components/app/components/primer/alpha/modal_dialog'; // Disable default turbo-drive for now as we don't need it for now AND it breaks angular routing @@ -24,3 +25,13 @@ document.addEventListener('turbo:submit-end', (event:CustomEvent) => { dialog && dialog.close(true); } }); + +interface TurboBeforeFrameRenderEventDetail { + render:(currentElement:HTMLElement, newElement:HTMLElement) => void; +} + +document.addEventListener('turbo:before-frame-render', (event:CustomEvent) => { + event.detail.render = (currentElement:HTMLElement, newElement:HTMLElement) => { + morphdom(currentElement, newElement, { childrenOnly: true }); + }; +}); From d213274582941335d8f83e5e061bbca6ef6f5067 Mon Sep 17 00:00:00 2001 From: Dombi Attila <83396+dombesz@users.noreply.github.com> Date: Wed, 10 Apr 2024 15:28:26 +0300 Subject: [PATCH 02/18] Refresh remaining work calculations on the work estimate modal. --- .../progress/base_modal_component.rb | 4 +- .../modal_body_component.html.erb | 3 +- .../work_based/modal_body_component.html.erb | 3 +- app/forms/work_packages/progress_form.rb | 6 +- .../progress/preview-progress.controller.ts | 71 +++++++++++++++++++ 5 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts diff --git a/app/components/work_packages/progress/base_modal_component.rb b/app/components/work_packages/progress/base_modal_component.rb index ecc48c4bf59a..728aff84b1cf 100644 --- a/app/components/work_packages/progress/base_modal_component.rb +++ b/app/components/work_packages/progress/base_modal_component.rb @@ -36,7 +36,9 @@ class BaseModalComponent < ApplicationComponent FIELD_MAP = { "estimatedTime" => :estimated_hours, - "remainingTime" => :remaining_hours + "work_package[estimated_hours]" => :estimated_hours, + "remainingTime" => :remaining_hours, + "work_package[remaining_hours]" => :remaining_hours, }.freeze include ApplicationHelper diff --git a/app/components/work_packages/progress/status_based/modal_body_component.html.erb b/app/components/work_packages/progress/status_based/modal_body_component.html.erb index d969fc6829f7..e050b328c620 100644 --- a/app/components/work_packages/progress/status_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/status_based/modal_body_component.html.erb @@ -5,7 +5,8 @@ class: 'progress-form', id: 'progress-form', data: { 'application-target': "dynamic", - controller: "work-packages--progress--focus-field" }) do |f| %> + "work-packages--progress--preview-progress-target": "form", + controller: "work-packages--progress--focus-field work-packages--progress--preview-progress" }) do |f| %> <%= flex_layout do |modal_body| %> <% modal_body.with_row do |_fields| %> <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:)) %> diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb index a5eb07f619e0..27ea211e1628 100644 --- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/work_based/modal_body_component.html.erb @@ -5,7 +5,8 @@ class: "progress-form", id: "progress-form", data: { "application-target": "dynamic", - controller: "work-packages--progress--focus-field" }) do |f| %> + "work-packages--progress--preview-progress-target": "form", + controller: "work-packages--progress--focus-field work-packages--progress--preview-progress" }) do |f| %> <%= flex_layout do |modal_body| %> <% modal_body.with_row do |_fields| %> <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:)) %> diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb index 37e448fb1546..967ebf9dacfe 100644 --- a/app/forms/work_packages/progress_form.rb +++ b/app/forms/work_packages/progress_form.rb @@ -147,10 +147,10 @@ def format_to_smallest_fractional_part(number) end def default_field_options(name) + data = { "work-packages--progress--preview-progress-target": "progressInput" } if @focused_field == name - { data: { "work-packages--progress--focus-field-target": "fieldToFocus" } } - else - {} + data[:"work-packages--progress--focus-field-target"] = "fieldToFocus" end + { data: } end end diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts new file mode 100644 index 000000000000..c66c8ad4ea63 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts @@ -0,0 +1,71 @@ +/* + * -- copyright + * OpenProject is an open source project management software. + * Copyright (C) 2024 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. + * ++ + */ + +import { Controller } from '@hotwired/stimulus'; +import { debounce } from 'lodash'; + +export default class PreviewProgressController extends Controller { + static targets = [ + 'form', 'progressInput', + ]; + + declare readonly progressInputTargets:HTMLInputElement[]; + declare readonly formTarget:HTMLFormElement; + + private debouncedPreview:(event:Event) => void; + + connect() { + this.debouncedPreview = debounce((event:Event) => { void this.preview(event); }, 500); + this.progressInputTargets.forEach((target) => target.addEventListener('input', this.debouncedPreview)); + } + + disconnect() { + this.progressInputTargets.forEach((target) => target.removeEventListener('input', this.debouncedPreview)); + } + + async preview(event:Event) { + const field = event.target as HTMLInputElement; + const form = this.formTarget; + const formData = new FormData(form) as unknown as undefined; + const formParams = new URLSearchParams(formData); + const wpParams = [ + ['work_package[remaining_hours]', formParams.get('work_package[remaining_hours]') || ''], + ['work_package[estimated_hours]', formParams.get('work_package[estimated_hours]') || ''], + ['field', field.name ?? 'estimatedTime'], + ]; + + const editUrl = `${form.action}/edit?${new URLSearchParams(wpParams).toString()}`; + const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement; + + if (turboFrame) { + turboFrame.src = editUrl; + } + } +} From 3fd2fe16111e8c806ec5b96e140d9ac5378c03e9 Mon Sep 17 00:00:00 2001 From: Dombi Attila <83396+dombesz@users.noreply.github.com> Date: Wed, 10 Apr 2024 16:49:16 +0300 Subject: [PATCH 03/18] Preserve cursor position of the progress modal by using morphing on the frame re-rendering. --- .../progress/preview-progress.controller.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts index c66c8ad4ea63..3b25384af892 100644 --- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts @@ -30,6 +30,11 @@ import { Controller } from '@hotwired/stimulus'; import { debounce } from 'lodash'; +import morphdom from 'morphdom'; + +interface TurboBeforeFrameRenderEventDetail { + render:(currentElement:HTMLElement, newElement:HTMLElement) => void; +} export default class PreviewProgressController extends Controller { static targets = [ @@ -40,14 +45,31 @@ export default class PreviewProgressController extends Controller { declare readonly formTarget:HTMLFormElement; private debouncedPreview:(event:Event) => void; + private frameMorphRenderer:(event:CustomEvent) => void; connect() { this.debouncedPreview = debounce((event:Event) => { void this.preview(event); }, 500); + // TODO: Ideally morphing in this single controller should not be necessary. + // Turbo supports morphing, by adding the attribute. + // However, it has a bug, and it doesn't morphs when reloading the frame via javascript. + // See https://github.com/hotwired/turbo/issues/1161 . Once the issue is solved, we can remove + // this code and just use instead. + this.frameMorphRenderer = (event:CustomEvent) => { + event.detail.render = (currentElement:HTMLElement, newElement:HTMLElement) => { + morphdom(currentElement, newElement, { childrenOnly: true }); + }; + }; + this.progressInputTargets.forEach((target) => target.addEventListener('input', this.debouncedPreview)); + + const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement; + turboFrame.addEventListener('turbo:before-frame-render', this.frameMorphRenderer); } disconnect() { this.progressInputTargets.forEach((target) => target.removeEventListener('input', this.debouncedPreview)); + const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement; + turboFrame.removeEventListener('turbo:before-frame-render', this.frameMorphRenderer); } async preview(event:Event) { From 4a953fe872d5e0f071d1952a350d1c8a6bad36c9 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Thu, 11 Apr 2024 22:08:24 -0500 Subject: [PATCH 04/18] Ensure modals can be previewed in the context of create forms Given the requests for previews are going to the `edit` action, this relies on a path that includes a `:work_package_id` in the request's path. Since the work package in a create context is unpersisted, there isn't an id to be provided here. This commit mimicks the behavior in the Angular side of the application by setting the `:work_package_id` to "new" in create contexts. --- .../progress/preview-progress.controller.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts index 3b25384af892..4df05b9f6917 100644 --- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts @@ -83,11 +83,25 @@ export default class PreviewProgressController extends Controller { ['field', field.name ?? 'estimatedTime'], ]; - const editUrl = `${form.action}/edit?${new URLSearchParams(wpParams).toString()}`; + const wpPath = this.ensureValidPathname(form.action); + + const editUrl = `${wpPath}/edit?${new URLSearchParams(wpParams).toString()}`; const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement; if (turboFrame) { turboFrame.src = editUrl; } } + + // Ensures that on create forms, there is an "id" for the un-persisted + // work package when sending requests to the edit action for previews. + private ensureValidPathname(formAction:string):string { + const wpPath = new URL(formAction); + + if (wpPath.pathname === '/work_packages/progress') { + wpPath.pathname = '/work_packages/new/progress'; + } + + return wpPath.toString(); + } } From 7c6552b317a51aafb699c6c6c7faae07bae3f36e Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Mon, 15 Apr 2024 09:53:03 -0500 Subject: [PATCH 05/18] Add feature specs for cases known to be buggy with live updating --- .../work_packages/progress_modal_spec.rb | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/spec/features/work_packages/progress_modal_spec.rb b/spec/features/work_packages/progress_modal_spec.rb index 922106c0ff0b..620e993934be 100644 --- a/spec/features/work_packages/progress_modal_spec.rb +++ b/spec/features/work_packages/progress_modal_spec.rb @@ -441,4 +441,72 @@ def update_work_package_with(work_package, attributes) include_examples "renders a banner with a warning message" end end + + describe "Live-update edge cases" do + context "given work = 10h, remaining work = 4h, % complete = 60%" do + before { update_work_package_with(work_package, estimated_hours: 10.0, remaining_hours: 4.0) } + + specify "Case 1: When I unset work it unsets remaining work" do + work_package_table.visit_query(progress_query) + work_package_table.expect_work_package_listed(work_package) + + work_edit_field = ProgressEditField.new(work_package_row, :estimatedTime) + remaining_work_edit_field = ProgressEditField.new(work_package_row, :remainingTime) + + work_edit_field.activate! + page.driver.wait_for_network_idle # Wait for initial loading to be ready + + work_edit_field.set_value("") + page.driver.wait_for_network_idle # Wait for live-update to finish + pending "Currently sets remaining work to 4h instead of unsetting it" + remaining_work_edit_field.expect_modal_field_value("") + end + + specify "Case 2: when work is set to 12h, " \ + "remaining work is automatically set to 6h " \ + "and subsequently work is set to 14h, " \ + "remaining work updates to 8h" do + work_package_table.visit_query(progress_query) + work_package_table.expect_work_package_listed(work_package) + + work_edit_field = ProgressEditField.new(work_package_row, :estimatedTime) + remaining_work_edit_field = ProgressEditField.new(work_package_row, :remainingTime) + + work_edit_field.activate! + page.driver.wait_for_network_idle # Wait for initial loading to be ready + + work_edit_field.set_value("12") + page.driver.wait_for_network_idle # Wait for live-update to finish + remaining_work_edit_field.expect_modal_field_value("6") + + work_edit_field.set_value("14") + page.driver.wait_for_network_idle # Wait for live-update to finish + pending "Currently remaining work is not updated to 8h" + remaining_work_edit_field.expect_modal_field_value("8") + end + + specify "Case 3: when work is set to 2h, " \ + "remaining work is automatically set to 0h, " \ + "and work is subsequently set to 12h, " \ + "remaining work is updated to 6h" do + work_package_table.visit_query(progress_query) + work_package_table.expect_work_package_listed(work_package) + + work_edit_field = ProgressEditField.new(work_package_row, :estimatedTime) + remaining_work_edit_field = ProgressEditField.new(work_package_row, :remainingTime) + + work_edit_field.activate! + page.driver.wait_for_network_idle # Wait for initial loading to be ready + + work_edit_field.set_value("2") + page.driver.wait_for_network_idle # Wait for live-update to finish + remaining_work_edit_field.expect_modal_field_value("0") + + work_edit_field.set_value("12") + page.driver.wait_for_network_idle # Wait for live-update to finish + pending "Currently remains at 0h and is not updated to 6h" + remaining_work_edit_field.expect_modal_field_value("6") + end + end + end end From e086ecb8feb9ac739320b27a995735a8ce464a8a Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Tue, 16 Apr 2024 10:54:44 -0500 Subject: [PATCH 06/18] Flag changed fields for change detection on server side --- .../progress/base_modal_component.rb | 7 ++- .../modal_body_component.html.erb | 6 ++- .../status_based/modal_body_component.rb | 2 +- .../work_based/modal_body_component.html.erb | 6 ++- .../work_based/modal_body_component.rb | 2 +- .../work_packages/progress_controller.rb | 39 ++++++++++++--- app/forms/work_packages/progress_form.rb | 26 +++++++++- .../work_packages/set_attributes_service.rb | 11 +++-- .../progress/preview-progress.controller.ts | 8 ++- .../touched-field-marker.controller.ts | 49 +++++++++++++++++++ .../work_packages/progress_modal_spec.rb | 8 ++- 11 files changed, 138 insertions(+), 26 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts diff --git a/app/components/work_packages/progress/base_modal_component.rb b/app/components/work_packages/progress/base_modal_component.rb index 384085329307..303d02e8c614 100644 --- a/app/components/work_packages/progress/base_modal_component.rb +++ b/app/components/work_packages/progress/base_modal_component.rb @@ -39,6 +39,8 @@ class BaseModalComponent < ApplicationComponent "work_package[estimated_hours]" => :estimated_hours, "remainingTime" => :remaining_hours, "work_package[remaining_hours]" => :remaining_hours, + "work_package[status_id]" => :status_id, + "statusId" => :status_id }.freeze include ApplicationHelper @@ -46,13 +48,14 @@ class BaseModalComponent < ApplicationComponent include OpPrimer::ComponentHelpers include OpenProject::StaticRouting::UrlHelpers - attr_reader :work_package, :mode, :focused_field + attr_reader :work_package, :mode, :focused_field, :touched_field_map - def initialize(work_package, focused_field: nil) + def initialize(work_package, focused_field: nil, touched_field_map: {}) super() @work_package = work_package @focused_field = map_field(focused_field) + @touched_field_map = touched_field_map end def submit_path diff --git a/app/components/work_packages/progress/status_based/modal_body_component.html.erb b/app/components/work_packages/progress/status_based/modal_body_component.html.erb index 957f17bdfd35..c852ea2b3076 100644 --- a/app/components/work_packages/progress/status_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/status_based/modal_body_component.html.erb @@ -6,10 +6,12 @@ id: 'progress-form', data: { 'application-target': "dynamic", "work-packages--progress--preview-progress-target": "form", - controller: "work-packages--progress--focus-field work-packages--progress--preview-progress" }) do |f| %> + controller: "work-packages--progress--focus-field " \ + "work-packages--progress--preview-progress " \ + "work-packages--progress--touched-field-marker" }) do |f| %> <%= flex_layout do |modal_body| %> <% modal_body.with_row do |_fields| %> - <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:)) %> + <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:, touched_field_map:)) %> <% end %> <% modal_body.with_row(mt: 3) do |_tooltip| %> diff --git a/app/components/work_packages/progress/status_based/modal_body_component.rb b/app/components/work_packages/progress/status_based/modal_body_component.rb index 83174e979237..eaea1cb086d0 100644 --- a/app/components/work_packages/progress/status_based/modal_body_component.rb +++ b/app/components/work_packages/progress/status_based/modal_body_component.rb @@ -32,7 +32,7 @@ module WorkPackages module Progress module StatusBased class ModalBodyComponent < BaseModalComponent # rubocop:disable OpenProject/AddPreviewForViewComponent - def initialize(work_package, focused_field: nil) + def initialize(work_package, focused_field: nil, touched_field_map: {}) super @mode = :status_based diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb index 27ea211e1628..7b50e69d71d1 100644 --- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/work_based/modal_body_component.html.erb @@ -6,10 +6,12 @@ id: "progress-form", data: { "application-target": "dynamic", "work-packages--progress--preview-progress-target": "form", - controller: "work-packages--progress--focus-field work-packages--progress--preview-progress" }) do |f| %> + controller: "work-packages--progress--focus-field " \ + "work-packages--progress--preview-progress " \ + "work-packages--progress--touched-field-marker" }) do |f| %> <%= flex_layout do |modal_body| %> <% modal_body.with_row do |_fields| %> - <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:)) %> + <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:, touched_field_map:)) %> <% end %> <% if should_display_migration_warning? %> diff --git a/app/components/work_packages/progress/work_based/modal_body_component.rb b/app/components/work_packages/progress/work_based/modal_body_component.rb index 2df6328dcc45..bf5e4de1161e 100644 --- a/app/components/work_packages/progress/work_based/modal_body_component.rb +++ b/app/components/work_packages/progress/work_based/modal_body_component.rb @@ -33,7 +33,7 @@ module Progress module WorkBased # rubocop:disable OpenProject/AddPreviewForViewComponent class ModalBodyComponent < BaseModalComponent - def initialize(work_package, focused_field: nil) + def initialize(work_package, focused_field: nil, touched_field_map: {}) super @mode = :work_based diff --git a/app/controllers/work_packages/progress_controller.rb b/app/controllers/work_packages/progress_controller.rb index 4201d91a233b..8d34771fad6a 100644 --- a/app/controllers/work_packages/progress_controller.rb +++ b/app/controllers/work_packages/progress_controller.rb @@ -36,13 +36,18 @@ class WorkPackages::ProgressController < ApplicationController layout false before_action :set_work_package + before_action :extract_persisted_progress_attributes, only: %i[edit create update] helper_method :modal_class def edit build_up_new_work_package - render modal_class.new(@work_package, focused_field: params[:field]) + render modal_class.new(@work_package, + focused_field: params[:field], + touched_field_map: params.require(:work_package).permit("estimated_hours_touched", + "remaining_hours_touched", + "status_id_touched").to_h) end def create @@ -105,10 +110,10 @@ def set_work_package @work_package = WorkPackage.new end - def create_work_package_params - params.require(:work_package) - .permit(allowed_params) - .compact_blank + def extract_persisted_progress_attributes + @persisted_progress_attributes = @work_package + .attributes + .slice("estimated_hours", "remaining_hours", "status_id") end def update_work_package_params @@ -124,12 +129,34 @@ def allowed_params end end + def reject_params_that_dont_differ_from_persisted_values + update_work_package_params.reject do |key, value| + @persisted_progress_attributes[key.to_s].to_f.to_s == value.to_f.to_s + end + end + + def final_params + {}.tap do |filtered_params| + if params.require(:work_package)[:estimated_hours_touched] == "true" + filtered_params[:estimated_hours] = update_work_package_params.fetch("estimated_hours") + end + + if params.require(:work_package)[:remaining_hours_touched] == "true" + filtered_params[:remaining_hours] = update_work_package_params.fetch("remaining_hours") + end + + if params.require(:work_package)[:status_id_touched] == "true" + filtered_params[:status_id] = update_work_package_params.fetch("status_id") + end + end + end + def build_up_new_work_package WorkPackages::SetAttributesService .new(user: current_user, model: @work_package, contract_class: WorkPackages::CreateContract) - .call(create_work_package_params) + .call(final_params) end def formatted_duration(hours) diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb index 83c95aa7584f..14d9e3dc068c 100644 --- a/app/forms/work_packages/progress_form.rb +++ b/app/forms/work_packages/progress_form.rb @@ -29,12 +29,14 @@ class WorkPackages::ProgressForm < ApplicationForm def initialize(work_package:, mode: :work_based, - focused_field: :remaining_hours) + focused_field: :remaining_hours, + touched_field_map: {}) super() @work_package = work_package @mode = mode @focused_field = focused_field_by_selection(focused_field) || focused_field_by_error + @touched_field_map = touched_field_map end form do |query_form| @@ -58,11 +60,29 @@ def initialize(work_package:, render_text_field(group, name: :estimated_hours, label: I18n.t(:label_work)) render_readonly_text_field(group, name: :remaining_hours, label: I18n.t(:label_remaining_work)) + + group.hidden(name: :status_id_touched, + value: @touched_field_map["status_id_touched"] || false, + data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput", + "referrer-field": "work_package[status_id]" }) + group.hidden(name: :estimated_hours_touched, + value: @touched_field_map["estimated_hours_touched"] || false, + data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput", + "referrer-field": "work_package[estimated_hours]" }) else render_text_field(group, name: :estimated_hours, label: I18n.t(:label_work)) render_text_field(group, name: :remaining_hours, label: I18n.t(:label_remaining_work), disabled: disabled_remaining_work_field?) render_readonly_text_field(group, name: :done_ratio, label: I18n.t(:label_percent_complete)) + + group.hidden(name: :estimated_hours_touched, + value: @touched_field_map["estimated_hours_touched"] || false, + data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput", + "referrer-field": "work_package[estimated_hours]" }) + group.hidden(name: :remaining_hours_touched, + value: @touched_field_map["remaining_hours_touched"] || false, + data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput", + "referrer-field": "work_package[remaining_hours]" }) end end end @@ -147,7 +167,9 @@ def format_to_smallest_fractional_part(number) end def default_field_options(name) - data = { "work-packages--progress--preview-progress-target": "progressInput" } + data = { "work-packages--progress--preview-progress-target": "progressInput", + "work-packages--progress--touched-field-marker-target": "progressInput", + action: "input->work-packages--progress--touched-field-marker#markFieldAsTouched" } if @focused_field == name data[:"work-packages--progress--focus-field-target"] = "fieldToFocus" end diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 5ca5ba983ff6..7ee4ef344575 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -151,6 +151,7 @@ def update_derivable work_package.start_date = days.start_date(work_package.due_date, work_package.duration) end end + # rubocop:enable Metrics/AbcSize def set_default_attributes(attributes) @@ -384,7 +385,8 @@ def update_remaining_hours if WorkPackage.use_status_for_done_ratio? update_remaining_hours_from_percent_complete elsif WorkPackage.use_field_for_done_ratio? && - work_package.estimated_hours_changed? + work_package.estimated_hours_changed? + # Here's the issue | Ask Christophe return if work_package.remaining_hours_changed? return if work_package.estimated_hours&.negative? return if work_was_unset_and_remaining_work_is_set? # remaining work is kept and % complete will be set @@ -397,6 +399,7 @@ def update_remaining_hours end end end + # rubocop:enable Metrics/AbcSize,Metrics/PerceivedComplexity def estimated_hours_from_done_ratio_and_remaining_hours @@ -413,8 +416,8 @@ def remaining_hours_from_done_ratio_and_estimated_hours def set_version_to_nil if work_package.version && - work_package.project && - work_package.project.shared_versions.exclude?(work_package.version) + work_package.project && + work_package.project.shared_versions.exclude?(work_package.version) work_package.version = nil end end @@ -485,7 +488,7 @@ def new_start_date def new_start_date_from_parent return unless work_package.parent_id_changed? && - work_package.parent + work_package.parent work_package.parent.soonest_start end diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts index 4df05b9f6917..9bf30325a46f 100644 --- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts @@ -60,7 +60,9 @@ export default class PreviewProgressController extends Controller { }; }; - this.progressInputTargets.forEach((target) => target.addEventListener('input', this.debouncedPreview)); + this.progressInputTargets.forEach((target) => { + target.addEventListener('input', this.debouncedPreview); + }); const turboFrame = this.formTarget.closest('turbo-frame') as HTMLFrameElement; turboFrame.addEventListener('turbo:before-frame-render', this.frameMorphRenderer); @@ -80,7 +82,11 @@ export default class PreviewProgressController extends Controller { const wpParams = [ ['work_package[remaining_hours]', formParams.get('work_package[remaining_hours]') || ''], ['work_package[estimated_hours]', formParams.get('work_package[estimated_hours]') || ''], + ['work_package[status_id]', formParams.get('work_package[status_id]') || ''], ['field', field.name ?? 'estimatedTime'], + ['work_package[remaining_hours_touched]', formParams.get('work_package[remaining_hours_touched]') || ''], + ['work_package[estimated_hours_touched]', formParams.get('work_package[estimated_hours_touched]') || ''], + ['work_package[status_id_touched]', formParams.get('work_package[status_id_touched]') || ''], ]; const wpPath = this.ensureValidPathname(form.action); diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts new file mode 100644 index 000000000000..3191f6ddb75c --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/touched-field-marker.controller.ts @@ -0,0 +1,49 @@ +/* + * -- copyright + * OpenProject is an open source project management software. + * Copyright (C) 2024 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. + * ++ + */ + +import { Controller } from '@hotwired/stimulus'; + +export default class TouchedFieldMarkerController extends Controller { + static targets = [ + 'touchedFieldInput', + 'progressInput', + ]; + + declare readonly touchedFieldInputTargets:HTMLInputElement[]; + declare readonly progressInputTargets:HTMLInputElement[]; + + private markFieldAsTouched(event:{ target:HTMLInputElement }) { + this.touchedFieldInputTargets.forEach((input) => { + if (input.dataset.referrerField === event.target.name) { + input.value = 'true'; + } + }); + } +} diff --git a/spec/features/work_packages/progress_modal_spec.rb b/spec/features/work_packages/progress_modal_spec.rb index 60819511bdba..9f840657fedc 100644 --- a/spec/features/work_packages/progress_modal_spec.rb +++ b/spec/features/work_packages/progress_modal_spec.rb @@ -458,10 +458,10 @@ def update_work_package_with(work_package, attributes) work_edit_field.activate! page.driver.wait_for_network_idle # Wait for initial loading to be ready - work_edit_field.set_value("") + clear_input_field_contents(work_edit_field.input_element) page.driver.wait_for_network_idle # Wait for live-update to finish - pending "Currently sets remaining work to 4h instead of unsetting it" - remaining_work_edit_field.expect_modal_field_value("") + + remaining_work_edit_field.expect_modal_field_value("", disabled: true) end specify "Case 2: when work is set to 12h, " \ @@ -483,7 +483,6 @@ def update_work_package_with(work_package, attributes) work_edit_field.set_value("14") page.driver.wait_for_network_idle # Wait for live-update to finish - pending "Currently remaining work is not updated to 8h" remaining_work_edit_field.expect_modal_field_value("8") end @@ -506,7 +505,6 @@ def update_work_package_with(work_package, attributes) work_edit_field.set_value("12") page.driver.wait_for_network_idle # Wait for live-update to finish - pending "Currently remains at 0h and is not updated to 6h" remaining_work_edit_field.expect_modal_field_value("6") end end From adb3bdda8e074fd22ff1755fc8b80cc026dcc963 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Thu, 18 Apr 2024 11:16:51 +0200 Subject: [PATCH 07/18] Disable autocomplete on progress form --- .../progress/status_based/modal_body_component.html.erb | 1 + .../progress/work_based/modal_body_component.html.erb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/components/work_packages/progress/status_based/modal_body_component.html.erb b/app/components/work_packages/progress/status_based/modal_body_component.html.erb index c852ea2b3076..facf99ae4297 100644 --- a/app/components/work_packages/progress/status_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/status_based/modal_body_component.html.erb @@ -4,6 +4,7 @@ url: submit_path, class: 'progress-form', id: 'progress-form', + html: { autocomplete: "off" }, data: { 'application-target': "dynamic", "work-packages--progress--preview-progress-target": "form", controller: "work-packages--progress--focus-field " \ diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb index 7b50e69d71d1..281a62a565b1 100644 --- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/work_based/modal_body_component.html.erb @@ -4,6 +4,7 @@ url: submit_path, class: "progress-form", id: "progress-form", + html: { autocomplete: "off" }, data: { "application-target": "dynamic", "work-packages--progress--preview-progress-target": "form", controller: "work-packages--progress--focus-field " \ From d0ef196988c74b090ee08faa6fbb6a110f350def Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Thu, 18 Apr 2024 11:17:45 +0200 Subject: [PATCH 08/18] Autocorrect with erb-lint --- .../status_based/modal_body_component.html.erb | 11 ++++++----- .../progress/work_based/modal_body_component.html.erb | 5 +++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/components/work_packages/progress/status_based/modal_body_component.html.erb b/app/components/work_packages/progress/status_based/modal_body_component.html.erb index facf99ae4297..cec2113fa517 100644 --- a/app/components/work_packages/progress/status_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/status_based/modal_body_component.html.erb @@ -2,14 +2,15 @@ <%= primer_form_with( model: work_package, url: submit_path, - class: 'progress-form', - id: 'progress-form', + class: "progress-form", + id: "progress-form", html: { autocomplete: "off" }, - data: { 'application-target': "dynamic", + data: { "application-target": "dynamic", "work-packages--progress--preview-progress-target": "form", controller: "work-packages--progress--focus-field " \ - "work-packages--progress--preview-progress " \ - "work-packages--progress--touched-field-marker" }) do |f| %> + "work-packages--progress--preview-progress " \ + "work-packages--progress--touched-field-marker" } + ) do |f| %> <%= flex_layout do |modal_body| %> <% modal_body.with_row do |_fields| %> <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:, touched_field_map:)) %> diff --git a/app/components/work_packages/progress/work_based/modal_body_component.html.erb b/app/components/work_packages/progress/work_based/modal_body_component.html.erb index 281a62a565b1..7faca0335d3c 100644 --- a/app/components/work_packages/progress/work_based/modal_body_component.html.erb +++ b/app/components/work_packages/progress/work_based/modal_body_component.html.erb @@ -8,8 +8,9 @@ data: { "application-target": "dynamic", "work-packages--progress--preview-progress-target": "form", controller: "work-packages--progress--focus-field " \ - "work-packages--progress--preview-progress " \ - "work-packages--progress--touched-field-marker" }) do |f| %> + "work-packages--progress--preview-progress " \ + "work-packages--progress--touched-field-marker" } + ) do |f| %> <%= flex_layout do |modal_body| %> <% modal_body.with_row do |_fields| %> <%= render(WorkPackages::ProgressForm.new(f, work_package:, mode:, focused_field:, touched_field_map:)) %> From f80ed8c94ee83b309f9c33daa58cb1982556d8c7 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Thu, 18 Apr 2024 11:46:56 +0200 Subject: [PATCH 09/18] Detect cases where remaining work is identical but changed by user nonetheless --- .../work_packages/set_attributes_service.rb | 5 +-- .../set_attributes_service_spec.rb | 42 ++++++++++++++++++- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 7ee4ef344575..6f1a4460a59a 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -332,6 +332,7 @@ def update_done_ratio end def update_remaining_hours_from_percent_complete + return if work_package.remaining_hours_came_from_user? return if work_package.estimated_hours&.negative? work_package.remaining_hours = remaining_hours_from_done_ratio_and_estimated_hours @@ -386,8 +387,7 @@ def update_remaining_hours update_remaining_hours_from_percent_complete elsif WorkPackage.use_field_for_done_ratio? && work_package.estimated_hours_changed? - # Here's the issue | Ask Christophe - return if work_package.remaining_hours_changed? + return if work_package.remaining_hours_came_from_user? return if work_package.estimated_hours&.negative? return if work_was_unset_and_remaining_work_is_set? # remaining work is kept and % complete will be set @@ -399,7 +399,6 @@ def update_remaining_hours end end end - # rubocop:enable Metrics/AbcSize,Metrics/PerceivedComplexity def estimated_hours_from_done_ratio_and_remaining_hours diff --git a/spec/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index c7b4279c2f89..b776c6014c32 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -199,6 +199,13 @@ it_behaves_like "service call", description: "recomputes remaining work according to the % complete value of the new status" end + + context "when floating point operations are inaccurate (2.4000000000000004h)" do + let(:call_attributes) { { estimated_hours: 8.0, status: status_70_pct_complete } } + let(:expected_attributes) { { remaining_hours: 2.4 } } # would be 2.4000000000000004 without rounding + + it_behaves_like "service call", description: "remaining work is computed and rounded (2.4)" + end end context "given a work package with work and remaining work unset, and a status with 0% complete" do @@ -378,10 +385,21 @@ context "when work is changed and remaining work is unset" do let(:call_attributes) { { estimated_hours: 8.0, remaining_hours: nil } } - let(:expected_attributes) { { remaining_hours: 2.4 } } # would be 2.4000000000000004 without rounding + let(:expected_attributes) { call_attributes.dup } let(:expected_kept_attributes) { %w[done_ratio] } - it_behaves_like "service call", description: "% complete is kept and remaining work is recomputed (and rounded)" + it_behaves_like "service call", + description: "% complete is kept and remaining work is kept unset and not recomputed" \ + "(error state to be detected by contract)" + end + + context "when work is increased and remaining work is set to its current value (to prevent it from being increased)" do + # work changed by +10h + let(:call_attributes) { { estimated_hours: 10.0 + 10.0, remaining_hours: 3 } } + let(:expected_attributes) { { remaining_hours: 3.0, done_ratio: 85 } } + + it_behaves_like "service call", + description: "remaining work is kept (not increased), and % complete is updated accordingly" end end @@ -459,6 +477,16 @@ it_behaves_like "service call", description: "remaining work is set to the same value and % complete is set to 0%" end + context "when work is changed and remaining work is unset" do + let(:call_attributes) { { estimated_hours: 10.0, remaining_hours: nil } } + let(:expected_attributes) { call_attributes.dup } + let(:expected_kept_attributes) { %w[done_ratio] } + + it_behaves_like "service call", + description: "% complete is kept and remaining work is kept unset and not recomputed" \ + "(error state to be detected by contract)" + end + context "when work is changed to a negative value" do let(:call_attributes) { { estimated_hours: -1.0 } } let(:expected_kept_attributes) { %w[remaining_hours done_ratio] } @@ -536,6 +564,16 @@ it_behaves_like "service call", description: "% complete is updated accordingly" end + context "when work is set and remaining work is unset" do + let(:call_attributes) { { estimated_hours: 10.0, remaining_hours: nil } } + let(:expected_attributes) { call_attributes.dup } + let(:expected_kept_attributes) { %w[done_ratio] } + + it_behaves_like "service call", + description: "% complete is kept and remaining work is kept unset and not recomputed" \ + "(error state to be detected by contract)" + end + context "when work and remaining work are both set to negative values" do let(:call_attributes) { { estimated_hours: -10, remaining_hours: -5 } } let(:expected_kept_attributes) { %w[done_ratio] } From da1b482fcac3a060fe2a5317e938c088f1b5d4c9 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Fri, 19 Apr 2024 08:47:59 +0200 Subject: [PATCH 10/18] Fix test: initial work package was inconsistent --- spec/workers/copy_project_job_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/workers/copy_project_job_spec.rb b/spec/workers/copy_project_job_spec.rb index 78ee6a18bd6a..2d5fad3e4a88 100644 --- a/spec/workers/copy_project_job_spec.rb +++ b/spec/workers/copy_project_job_spec.rb @@ -178,9 +178,9 @@ let(:params) { { name: "Copy", identifier: "copy" } } let_work_packages(<<~TABLE) - hierarchy | work | start date | end date - parent | 1h | 2024-01-23 | 2024-01-26 - child | 3h | 2024-01-23 | 2024-01-26 + hierarchy | work | remaining work | start date | end date + parent | 1h | 0h | 2024-01-23 | 2024-01-26 + child | 3h | 1.5h | 2024-01-23 | 2024-01-26 TABLE before do From 80a992e551ba95a993a5297578d3ae3d917da42b Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Fri, 19 Apr 2024 09:46:36 -0500 Subject: [PATCH 11/18] Fix modal rendering for new work packages --- .../work_packages/progress_controller.rb | 66 +++++++++++++------ app/forms/work_packages/progress_form.rb | 3 + config/routes.rb | 2 +- .../core/path-helper/path-helper.service.ts | 4 ++ .../progress-popover-edit-field.component.ts | 4 ++ 5 files changed, 57 insertions(+), 22 deletions(-) diff --git a/app/controllers/work_packages/progress_controller.rb b/app/controllers/work_packages/progress_controller.rb index 8d34771fad6a..b55e73694631 100644 --- a/app/controllers/work_packages/progress_controller.rb +++ b/app/controllers/work_packages/progress_controller.rb @@ -40,18 +40,24 @@ class WorkPackages::ProgressController < ApplicationController helper_method :modal_class + def new + build_up_brand_new_work_package + + render modal_class.new(@work_package, + focused_field: params[:field], + touched_field_map:) + end + def edit - build_up_new_work_package + build_up_work_package render modal_class.new(@work_package, focused_field: params[:field], - touched_field_map: params.require(:work_package).permit("estimated_hours_touched", - "remaining_hours_touched", - "status_id_touched").to_h) + touched_field_map:) end def create - service_call = build_up_new_work_package + service_call = build_up_brand_new_work_package if service_call.errors .map(&:attribute) @@ -75,7 +81,7 @@ def update service_call = WorkPackages::UpdateService .new(user: current_user, model: @work_package) - .call(update_work_package_params) + .call(work_package_params) if service_call.success? respond_to do |format| @@ -110,13 +116,19 @@ def set_work_package @work_package = WorkPackage.new end + def touched_field_map + params.require(:work_package).permit("estimated_hours_touched", + "remaining_hours_touched", + "status_id_touched").to_h + end + def extract_persisted_progress_attributes @persisted_progress_attributes = @work_package .attributes .slice("estimated_hours", "remaining_hours", "status_id") end - def update_work_package_params + def work_package_params params.require(:work_package) .permit(allowed_params) end @@ -130,33 +142,45 @@ def allowed_params end def reject_params_that_dont_differ_from_persisted_values - update_work_package_params.reject do |key, value| + work_package_params.reject do |key, value| @persisted_progress_attributes[key.to_s].to_f.to_s == value.to_f.to_s end end - def final_params + def filtered_work_package_params {}.tap do |filtered_params| - if params.require(:work_package)[:estimated_hours_touched] == "true" - filtered_params[:estimated_hours] = update_work_package_params.fetch("estimated_hours") - end + filtered_params[:estimated_hours] = work_package_params["estimated_hours"] if estimated_hours_touched? + filtered_params[:remaining_hours] = work_package_params["remaining_hours"] if remaining_hours_touched? + filtered_params[:status_id] = work_package_params["status_id"] if status_id_touched? + end + end - if params.require(:work_package)[:remaining_hours_touched] == "true" - filtered_params[:remaining_hours] = update_work_package_params.fetch("remaining_hours") - end + def estimated_hours_touched? + params.require(:work_package)[:estimated_hours_touched] == "true" + end - if params.require(:work_package)[:status_id_touched] == "true" - filtered_params[:status_id] = update_work_package_params.fetch("status_id") - end - end + def remaining_hours_touched? + params.require(:work_package)[:remaining_hours_touched] == "true" + end + + def status_id_touched? + params.require(:work_package)[:status_id_touched] == "true" + end + + def build_up_work_package + WorkPackages::SetAttributesService + .new(user: current_user, + model: @work_package, + contract_class: WorkPackages::CreateContract) + .call(filtered_work_package_params) end - def build_up_new_work_package + def build_up_brand_new_work_package WorkPackages::SetAttributesService .new(user: current_user, model: @work_package, contract_class: WorkPackages::CreateContract) - .call(final_params) + .call(work_package_params) end def formatted_duration(hours) diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb index 14d9e3dc068c..f6b5c82f14f3 100644 --- a/app/forms/work_packages/progress_form.rb +++ b/app/forms/work_packages/progress_form.rb @@ -61,6 +61,9 @@ def initialize(work_package:, render_text_field(group, name: :estimated_hours, label: I18n.t(:label_work)) render_readonly_text_field(group, name: :remaining_hours, label: I18n.t(:label_remaining_work)) + # Add a hidden field in create forms as the select field is disabled and is otherwise not included in the form payload + group.hidden(name: :status_id) if @work_package.new_record? + group.hidden(name: :status_id_touched, value: @touched_field_map["status_id_touched"] || false, data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput", diff --git a/config/routes.rb b/config/routes.rb index 202af54fff36..6c95f681534b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -523,7 +523,7 @@ end end - resource :progress, only: %i[edit update], controller: "work_packages/progress" + resource :progress, only: %i[new edit update], controller: "work_packages/progress" collection do resource :progress, only: :create, diff --git a/frontend/src/app/core/path-helper/path-helper.service.ts b/frontend/src/app/core/path-helper/path-helper.service.ts index 74f7cab322f2..d3974bc03368 100644 --- a/frontend/src/app/core/path-helper/path-helper.service.ts +++ b/frontend/src/app/core/path-helper/path-helper.service.ts @@ -265,6 +265,10 @@ export class PathHelperService { } public workPackageProgressModalPath(workPackageId:string|number) { + if (workPackageId === 'new') { + return `${this.workPackagePath(workPackageId)}/progress/new`; + } + return `${this.workPackagePath(workPackageId)}/progress/edit`; } diff --git a/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts b/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts index 5c320d54c051..202f0cdfecfc 100644 --- a/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts +++ b/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts @@ -216,6 +216,10 @@ export class ProgressPopoverEditFieldComponent extends ProgressEditFieldComponen url.searchParams.set('work_package[remaining_hours]', this.formatter(this.resource.remainingTime)); // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access url.searchParams.set('work_package[status_id]', this.statusFormatter(this.resource.status?.id as string)); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (this.resource?.id === 'new') { + url.searchParams.set('work_package[status_id_touched]', 'true'); + } // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access this.frameSrc = url.toString(); From 757700db1510bd30da17f0a2acaeeb8c01dc8bf8 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 22 Apr 2024 09:23:42 +0200 Subject: [PATCH 12/18] Detect cases where work is not changed but set by user nonetheless Reproduction scenario is: * a work package with nothing set, * open the modal, set the work to 10, * => remaining work is set to 10h and % is set to 0% * set remaining work to 8 (so that it's touched) * => % complete is set to 20% * unset work * => before: work was set back to 8h (because not changed from its initial `nil` value) => ko * => after: work is kept as unset and an error message is displayed saying that work must be set if remaining work is set. => ok --- app/services/work_packages/set_attributes_service.rb | 2 +- .../work_packages/set_attributes_service_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/services/work_packages/set_attributes_service.rb b/app/services/work_packages/set_attributes_service.rb index 605ac8cdadca..850c23e47591 100644 --- a/app/services/work_packages/set_attributes_service.rb +++ b/app/services/work_packages/set_attributes_service.rb @@ -372,7 +372,7 @@ def invalid_progress_values? def update_estimated_hours return unless WorkPackage.use_field_for_done_ratio? - return if work_package.estimated_hours_changed? + return if work_package.estimated_hours_came_from_user? return unless work_package.remaining_hours_changed? work = work_package.estimated_hours diff --git a/spec/services/work_packages/set_attributes_service_spec.rb b/spec/services/work_packages/set_attributes_service_spec.rb index bbf4d5b010f3..08b3beee5d8f 100644 --- a/spec/services/work_packages/set_attributes_service_spec.rb +++ b/spec/services/work_packages/set_attributes_service_spec.rb @@ -663,6 +663,16 @@ description: "is an error state (to be detected by contract), " \ "and % complete and work are kept" end + + context "when remaining work is set and work is unset" do + let(:call_attributes) { { estimated_hours: nil, remaining_hours: 6.7 } } + let(:expected_attributes) { call_attributes.dup } + let(:expected_kept_attributes) { %w[done_ratio] } + + it_behaves_like "service call", + description: "% complete is kept and work is kept unset and not recomputed" \ + "(error state to be detected by contract)" + end end end end From 9b657120bded2615476b7746ed68cc19b46a11fe Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 22 Apr 2024 12:24:46 +0200 Subject: [PATCH 13/18] Avoid sending the remainingTime in create form when status-based In status based mode, `remainingTime` is marked as `writable: false`. The popover edit field tries to write it, but that is ignored when submitting the creation, and it renders nil instead. When then submitting, this nil value will be submitted and result in the error "Remaining work must be set when work is set". By setting `render_nil: false` for `remainingTime` in the `WorkPackageRepresenter`, the field is not returned back from the form endpoint when it is not set. This way, the field is not rendered in the form on submit, and the error is avoided. In work-based mode, the remainingTime being not sent back by the form anymore when it is nil means we have to deal with it being undefined, which is why the progress popover had to be modified too. --- .../progress-popover-edit-field.component.ts | 4 ++-- lib/api/v3/work_packages/work_package_representer.rb | 2 +- spec/features/work_packages/progress_modal_spec.rb | 10 ++++++++++ spec/support/toasts/expectations.rb | 8 ++++---- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts b/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts index 202f0cdfecfc..1bf2fad7be6d 100644 --- a/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts +++ b/frontend/src/app/shared/components/fields/edit/field-types/progress-popover-edit-field.component.ts @@ -132,8 +132,8 @@ export class ProgressPopoverEditFieldComponent extends ProgressEditFieldComponen return this.text.placeholder; } - public formatter(value:null|string):string { - if (value === null) { + public formatter(value:undefined|null|string):string { + if (value === undefined || value === null) { return ''; } return `${this.timezoneService.toHours(value)}`; diff --git a/lib/api/v3/work_packages/work_package_representer.rb b/lib/api/v3/work_packages/work_package_representer.rb index eaffde1acf8e..b265ac2761d8 100644 --- a/lib/api/v3/work_packages/work_package_representer.rb +++ b/lib/api/v3/work_packages/work_package_representer.rb @@ -410,7 +410,7 @@ def self_v3_path(*) allow_nil: true) end, writable: ->(*) { !WorkPackage.use_status_for_done_ratio? }, - render_nil: true + render_nil: false property :derived_remaining_time, exec_context: :decorator, diff --git a/spec/features/work_packages/progress_modal_spec.rb b/spec/features/work_packages/progress_modal_spec.rb index adb46f411122..d74b669604d0 100644 --- a/spec/features/work_packages/progress_modal_spec.rb +++ b/spec/features/work_packages/progress_modal_spec.rb @@ -36,6 +36,7 @@ shared_let(:type_task) { create(:type_task) } shared_let(:project) { create(:project, types: [type_task]) } + shared_let(:priority) { create(:default_priority, name: "Normal") } shared_let(:open_status_with_0p_done_ratio) do create(:status, name: "open", default_done_ratio: 0) end @@ -273,6 +274,15 @@ def update_work_package_with(work_package, attributes) role:) end + it "can create work package after setting work" do + work_package_create_page.visit! + + work_package_create_page.set_attributes({ subject: "hello" }) + work_package_create_page.set_progress_attributes({ estimatedTime: "10h" }) + work_package_create_page.save! + work_package_table.expect_and_dismiss_toaster(message: "Successful creation.", wait: 5) + end + it "renders the status selection field inside the modal as disabled " \ "and allows setting the status solely by the top-left field" do work_package_create_page.visit! diff --git a/spec/support/toasts/expectations.rb b/spec/support/toasts/expectations.rb index acd09fc346cf..240bf5946d7a 100644 --- a/spec/support/toasts/expectations.rb +++ b/spec/support/toasts/expectations.rb @@ -1,8 +1,8 @@ module Toasts module Expectations - def expect_toast(message:, type: :success) + def expect_toast(message:, type: :success, wait: 20) if toast_type == :angular - expect(page).to have_css(".op-toast.-#{type}", text: message, wait: 20) + expect(page).to have_css(".op-toast.-#{type}", text: message, wait:) elsif type == :error expect(page).to have_css(".errorExplanation", text: message) elsif type == :success @@ -12,8 +12,8 @@ def expect_toast(message:, type: :success) end end - def expect_and_dismiss_toaster(message: nil, type: :success) - expect_toast(type:, message:) + def expect_and_dismiss_toaster(message: nil, type: :success, wait: 20) + expect_toast(type:, message:, wait:) dismiss_toaster! expect_no_toaster(type:, message:, wait: 0.1) end From b037e762f6f72f035e578ae005ef1fbf051badb1 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 22 Apr 2024 14:17:52 +0200 Subject: [PATCH 14/18] Fix test `remainingTime` is not sent anymore when it's nil (it now has `render_nil: false` in the representer). --- .../work_package_payload_representer_spec.rb | 34 ++++++------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb b/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb index 31897bbade19..25f288df2aee 100644 --- a/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_payload_representer_spec.rb @@ -102,46 +102,34 @@ end describe "estimated hours" do - before do - allow(work_package) - .to receive(:leaf?) - .and_return(true) - end - - it { is_expected.to have_json_path("estimatedTime") } - - it do - expect(subject).to be_json_eql(work_package.estimated_hours.to_json) - .at_path("estimatedTime") - end - context "when not set" do it { is_expected.to have_json_type(NilClass).at_path("estimatedTime") } end context "when set" do - let(:work_package) { build(:work_package, estimated_hours: 0) } + let(:work_package) { build(:work_package, estimated_hours: 2) } it { is_expected.to have_json_type(String).at_path("estimatedTime") } + + it "has a ISO duration representation (PT2H for instance)" do + expect(subject).to be_json_eql("PT2H".to_json).at_path("estimatedTime") + end end end describe "remaining hours" do - it { is_expected.to have_json_path("remainingTime") } - - it do - expect(subject).to be_json_eql(work_package.estimated_hours.to_json) - .at_path("estimatedTime") - end - context "when not set" do - it { is_expected.to have_json_type(NilClass).at_path("estimatedTime") } + it { is_expected.not_to have_json_path("remainingTime") } end context "when set" do let(:work_package) { build(:work_package, estimated_hours: 7, remaining_hours: 5) } - it { is_expected.to have_json_type(String).at_path("estimatedTime") } + it { is_expected.to have_json_type(String).at_path("remainingTime") } + + it "has a ISO duration representation (PT2H for instance)" do + expect(subject).to be_json_eql("PT5H".to_json).at_path("remainingTime") + end end end From f7107dd6bbe706a8397ec697e9d5b50651dcd252 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 22 Apr 2024 16:39:40 +0200 Subject: [PATCH 15/18] Display only one error when work < remaining work --- app/forms/work_packages/progress_form.rb | 36 +++++++++++------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/app/forms/work_packages/progress_form.rb b/app/forms/work_packages/progress_form.rb index f6b5c82f14f3..1102a680d2df 100644 --- a/app/forms/work_packages/progress_form.rb +++ b/app/forms/work_packages/progress_form.rb @@ -27,6 +27,8 @@ # ++ class WorkPackages::ProgressForm < ApplicationForm + attr_reader :work_package + def initialize(work_package:, mode: :work_based, focused_field: :remaining_hours, @@ -35,8 +37,9 @@ def initialize(work_package:, @work_package = work_package @mode = mode - @focused_field = focused_field_by_selection(focused_field) || focused_field_by_error + @focused_field = focused_field_by_selection(focused_field) @touched_field_map = touched_field_map + ensure_only_one_error_for_remaining_work_exceeding_work end form do |query_form| @@ -92,6 +95,19 @@ def initialize(work_package:, private + def ensure_only_one_error_for_remaining_work_exceeding_work + if work_package.errors.added?(:remaining_hours, :cant_exceed_work) && + work_package.errors.added?(:estimated_hours, :cant_be_inferior_to_remaining_work) + error_to_delete = + if @focused_field == :estimated_hours + :remaining_hours + else + :estimated_hours + end + work_package.errors.delete(error_to_delete) + end + end + def focused_field_by_selection(field) if field == :remaining_hours && @work_package.estimated_hours.nil? :estimated_hours @@ -100,24 +116,6 @@ def focused_field_by_selection(field) end end - # First field with an error is focused. If it's readonly or disabled, then the - # field before it will be focused - def focused_field_by_error - fields = if @mode == :work_based - %i[estimated_hours remaining_hours done_ratio] - else - %i[status_id estimated_hours remaining_hours] - end - - fields.each do |field_name| - break if @focused_field - - @focused_field = field_name if @work_package.errors.map(&:attribute).include?(field_name) - end - - @focused_field - end - def render_text_field(group, name:, label:, From 4354d8f2391407bf4c3bc7bcdd35f9b2de0c9e64 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 22 Apr 2024 17:42:32 +0200 Subject: [PATCH 16/18] Make the test pass with a sleep I do not like sleep like this. But because I do not manage to find a better synchronization strategy for the turbo stream to come back and update values, I prefer something slow and working than something fast an breaking. Using `page.driver.wait_for_network_idle` does not seem to wait for long enough as Angular has to do its Angular things to update the model being created. Please fix it if you can. --- spec/support/edit_fields/progress_edit_field.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/support/edit_fields/progress_edit_field.rb b/spec/support/edit_fields/progress_edit_field.rb index fc2f2cd89b3b..897b613998b9 100644 --- a/spec/support/edit_fields/progress_edit_field.rb +++ b/spec/support/edit_fields/progress_edit_field.rb @@ -73,6 +73,7 @@ def active? def set_value(value) page.fill_in field_name, with: value + sleep 1 end def input_element From 511f982162759171456fd46cb49dbf30db9ae707 Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Mon, 22 Apr 2024 12:10:00 -0500 Subject: [PATCH 17/18] Fix share/filter_spec For some reason this stopped working on the live-preview branch. Upon inspecting the behavior of the method with some debuggers, it seemed correct that the specs were to fail as it does fail as well upon manual testing. What's interesting is that it only fails: 1. On subsequent modal updates 2. Not on initial modal loading This is the best fix I could come up with in the meantime. I'm not aware of a reason within the current branch as to why this stopped working. --- .../work_packages/share/permission_button_component.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/work_packages/share/permission_button_component.rb b/app/components/work_packages/share/permission_button_component.rb index 9bfee0e573d2..8205a1d6b07e 100644 --- a/app/components/work_packages/share/permission_button_component.rb +++ b/app/components/work_packages/share/permission_button_component.rb @@ -72,7 +72,7 @@ def active_role end def permission_name(value) - options.select { |option| option[:value] == value } + options.find { |option| option[:value] == value }[:label] end def form_inputs(role_id) From 3e57d488b8af09d209da017e5a2ef09f691836ff Mon Sep 17 00:00:00 2001 From: Aaron Contreras Date: Tue, 23 Apr 2024 08:07:50 -0500 Subject: [PATCH 18/18] Ensure that relative urls are respected --- .../work-packages/progress/preview-progress.controller.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts index 9bf30325a46f..091f5cdf8ef2 100644 --- a/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts @@ -104,8 +104,9 @@ export default class PreviewProgressController extends Controller { private ensureValidPathname(formAction:string):string { const wpPath = new URL(formAction); - if (wpPath.pathname === '/work_packages/progress') { - wpPath.pathname = '/work_packages/new/progress'; + if (wpPath.pathname.endsWith('/work_packages/progress')) { + // Replace /work_packages/progress with /work_packages/new/progress + wpPath.pathname = wpPath.pathname.replace('/work_packages/progress', '/work_packages/new/progress'); } return wpPath.toString();