Skip to content

Commit

Permalink
Keep Work value when it's set
Browse files Browse the repository at this point in the history
If Remaining work and % Complete are both modified and Work is initially
empty, then it will be derived, but if it is set then it's % Complete
and Remaining work which will change.

The goal is to keep the Work value unchanged as much as possible.
  • Loading branch information
cbliard committed Aug 22, 2024
1 parent 217be2e commit 078d21c
Show file tree
Hide file tree
Showing 8 changed files with 203 additions and 31 deletions.
6 changes: 5 additions & 1 deletion app/forms/work_packages/progress_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,15 @@ def readonly_text_field(group,

def hidden_touched_field(group, name:)
group.hidden(name: :"#{name}_touched",
value: @touched_field_map["#{name}_touched"] || false,
value: touched(name),
data: { "work-packages--progress--touched-field-marker-target": "touchedFieldInput",
"referrer-field": "work_package[#{name}]" })
end

def touched(name)
@touched_field_map["#{name}_touched"] || false
end

def field_value(name)
errors = @work_package.errors.where(name)
if (user_value = errors.map { |error| error.options[:value] }.find { !_1.nil? })
Expand Down
24 changes: 14 additions & 10 deletions app/forms/work_packages/progress_form/initial_values_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,25 @@ def initialize(work_package:,

form do |form|
if mode == :status_based
form.hidden(name: :status_id,
value: work_package.status_id_was)
form.hidden(name: :estimated_hours,
value: work_package.estimated_hours_was)
hidden_initial_field(form, name: :status_id)
hidden_initial_field(form, name: :estimated_hours)
else
form.hidden(name: :estimated_hours,
value: work_package.estimated_hours_was)
form.hidden(name: :remaining_hours,
value: work_package.remaining_hours_was)
hidden_initial_field(form, name: :estimated_hours)
hidden_initial_field(form, name: :remaining_hours)
# next line to be removed in 15.0 with :percent_complete_edition feature flag removal
next unless OpenProject::FeatureDecisions.percent_complete_edition_active?

form.hidden(name: :done_ratio,
value: work_package.done_ratio_was)
hidden_initial_field(form, name: :done_ratio)
end
end

private

def hidden_initial_field(form, name:)
form.hidden(name:,
value: work_package.public_send(:"#{name}_was"),
data: { "work-packages--progress--touched-field-marker-target": "initialValueInput",
"referrer-field": "work_package[#{name}]" })
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -91,20 +91,22 @@ export default class PreviewProgressController extends Controller {
const formData = new FormData(form) as unknown as undefined;
const formParams = new URLSearchParams(formData);
const wpParams = [
['work_package[initial][remaining_hours]', formParams.get('work_package[initial][remaining_hours]') || ''],
['work_package[initial][estimated_hours]', formParams.get('work_package[initial][estimated_hours]') || ''],
['work_package[initial][remaining_hours]', formParams.get('work_package[initial][remaining_hours]') || ''],
['work_package[initial][done_ratio]', formParams.get('work_package[initial][done_ratio]') || ''],
['work_package[remaining_hours]', formParams.get('work_package[remaining_hours]') || ''],
['work_package[estimated_hours]', formParams.get('work_package[estimated_hours]') || ''],
['work_package[remaining_hours]', formParams.get('work_package[remaining_hours]') || ''],
['work_package[done_ratio]', formParams.get('work_package[done_ratio]') || ''],
['work_package[status_id]', formParams.get('work_package[status_id]') || ''],
['field', field?.name ?? ''],
['work_package[estimated_hours_touched]', formParams.get('work_package[estimated_hours_touched]') || ''],
['work_package[remaining_hours_touched]', formParams.get('work_package[remaining_hours_touched]') || ''],
['work_package[done_ratio_touched]', formParams.get('work_package[done_ratio_touched]') || ''],
['work_package[status_id_touched]', formParams.get('work_package[status_id_touched]') || ''],
];

this.progressInputTargets.forEach((progressInput) => {
const touchedInputName = progressInput.name.replace(']', '_touched]');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding High

This replaces only the first occurrence of ']'.
const touchedValue = formParams.get(touchedInputName) || '';
wpParams.push([touchedInputName, touchedValue]);
});

const wpPath = this.ensureValidPathname(form.action);
const wpAction = wpPath.endsWith('/work_packages/new/progress') ? 'new' : 'edit';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,122 @@ import { Controller } from '@hotwired/stimulus';

export default class TouchedFieldMarkerController extends Controller {
static targets = [
'initialValueInput',
'touchedFieldInput',
'progressInput',
];

declare readonly initialValueInputTargets:HTMLInputElement[];
declare readonly touchedFieldInputTargets:HTMLInputElement[];
declare readonly progressInputTargets:HTMLInputElement[];

private targetFieldName:string;

private markFieldAsTouched(event:{ target:HTMLInputElement }) {
this.touchedFieldInputTargets.forEach((input) => {
if (input.dataset.referrerField === event.target.name) {
input.value = 'true';
this.targetFieldName = event.target.name.replace(/^work_package\[([^\]]+)\]$/, '$1');
const touchedInput = this.findTouchedInput(this.targetFieldName);

if (touchedInput) {
touchedInput.value = 'true';
this.keepWorkValue();
}
}

private keepWorkValue() {
if (this.isInitialValueEmpty('estimated_hours') && !this.isTouched('estimated_hours')) {
// let work be derived
return;
}

if (this.isBeingEdited('estimated_hours')) {
this.untouchFieldsWhenWorkIsEdited();
} else if (this.isBeingEdited('remaining_hours')) {
this.untouchFieldsWhenRemainingWorkIsEdited();
} else if (this.isBeingEdited('done_ratio')) {
this.untouchFieldsWhenPercentCompleteIsEdited();
}
}

private untouchFieldsWhenWorkIsEdited() {
if (this.areBothTouched('remaining_hours', 'done_ratio')) {
if (this.isValueEmpty('done_ratio')) {
this.markUntouched('done_ratio');
} else {
this.markUntouched('remaining_hours');
}
});
}
}

private untouchFieldsWhenRemainingWorkIsEdited() {
if (this.isValueSet('estimated_hours')) {
this.markUntouched('done_ratio');
}
}

private untouchFieldsWhenPercentCompleteIsEdited() {
if (this.isValueSet('estimated_hours')) {
this.markUntouched('remaining_hours');
}
}

private areBothTouched(fieldName1:string, fieldName2:string) {
return this.isTouched(fieldName1) && this.isTouched(fieldName2);
}

private isBeingEdited(fieldName:string) {
return fieldName === this.targetFieldName;
}

// Finds the hidden initial value input based on a field name.
//
// The initial value input field holds the initial value of the work package
// before being set by the user or derived.
private findInitialValueInput(fieldName:string):HTMLInputElement|undefined {
return this.initialValueInputTargets.find((input) =>
(input.dataset.referrerField === fieldName) || (input.dataset.referrerField === `work_package[${fieldName}]`));
}

// Finds the touched field input based on a field name.
//
// The touched input field is used to mark a field as touched by the user so
// that the backend keeps the value instead of deriving it.
private findTouchedInput(fieldName:string):HTMLInputElement|undefined {
return this.touchedFieldInputTargets.find((input) =>
(input.dataset.referrerField === fieldName) || (input.dataset.referrerField === `work_package[${fieldName}]`));
}

// Finds the value field input based on a field name.
//
// The value field input holds the current value of a progress field.
private findValueInput(fieldName:string):HTMLInputElement|undefined {
return this.progressInputTargets.find((input) =>
(input.name === fieldName) || (input.name === `work_package[${fieldName}]`));
}

private isTouched(fieldName:string) {
const touchedInput = this.findTouchedInput(fieldName);
return touchedInput?.value === 'true';
}

private isInitialValueEmpty(fieldName:string) {
const valueInput = this.findInitialValueInput(fieldName);
return valueInput?.value === '';
}

private isValueEmpty(fieldName:string) {
const valueInput = this.findValueInput(fieldName);
return valueInput?.value === '';
}

private isValueSet(fieldName:string) {
const valueInput = this.findValueInput(fieldName);
return valueInput !== undefined && valueInput.value !== '';
}

private markUntouched(fieldName:string) {
const touchedInput = this.findTouchedInput(fieldName);
if (touchedInput) {
touchedInput.value = 'false';
}
}
}
33 changes: 33 additions & 0 deletions spec/features/work_packages/progress_modal_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,39 @@ def visit_progress_query_displaying_work_package
progress_popover.set_values(work: "12")
progress_popover.expect_values(remaining_work: "6h")
end

specify "Case 4: when remaining work or % complete are set, work never " \
"changes, instead remaining work and % complete are derived" do
visit_progress_query_displaying_work_package

progress_popover.open
progress_popover.set_values(remaining_work: "2h")
progress_popover.expect_values(work: "10h", percent_complete: "80%")

progress_popover.set_values(percent_complete: "50%")
progress_popover.expect_values(work: "10h", remaining_work: "5h")

progress_popover.set_values(remaining_work: "9h")
progress_popover.expect_values(work: "10h", percent_complete: "10%")
end
end

context "given work, remaining work, and % complete are all empty" do
before do
update_work_package_with(work_package, estimated_hours: nil, remaining_hours: nil, done_ratio: nil)
end

specify "Case 1: when remaining work and % complete are both set, work " \
"is derived because it's empty" do
visit_progress_query_displaying_work_package

progress_popover.open
progress_popover.set_values(remaining_work: "2h", percent_complete: "50%")
progress_popover.expect_values(work: "4h")

progress_popover.set_values(remaining_work: "10h")
progress_popover.expect_values(work: "20h")
end
end
end
end
11 changes: 4 additions & 7 deletions spec/support/components/work_packages/progress_popover.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,13 @@ def save
field(:work).submit_by_clicking_save
end

def clear(field_name)
clear_input_field_contents(field(field_name).input_element)
def focus(field_name)
field(field_name).focus
end

def set_value(field_name, value)
if value == ""
clear(field_name)
else
field(field_name).set_value(value)
end
focus(field_name)
field(field_name).set_value(value)
end

def set_values(**field_value_pairs)
Expand Down
6 changes: 5 additions & 1 deletion spec/support/edit_fields/edit_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ def label_element

def clear(with_backspace: false)
if with_backspace
input_element.set(" ", fill_options: { clear: :backspace })
if using_cuprite?
clear_input_field_contents(input_element)
else
input_element.set(" ", fill_options: { clear: :backspace })
end
else
input_element.native.clear
end
Expand Down
28 changes: 26 additions & 2 deletions spec/support/edit_fields/progress_edit_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,33 @@ def active?
page.has_selector?(MODAL_SELECTOR, wait: 1)
end

def clear
super(with_backspace: true)
end

def set_value(value)
page.fill_in field_name, with: value
sleep 1
if value == ""
clear
else
page.fill_in field_name, with: value
end
wait_for_preview_to_complete
end

def focus
return if focused?

input_element.click
wait_for_preview_to_complete
end

# Wait for the popover preview to be refreshed.
# Preview occurs on field blur or change.
def wait_for_preview_to_complete
sleep 0.110 # the preview on popover has a debounce of 100ms
if using_cuprite?
wait_for_network_idle # Wait for preview to finish
end
end

def input_element
Expand Down

0 comments on commit 078d21c

Please sign in to comment.