Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#54275] Live update the work package progress modal with morphing on turbo frame re-rendering. #15213

Merged
merged 21 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
12b3edc
Use morphing on turbo frame re-rendering.
dombesz Apr 10, 2024
d213274
Refresh remaining work calculations on the work estimate modal.
dombesz Apr 10, 2024
3fd2fe1
Preserve cursor position of the progress modal by using morphing on t…
dombesz Apr 10, 2024
4a953fe
Ensure modals can be previewed in the context of create forms
aaron-contreras Apr 12, 2024
7c6552b
Add feature specs for cases known to be buggy with live updating
aaron-contreras Apr 15, 2024
a19dcdf
Merge branch 'release/14.0' into feature-add-turbo-frame-morphing
aaron-contreras Apr 15, 2024
e086ecb
Flag changed fields for change detection on server side
aaron-contreras Apr 16, 2024
adb3bdd
Disable autocomplete on progress form
cbliard Apr 18, 2024
d0ef196
Autocorrect with erb-lint
cbliard Apr 18, 2024
f80ed8c
Detect cases where remaining work is identical but changed by user no…
cbliard Apr 18, 2024
309a5ba
Merge branch 'release/14.0' into feature-add-turbo-frame-morphing
aaron-contreras Apr 18, 2024
da1b482
Fix test: initial work package was inconsistent
cbliard Apr 19, 2024
80a992e
Fix modal rendering for new work packages
aaron-contreras Apr 19, 2024
757700d
Detect cases where work is not changed but set by user nonetheless
cbliard Apr 22, 2024
9b65712
Avoid sending the remainingTime in create form when status-based
cbliard Apr 22, 2024
b037e76
Fix test
cbliard Apr 22, 2024
f7107dd
Display only one error when work < remaining work
cbliard Apr 22, 2024
4354d8f
Make the test pass with a sleep
cbliard Apr 22, 2024
511f982
Fix share/filter_spec
aaron-contreras Apr 22, 2024
4fabb80
Merge branch 'release/14.0' into feature-add-turbo-frame-morphing
cbliard Apr 23, 2024
3e57d48
Ensure that relative urls are respected
aaron-contreras Apr 23, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions app/components/work_packages/progress/base_modal_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,26 @@ 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,
"work_package[status_id]" => :status_id,
"statusId" => :status_id
}.freeze

include ApplicationHelper
include Turbo::FramesHelper
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,18 @@
<%= primer_form_with(
model: work_package,
url: submit_path,
class: 'progress-form',
id: 'progress-form',
data: { 'application-target': "dynamic",
controller: "work-packages--progress--focus-field" }) do |f| %>
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 " \
"work-packages--progress--preview-progress " \
"work-packages--progress--touched-field-marker" }
) do |f| %>
<%= flex_layout do |modal_body| %>
<% modal_body.with_row(classes: "FormControl-horizontalGroup--sm-vertical") 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| %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,16 @@
url: submit_path,
class: "progress-form",
id: "progress-form",
html: { autocomplete: "off" },
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 " \
"work-packages--progress--touched-field-marker" }
) do |f| %>
<%= flex_layout do |modal_body| %>
<% modal_body.with_row(classes: "FormControl-horizontalGroup--sm-vertical") 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? %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
73 changes: 62 additions & 11 deletions app/controllers/work_packages/progress_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,28 @@

layout false
before_action :set_work_package
before_action :extract_persisted_progress_attributes, only: %i[edit create update]

helper_method :modal_class

def new
build_up_brand_new_work_package

render modal_class.new(@work_package,
focused_field: params[:field],

Check notice

Code scanning / Brakeman

Render path contains parameter value. Note

Render path contains parameter value.
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])
render modal_class.new(@work_package,
focused_field: params[:field],
Fixed Show fixed Hide fixed

Check notice

Code scanning / Brakeman

Render path contains parameter value. Note

Render path contains parameter value.
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)
Expand All @@ -70,7 +81,7 @@
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|
Expand Down Expand Up @@ -105,13 +116,19 @@
@work_package = WorkPackage.new
end

def create_work_package_params
params.require(:work_package)
.permit(allowed_params)
.compact_blank
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
Expand All @@ -124,12 +141,46 @@
end
end

def build_up_new_work_package
def reject_params_that_dont_differ_from_persisted_values
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 filtered_work_package_params
{}.tap do |filtered_params|
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

def estimated_hours_touched?
params.require(:work_package)[:estimated_hours_touched] == "true"
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_brand_new_work_package
WorkPackages::SetAttributesService
.new(user: current_user,
model: @work_package,
contract_class: WorkPackages::CreateContract)
.call(create_work_package_params)
.call(work_package_params)
end

def formatted_duration(hours)
Expand Down
69 changes: 46 additions & 23 deletions app/forms/work_packages/progress_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,19 @@
# ++

class WorkPackages::ProgressForm < ApplicationForm
attr_reader :work_package

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
@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|
Expand All @@ -58,17 +63,51 @@ 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",
"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

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
Expand All @@ -77,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:,
Expand Down Expand Up @@ -147,11 +168,13 @@ def format_to_smallest_fractional_part(number)
end

def default_field_options(name)
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" } }
else
{}
data[:"work-packages--progress--focus-field-target"] = "fieldToFocus"
end
{ data: }
end

# Remaining work field is enabled when work is set, or when there are errors
Expand Down
14 changes: 8 additions & 6 deletions app/services/work_packages/set_attributes_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -343,6 +344,7 @@ def round_progress_values
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
Expand Down Expand Up @@ -376,7 +378,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
Expand All @@ -396,8 +398,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?
return if work_package.remaining_hours_changed?
work_package.estimated_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

Expand Down Expand Up @@ -426,8 +428,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
Expand Down Expand Up @@ -498,7 +500,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
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading
Loading