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

dombesz · 2024-04-10T13:49:01Z

Description

This is a follow up PR on #14768 , adding live updating on the % done when editing working hours.
The PR also experiments with introducing turbo frame morphing across the project for a smoother turbo rendering experience. Once this PR is merged, we don't need to implement our custom morph rendering, just use the one provided with turbo.

Work Package: https://community.openproject.org/work_packages/54275

Edge cases that do not work correctly

There are some edge cases that do not work correctly with the live update. Once they are addressed, we can merge this PR:

Case 1
1. initially: work based mode, work = 10h, remaining work = 4h, % complete = 60%
2. try unsetting work
3. expected: should unset remaining work
4. actual: set work back to 10h => ko
Case 2
1. initially: work based mode, work = 10h, remaining work = 4h, % complete = 60%
2. try setting work to 12h (+2h)
3. remaining work is automatically set to 6h (+2h) => ok
4. now try setting work to 14h (+2h again)
5. expected: remaining work updates to 8h (+2h again)
6. actual: remaining work does not change anymore => ko
Case 3
1. initially: work based mode, work = 10h, remaining work = 4h, % complete = 60%
2. try setting work to 2h (-8h)
3. remaining work is automatically set to 0h (-8h but can't go below 0h) => ok
4. now try setting work to 12h (+2h from initial 10h value)
5. expected: remaining work updates to 6h (+2h from initial value)
6. actual: remaining work does not change anymore => ko
Case 4 (@aaron-contreras)
1. in a create work package form
2. Fill out any field
3. expected: live update works
4. actual: 404 error
Case 5 (@cbliard)
1. initially: work based mode in a create work package form, work = unset, remaining work = unset, % complete = unset
2. try setting work to 4h
3. remaining work is automatically set to 4h => ok
4. try unsetting remaining work
5. expected: remaining work is unset and error message indicating remaining work must be set when work is set
6. actual: remaining work gets set back to 4h

…he frame re-rendering.

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.

app/controllers/work_packages/progress_controller.rb

…netheless

app/controllers/work_packages/progress_controller.rb

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


app/controllers/work_packages/progress_controller.rb


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


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

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.

`remainingTime` is not sent anymore when it's nil (it now has `render_nil: false` in the representer).

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.

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.

ulferts

I haven't played around with it for a very long time. But the cases I tried all worked so I think it is almost done.

I suspect there is an error in there when a relative url root is configured for the application, though.

ulferts · 2024-04-23T08:27:49Z

frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts

+      ['work_package[status_id_touched]', formParams.get('work_package[status_id_touched]') || ''],
+    ];
+
+    const wpPath = this.ensureValidPathname(form.action);


I haven't tried it out but I suspect this will fail if the application is run with a relative url root like /custom. In this case, the matching would not work.

ulferts · 2024-04-23T11:51:17Z

frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview-progress.controller.ts

+  private frameMorphRenderer:(event:CustomEvent<TurboBeforeFrameRenderEventDetail>) => void;
+
+  connect() {
+    this.debouncedPreview = debounce((event:Event) => { void this.preview(event); }, 500);


I would reduce the debounce time as it feels a bit sluggish. On the other hand, when reducing the time, I oftentimes ran into the situation where my last input would be eaten up. Knowing of this problem, even when not changing the time, I could time my input so that they would be eaten up. I don't know if it is just a forced scenario or something that is likely to happen to users that just type in numbers at the wrong time. Since the inserted values normally shouldn't be that long I would think that it is less likely to happen so I would be willing to accept this potential problem and also the need for the sluggy behaviour.

In datepickers, we have a similar pattern and also use a debounce of 500: https://github.com/opf/openproject/blob/dev/frontend/src/app/shared/components/datepicker/wp-multi-date-form/wp-multi-date-form.component.ts#L802

So I'd leave it as it is and fine-tune later

oliverguenther

Tiny code review, LGTM/WCGW!

dombesz changed the base branch from dev to feature/40749-consistent-calculation-of-percent-complete April 10, 2024 14:55

dombesz force-pushed the feature-add-turbo-frame-morphing branch from 960dc33 to 12b3edc Compare April 10, 2024 14:57

dombesz added 3 commits April 10, 2024 18:01

Use morphing on turbo frame re-rendering.

12b3edc

Refresh remaining work calculations on the work estimate modal.

d213274

Preserve cursor position of the progress modal by using morphing on t…

3fd2fe1

…he frame re-rendering.

dombesz changed the title ~~Use morphing on turbo frame re-rendering.~~ Live update the work package progress modal with morphing on turbo frame re-rendering. Apr 10, 2024

Base automatically changed from feature/40749-consistent-calculation-of-percent-complete to dev April 10, 2024 18:39

cbliard changed the base branch from dev to release/14.0 April 11, 2024 14:05

aaron-contreras added the feature label Apr 12, 2024

aaron-contreras changed the title ~~Live update the work package progress modal with morphing on turbo frame re-rendering.~~ [#54275] Live update the work package progress modal with morphing on turbo frame re-rendering. Apr 15, 2024

aaron-contreras added 2 commits April 15, 2024 09:53

Add feature specs for cases known to be buggy with live updating

7c6552b

Merge branch 'release/14.0' into feature-add-turbo-frame-morphing

a19dcdf

github-advanced-security bot found potential problems Apr 17, 2024

View reviewed changes

app/controllers/work_packages/progress_controller.rb Fixed Show fixed Hide fixed

Flag changed fields for change detection on server side

e086ecb

aaron-contreras force-pushed the feature-add-turbo-frame-morphing branch from 8b478b3 to e086ecb Compare April 17, 2024 15:49

cbliard and others added 5 commits April 18, 2024 11:16

Disable autocomplete on progress form

adb3bdd

Autocorrect with erb-lint

d0ef196

Detect cases where remaining work is identical but changed by user no…

f80ed8c

…netheless

Merge branch 'release/14.0' into feature-add-turbo-frame-morphing

309a5ba

Fix test: initial work package was inconsistent

da1b482

cbliard added the pullpreview label Apr 19, 2024

github-advanced-security bot found potential problems Apr 19, 2024

View reviewed changes

Fix modal rendering for new work packages

80a992e

aaron-contreras force-pushed the feature-add-turbo-frame-morphing branch from 003bd67 to 80a992e Compare April 19, 2024 22:48

github-advanced-security bot found potential problems Apr 19, 2024

View reviewed changes

cbliard added 4 commits April 22, 2024 09:23

Fix test

b037e76

`remainingTime` is not sent anymore when it's nil (it now has `render_nil: false` in the representer).

Display only one error when work < remaining work

f7107dd

cbliard and others added 3 commits April 22, 2024 17:42

Merge branch 'release/14.0' into feature-add-turbo-frame-morphing

4fabb80

cbliard marked this pull request as ready for review April 23, 2024 07:55

ulferts requested changes Apr 23, 2024

View reviewed changes

ulferts reviewed Apr 23, 2024

View reviewed changes

Ensure that relative urls are respected

3e57d48

aaron-contreras requested review from ulferts and oliverguenther April 23, 2024 13:10

ulferts approved these changes Apr 23, 2024

View reviewed changes

oliverguenther approved these changes Apr 23, 2024

View reviewed changes

oliverguenther merged commit fe7b5b1 into release/14.0 Apr 23, 2024
12 checks passed

oliverguenther deleted the feature-add-turbo-frame-morphing branch April 23, 2024 13:32

github-actions bot removed the pullpreview label Apr 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dombesz commented Apr 10, 2024 •

edited by cbliard

Loading

ulferts left a comment

ulferts Apr 23, 2024 •

edited

Loading

ulferts Apr 23, 2024

oliverguenther Apr 23, 2024

oliverguenther left a comment

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

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

Conversation

dombesz commented Apr 10, 2024 • edited by cbliard Loading

Description

Edge cases that do not work correctly

ulferts left a comment

Choose a reason for hiding this comment

ulferts Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

ulferts Apr 23, 2024

Choose a reason for hiding this comment

oliverguenther Apr 23, 2024

Choose a reason for hiding this comment

oliverguenther left a comment

Choose a reason for hiding this comment

dombesz commented Apr 10, 2024 •

edited by cbliard

Loading

ulferts Apr 23, 2024 •

edited

Loading