Skip to content

fix editing wp in split view after moving #24077

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.

Sign up for GitHub

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

Merged
ulferts merged 3 commits into from
Jul 3, 2026
Merged

fix editing wp in split view after moving #24077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad0c3b3
fix editing wp in split view after moving
ulferts Jul 2, 2026
536df6f
change to a stimulus global action
ulferts Jul 3, 2026
bbfabe6
reload the wp only if it has ever been loaded
ulferts Jul 3, 2026
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
71 changes: 69 additions & 2 deletions frontend/src/stimulus/controllers/dynamic/backlogs.controller.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,17 @@ describe('Backlogs controller', () => {
let events$:Subject<HalEvent[]>;
let aggregated$:Mock;
let reload:Mock;
let refresh:Mock;
let id:Mock;
let hasValue:Mock;
let state:Mock;
let originalOpenProject:typeof window.OpenProject;

const pluginContext = () => ({
services: { halEvents: { aggregated$ } },
services: {
halEvents: { aggregated$ },
apiV3Service: { work_packages: { id, cache: { state } } },
},
});

beforeAll(async () => {
Expand All @@ -57,6 +64,10 @@ describe('Backlogs controller', () => {
events$ = new Subject<HalEvent[]>();
aggregated$ = vi.fn(() => events$);
reload = vi.fn(() => Promise.resolve());
refresh = vi.fn(() => Promise.resolve());
id = vi.fn(() => ({ refresh }));
hasValue = vi.fn(() => true);
state = vi.fn(() => ({ hasValue }));
originalOpenProject = window.OpenProject;
window.OpenProject = {
getPluginContext: () => Promise.resolve(pluginContext()),
Expand All @@ -75,7 +86,8 @@ describe('Backlogs controller', () => {

async function renderBacklogs() {
await ctx.mount(`
<div data-controller="backlogs">
<div data-controller="backlogs"
data-action="op-dispatched:backlogs:work-package-moved@document->backlogs#onWorkPackageMoved">
<turbo-frame id="backlogs_container"></turbo-frame>
</div>
`);
Expand Down Expand Up @@ -127,6 +139,61 @@ describe('Backlogs controller', () => {
expect(reload).not.toHaveBeenCalled();
});

it('refreshes the moved work package cache when it is loaded', async () => {
await renderBacklogs();
await waitFor(() => { expect(aggregated$).toHaveBeenCalled(); });

document.dispatchEvent(new CustomEvent('op-dispatched:backlogs:work-package-moved', {
detail: { work_package_id: 42 },
}));

await waitFor(() => {
expect(state).toHaveBeenCalledWith('42');
expect(id).toHaveBeenCalledWith('42');
expect(refresh).toHaveBeenCalled();
});
});

it('does not refresh when the moved work package is not loaded', async () => {
hasValue.mockReturnValue(false);
await renderBacklogs();
await waitFor(() => { expect(aggregated$).toHaveBeenCalled(); });

document.dispatchEvent(new CustomEvent('op-dispatched:backlogs:work-package-moved', {
detail: { work_package_id: 42 },
}));
await ctx.nextFrame();

expect(state).toHaveBeenCalledWith('42');
expect(refresh).not.toHaveBeenCalled();
});

it('ignores a work-package-moved event without a work package id', async () => {
await renderBacklogs();
await waitFor(() => { expect(aggregated$).toHaveBeenCalled(); });

document.dispatchEvent(new CustomEvent('op-dispatched:backlogs:work-package-moved', { detail: {} }));
await ctx.nextFrame();

expect(refresh).not.toHaveBeenCalled();
});

it('stops refreshing once disconnected', async () => {
await renderBacklogs();
await waitFor(() => { expect(aggregated$).toHaveBeenCalled(); });

const root = ctx.container.querySelector('[data-controller="backlogs"]')!;
root.remove();
await ctx.nextFrame();

document.dispatchEvent(new CustomEvent('op-dispatched:backlogs:work-package-moved', {
detail: { work_package_id: 42 },
}));
await ctx.nextFrame();

expect(refresh).not.toHaveBeenCalled();
});

it('does not subscribe when disconnected before the context resolves', async () => {
let resolveContext!:(context:unknown) => void;
window.OpenProject = {
Expand Down
27 changes: 26 additions & 1 deletion frontend/src/stimulus/controllers/dynamic/backlogs.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,16 @@

import { Controller } from '@hotwired/stimulus';
import { FrameElement } from '@hotwired/turbo';
import type { ApiV3Service } from 'core-app/core/apiv3/api-v3.service';
import { HalEventsService } from 'core-app/features/hal/services/hal-events.service';
import { filter, Subscription } from 'rxjs';

import { useAngularServices, type ServiceKey } from 'core-stimulus/mixins/use-angular-services';

export default class BacklogsController extends Controller<HTMLElement> {
static services:ServiceKey[] = ['halEvents'];
static services:ServiceKey[] = ['halEvents', 'apiV3Service'];
declare halEvents:HalEventsService;
declare apiV3Service:ApiV3Service;

private subscription:Subscription|null = null;

Expand All @@ -54,6 +56,29 @@ export default class BacklogsController extends Controller<HTMLElement> {
this.subscription = null;
}

// Bound to the `op-dispatched:backlogs:work-package-moved` document event.
// Refreshes a work package already in the cache. The most likely reason for this is that the
// work package is or has been displayed in the split view. Refreshing prevents
// the lock version becoming stale after move and also reflects the change to the sprint and
// bucket property.
// The method currently does not check if the work package is still open in the split view. If it
// was ever opened, it will still be in the cache leading to a refresh request. The upside of this potentially
// wasteful refresh is that when the work package is later on reopened in the split view, its information
// is correct as well.
onWorkPackageMoved(event:CustomEvent<{ work_package_id?:number }>):void {
const workPackageId = event.detail?.work_package_id;
// apiV3Service is wired asynchronously via useAngularServices, so it may be absent
// if the event somehow fires before the services resolve.
if (workPackageId === undefined || !this.apiV3Service) { return; }

const id = workPackageId.toString();
const { work_packages: workPackages } = this.apiV3Service;

if (workPackages.cache.state(id).hasValue()) {
void workPackages.id(id).refresh();
}
}

private refreshList() {
void this.listElement.reload();
}
Expand Down
12 changes: 12 additions & 0 deletions modules/backlogs/app/controllers/backlogs/work_packages_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ class WorkPackagesController < BaseController
include OpTurbo::ComponentStream
include Backlogs::Concerns::ContainerLoading

# Document event dispatched after a successful move so the frontend can refresh a
# split view open on the moved work package (see backlogs.controller.ts).
WORK_PACKAGE_MOVED_EVENT = "op-dispatched:backlogs:work-package-moved"

before_action :load_work_package

# Deferred ActionMenu items (Primer include-fragment).
Expand Down Expand Up @@ -73,6 +77,14 @@ def move # rubocop:disable Metrics/AbcSize
if call.success?
move_work_package_to_target_component_via_turbo_stream(source_sprint:, target_sprint: call.result.sprint)

# A split view open on the moved work package caches its lock_version. Signal the
# move so the frontend can refresh that cache and avoid a stale-lock_version conflict
# on the next edit. Covers both drag-and-drop and the move-to-sprint/bucket dialogs.
dispatch_event_via_turbo_stream(
WORK_PACKAGE_MOVED_EVENT,
detail: { work_package_id: call.result.id }
)

if work_package_invisible_after_move?(call.result)
backlog_name = call.result.backlog_bucket&.name || I18n.t(:label_inbox)
render_flash_message_via_turbo_stream(
Expand Down
1 change: 1 addition & 0 deletions modules/backlogs/app/views/backlogs/backlog/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ See COPYRIGHT and LICENSE files for more details.
<% html_title t(:label_backlogs) %>

<% content_controller "backlogs",
action: "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs#onWorkPackageMoved",
"backlogs-list-url-value": project_backlogs_backlog_path(@project) %>

<% content_for :content_header do %>
Expand Down
99 changes: 99 additions & 0 deletions modules/backlogs/spec/features/work_packages/edit_in_split_view_after_move_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

require "spec_helper"
require_relative "../../support/pages/backlog"

# Regression spec: when a work package is moved to a different container while it is
# open in the split view, the split view still holds the lock_version it fetched on
# opening. Editing then failed with a conflicting-modifications error. The move now
# signals the frontend to refresh the cached work package so editing keeps working.
RSpec.describe "Editing a work package in the split view after moving it",
:js do
let(:project) { create(:project) }
let(:user) do
create(:user,
member_with_permissions: {
project => %i[view_sprints manage_sprint_items
view_work_packages edit_work_packages]
})
end
let(:backlogs_page) { Pages::Backlog.new(project) }

let!(:sprint) do
create(:sprint, project:)
end
let!(:target_sprint) do
create(:sprint, project:)
end
let!(:work_package) { create(:work_package, project:, sprint:) }

current_user { user }

shared_examples "editing works after the move" do
it "updates the work package without a conflict error" do
backlogs_page.visit!

split_view = backlogs_page.open_work_package_details(work_package)

move_work_package(target_sprint)

backlogs_page.expect_work_package_not_in_sprint(work_package, sprint)
backlogs_page.expect_work_package_in_sprint(work_package, target_sprint)
split_view.expect_attributes sprint: target_sprint

split_view.edit_field(:subject).update("Updated after move")

backlogs_page.expect_and_dismiss_toaster(message: "Successful update")
expect(work_package.reload.subject).to eq("Updated after move")
Comment on lines +72 to +75
end
end

context "when moving the work package via the work package menu" do
def move_work_package(target)
backlogs_page.click_in_work_package_menu(work_package, "Move to sprint")

within_modal "Move to sprint" do
select target.name, from: "target_id"
click_on "Move"
end
end

it_behaves_like "editing works after the move"
end

context "when moving the work package by dragging it with the mouse" do
def move_work_package(target)
backlogs_page.drag_work_package(work_package, into: target)
end

it_behaves_like "editing works after the move"
end
end
Loading