[57423] Progress modal should always prevent save when there are invalid values #16735
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge `progress-preview` and `touched-field-marker` stimulus controllers together into `preview` controller. They are sharing some state, the touched status, showing that they need to be grouped together. It will make it easier to deal with the touched state later.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/src/stimulus/controllers/dynamic/work-packages/progress/preview.controller.ts
Fixed
Show fixed
Hide fixed
cbliard
force-pushed
the
bug/57423-prevent-popover-save-on-invalid-progress-values
branch
from
ebdd605 to
61b6f36
Compare
Because of the weird way we initialize the fake work package, it could believe that the status did not change and no recomputation was needed. This is an edge case, and I'm not satisfied with its design.
cbliard
force-pushed
the
bug/57423-prevent-popover-save-on-invalid-progress-values
branch
from
61b6f36 to
c40bd5f
Compare
When submitting with invalid values, the progress popover was updated via turbo-stream with a modal which did not include the touched fields. This caused the fields to be ignored on the second submit, even if they appeared to be invalid. This was also causing the preview to not work as expected. This has been changed: - the update is done through a turbo-stream morph instead of an update. It avoids disconnecting and reconnecting the stimulus controllers. - the touched fields are now correctly sent to the component for rendering. The state is maintained in the stimulus controller instead of the hidden fields. Fields are still needed for form submission. - the fields sent to the server when previewing are exactly the same as the ones being sent on submit. - the turbo_stream commands are sent directly from the controller. None are sent if the update completed successfully as the modal will be closed by client side javascript anyway. - the referrer-field is now just the field name instead of the full work package key, to reduce noise.
cbliard
force-pushed
the
bug/57423-prevent-popover-save-on-invalid-progress-values
branch
from
284f559 to
0a5765f
Compare
| // Turbo supports morphing, by adding the <turbo-frame refresh="morph"> 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 <turbo-frame refresh="morph"> instead. |
There was a problem hiding this comment.
Could it be that this is now fixed with 8.0.10?
See:
https://github.com/hotwired/turbo/releases/tag/v8.0.10
hotwired/turbo#1192
There was a problem hiding this comment.
It seems I can't make it work properly. I'll merge this one, and open another PR.
oliverguenther
approved these changes
Sep 18, 2024
There was a problem hiding this comment.
👍 Nice cleanup. I feel like the update flow is now a lot easier. Current behavior fixed as specified in the expected behavior.
There is a small remark on whether the newest update from today might be able to resolve your workaround. I'm fine with merging it before checking.
cbliard
deleted the
bug/57423-prevent-popover-save-on-invalid-progress-values
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/57423
What are you trying to accomplish?
In progress popover, when inputs are invalid, the modal can still be "saved" when the Save button is pressed multiple time. Nothing is actually changed, but this is still confusing behavior.
Change it so that the popover remains open.
Screenshots
Before:
double.submit.not.blocked.webm
After:
multiple.save.does.not.submit.webm
What approach did you choose and why?
The problem came from the modal form being updated through turbo-stream on submit. In this code path, the rendered modal form did not include the touched fields information, which lead to no fields being touched, meaning it saves without any modifications.
To fix it:
And to clarify the code:
updateaction:updatetemplatecreateaction:updatetemplatemorphinstead ofupdatewith turbo-stream to avoid disconnecting and reconnecting stimulus controllersAnd as a bonus:
Merge checklist