[OP-19606] Preserve move form values on refresh by myabc · Pull Request #24062 · opf/openproject

myabc · 2026-07-02T10:31:17Z

Note

This PR is stacked on #23938 and #24061. Please review/merge those PRs first.

Ticket

https://community.openproject.org/wp/OP-19606

What are you trying to accomplish?

On the move/copy work package form, changing the target project or type triggers a form refresh that wiped every previously filled field (dates, assignee, priority, custom fields, …) — even when those fields were still applicable in the target project.

Screen recording

Screen.Recording.2026-07-02.at.12.27.12.mov

What approach did you choose and why?

The submitted values are passed back into WorkPackages::Moves::FormComponent as selected_values (rendering only), so the refreshed form keeps the user's current selections; the persisted move/copy attributes stay on the existing permitted-params path.
The permitted form values are defined centrally in PermittedParams#move_work_package_form_values, shared by the refresh and the move itself; malformed custom-field params are guarded.
The component derives its own type/version/status collections (with current_user injected) and warns when a moved work package's current type does not exist in the target project.

Review follow-ups

Renamed types → available_types and let the component compute its own refresh URL.
new_type_id/new_project_id are now normalized inside PermittedParams#move_work_package_form_values instead of being passed separately.
The unavailable-type warning copy now also mentions descendants.
Drive-by fix (separate commit): the status dropdown was populated from the user's roles in the source project, although the chosen status is applied in the target project. It now derives statuses from the target project. Pre-existing issue raised by @EinLama in review.

Important

Note for QA: the move/copy form's status dropdown now lists statuses from the user's workflows in the target project (previously: source project). No visible change for admins; non-admin users with differing roles between source and target will see a different status list after picking a target project.

Stacked PRs (split per work package)

Stacks on #23938 ([SC-272] streamable form component + POST refresh), which is based on #24061 ([OP-19677] — preserving the Budget field relies on budget_id being permitted). Merge order: #24061 → #23938 → this PR.

Merge checklist

Added/updated tests
Tested major browsers (Chrome, Firefox, Edge, ...)

Copilot

Pull request overview

This PR addresses the bulk move/copy work package form losing already-entered values (assignee, dates, priority, custom fields, etc.) when the form refreshes after changing target project/type. It does so by round-tripping the currently selected form values back into the move form component (render-only), while centralizing and hardening the permitted parameter handling used by both refresh and execution.

Changes:

Preserve user-entered move/copy form values across Turbo Stream refreshes by passing selected_values into WorkPackages::Moves::FormComponent.
Centralize move form value permitting via PermittedParams#move_work_package_form_values and guard against malformed custom field params.
Move form option derivation (types/versions/statuses) and “type unavailable in target project” warning logic into the component, and expand test coverage across request/component/feature specs.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
spec/requests/work_packages/moves_spec.rb	Adds request coverage for preserving form selections and handling malformed custom field params during refresh.
spec/models/permitted_params_spec.rb	Adds specs for `move_work_package` normalization and new `move_work_package_form_values` permitting/guarding.
spec/features/work_packages/bulk/move_work_package_spec.rb	Adds JS feature spec ensuring form values persist after a project-change-triggered refresh.
spec/components/work_packages/moves/form_component_spec.rb	Adds component specs for selected values preservation and warning rendering behavior.
app/views/work_packages/moves/new.html.erb	Switches to rendering a pre-built controller-provided component instance.
app/models/permitted_params.rb	Introduces `move_work_package_form_values` and tightens custom field value handling against malformed shapes.
app/helpers/custom_fields_helper.rb	Extends bulk-edit custom field rendering helper to accept a preselected value and apply it to inputs.
app/controllers/work_packages/moves_controller.rb	Builds and reuses the move form component with `selected_values`, `turbo_stream_url`, and `current_user`.
app/components/work_packages/moves/form_component.rb	Computes types/versions/statuses internally and introduces warning logic for unavailable types in target project.
app/components/work_packages/moves/form_component.html.erb	Applies selected values to selects/date pickers/custom fields and uses the new warning predicate.

Keeps the submitted move-form field values when the Turbo Stream refresh rebuilds the form after project or type changes. Passes the refresh request values into the streamable form component for rendering only, while leaving the persisted move/copy attributes on the existing permitted params path. https://community.openproject.org/wp/OP-19606

Derives the type list, selected target type, versions, statuses, and unavailable-type warning inside FormComponent from its inputs instead of precomputing render-only state in the controller. Passes `current_user` explicitly so status availability stays a real dependency, and avoids recomputing form-only collections on create paths where the form is never rendered. Use an indexed `find_by(id:)` lookup for the selected target project instead of loading the whole `allowed_target_projects_on_move` relation only to detect one project. Scopes required custom-field options to the target project while rendering the refreshed move form.

Add `PermittedParams#move_work_package_form_values` for form refresh rendering so the move controller does not maintain the duplicate `MOVE_FORM_VALUE_PARAMS` allow-list. Reuse it from `#move_work_package`, then merge the submit path keys normalized from `new_project_id`, `new_type_id`, and `notes` for persistence. Cover both move submit params and refresh-form params, including custom field value filtering.

Treat array-shaped `custom_field_values` as absent before the shared custom field helper filters permitted values. This avoids `Array#each_value` failures in both move submit params and move form refresh params while keeping hash-shaped `custom_field_values` unchanged.

The unavailable-type check compared the selected new type against the target project's types, but that type is always chosen from them, so the branch was dead. It now checks the moved work packages' current types, matching the warning copy. Also fixes `ancestor_types_missing_in_target?`, which called `work_packages.select(:id)` as a query subquery; that only works when `work_packages` is an `ActiveRecord::Relation`. The predicate now runs this branch unconditionally, so specs that pass a plain array (this file's convention) surfaced the crash. Switched to `.map(&:id)` to match the sibling method and work with either.

`selected_value` fell back between symbol and string keys to bridge test hashes and controller `Parameters`. Converting `@selected_values` to indifferent access once lets the lookup use a single key.

The unavailable-type check looked at the ancestor side of each hierarchy row, i.e. the moved work packages' own types, so any child carried along with its parent never triggered the warning.

The notes value only reaches the refresh request via the CKEditor flush, then must survive re-rendering into a fresh editor instance; no end-to-end spec exercised that path.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

github-actions · 2026-07-02T11:58:09Z

Warning

Flaky specs

rspec ./spec/features/work_packages/share/multi_invite_spec.rb[1:1:2:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #24062, linked for reference only):

- `rspec ./spec/features/work_packages/share/multi_invite_spec.rb[1:1:2:1]`

Treat this as a standalone task, unrelated to PR #24062. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24062 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

EinLama

Found only a handful of smaller issues. The rest looks good to me! 😄 I agree that it's worth updating the translation to make the error message clearer.

dombesz

Looks good, just left a few minor comments on code cleanup.

Clarifies that the method returns the target project's types, not the source project's, and aligns the name with the sibling readers `available_versions` and `available_statuses`.

Normalizes `new_type_id`/`new_project_id` inside `move_work_package_form_values` so the controller no longer passes the type separately and the component reads it from `selected_values`.

The refresh endpoint is a constant collection route, so the component derives it via `url_helpers` instead of receiving it from the controller.

The status options were derived from the user's roles in the source project, although the chosen status is applied in the target project. Drive-by fix of pre-existing behavior, surfaced during review.

The warning also fires when only a descendant's type is missing in the target project, so the copy now covers both cases and points at enabling the missing types.

myabc · 2026-07-02T18:40:55Z

@EinLama @dombesz many thanks for the great feedback! I think I've addressed it all now.

EinLama

Looks great, thanks for addressing my points 👏

dombesz

Looks good, thanks Alex!

Copilot AI review requested due to automatic review settings July 2, 2026 10:31

myabc mentioned this pull request Jul 2, 2026

[SC-272] Fix RefreshOnFormChangesController user data leak #23938

Open

2 tasks

Copilot started reviewing on behalf of myabc July 2, 2026 10:32 View session

myabc added needs review ruby Pull requests that update Ruby code bugfix labels Jul 2, 2026

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread app/components/work_packages/moves/form_component.rb Outdated

myabc added 6 commits July 2, 2026 12:02

Normalize selected values to indifferent access

b0359da

`selected_value` fell back between symbol and string keys to bridge test hashes and controller `Parameters`. Converting `@selected_values` to indifferent access once lets the lookup use a single key.

myabc mentioned this pull request Jul 2, 2026

[OP-19677] Permit budget on work package move #24061

Merged

2 tasks

myabc force-pushed the bug/OP-19606-preserve-move-form-values-on-refresh branch from 2221d90 to b0359da Compare July 2, 2026 11:03

myabc force-pushed the bug/SC-272-refresh-on-form-changes-controller-data-leak branch from 480c637 to b731557 Compare July 2, 2026 11:03

myabc added this to the 17.6.x milestone Jul 2, 2026

Warn when descendant type is missing in target

d717781

The unavailable-type check looked at the ancestor side of each hierarchy row, i.e. the moved work packages' own types, so any child carried along with its parent never triggered the warning.

myabc requested review from EinLama and Copilot July 2, 2026 11:24

Copilot started reviewing on behalf of myabc July 2, 2026 11:30 View session

Cover notes preservation on move form refresh

ca208d6

The notes value only reaches the refresh request via the CKEditor flush, then must survive re-rendering into a fresh editor instance; no end-to-end spec exercised that path.

Copilot AI reviewed Jul 2, 2026

View reviewed changes

EinLama requested changes Jul 2, 2026

View reviewed changes

dombesz requested changes Jul 2, 2026

View reviewed changes

Comment thread app/controllers/work_packages/moves_controller.rb Outdated

Comment thread app/models/permitted_params.rb

Comment thread app/controllers/work_packages/moves_controller.rb Outdated

myabc added 4 commits July 2, 2026 19:14

Rename types to available_types in move form

63b95a6

Clarifies that the method returns the target project's types, not the source project's, and aligns the name with the sibling readers `available_versions` and `available_statuses`.

Merge move target params into form values

7c21dc4

Normalizes `new_type_id`/`new_project_id` inside `move_work_package_form_values` so the controller no longer passes the type separately and the component reads it from `selected_values`.

Compute refresh URL inside move form component

ad5cefb

The refresh endpoint is a constant collection route, so the component derives it via `url_helpers` instead of receiving it from the controller.

Offer target project statuses on move form

e459f22

The status options were derived from the user's roles in the source project, although the chosen status is applied in the target project. Drive-by fix of pre-existing behavior, surfaced during review.

Mention descendants in unavailable-type warning

1a05c1b

The warning also fires when only a descendant's type is missing in the target project, so the copy now covers both cases and points at enabling the missing types.

myabc requested review from EinLama and dombesz July 2, 2026 18:31

EinLama approved these changes Jul 3, 2026

View reviewed changes

dombesz approved these changes Jul 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[OP-19606] Preserve move form values on refresh#24062

[OP-19606] Preserve move form values on refresh#24062
myabc wants to merge 13 commits into
bug/SC-272-refresh-on-form-changes-controller-data-leakfrom
bug/OP-19606-preserve-move-form-values-on-refresh

myabc commented Jul 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

EinLama left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dombesz left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

myabc commented Jul 2, 2026

Uh oh!

EinLama left a comment

Uh oh!

dombesz left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

myabc commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Screen recording

What approach did you choose and why?

Review follow-ups

Stacked PRs (split per work package)

Merge checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

EinLama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dombesz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

myabc commented Jul 2, 2026

Uh oh!

EinLama left a comment

Choose a reason for hiding this comment

Uh oh!

dombesz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

myabc commented Jul 2, 2026 •

edited

Loading