Merge pull request #7583 from opf/fix/30817/dont-apply-filter-when-matching-value

[30817] Don't apply the filter when current value is already valid
5 years ago · 9a275f9bea
parent 9c45cca84a f91fcc51cb
commit 9a275f9bea
7 changed files with 400 additions and 5 deletions
--- a/frontend/src/app/components/wp-edit-form/work-package-filter-values.spec.ts
+++ b/frontend/src/app/components/wp-edit-form/work-package-filter-values.spec.ts
@ -0,0 +1,179 @@
+// -- copyright
+// OpenProject is a project management system.
+// Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+//
+// 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 doc/COPYRIGHT.rdoc for more details.
+// ++
+
+import {TestBed} from "@angular/core/testing";
+import {CurrentUserService} from "core-components/user/current-user.service";
+import {HalResourceService} from "core-app/modules/hal/services/hal-resource.service";
+import {Injector} from "@angular/core";
+import {WorkPackageCacheService} from "core-components/work-packages/work-package-cache.service";
+import {SchemaCacheService} from "core-components/schemas/schema-cache.service";
+import {WorkPackageChangeset} from "core-components/wp-edit-form/work-package-changeset";
+import {WorkPackageFilterValues} from "core-components/wp-edit-form/work-package-filter-values";
+import {WorkPackageNotificationService} from "core-components/wp-edit/wp-notification.service";
+import {IWorkPackageCreateServiceToken} from "core-components/wp-new/wp-create.service.interface";
+import {IWorkPackageEditingServiceToken} from "core-components/wp-edit-form/work-package-editing.service.interface";
+import {WorkPackagesActivityService} from "core-components/wp-single-view-tabs/activity-panel/wp-activity.service";
+import {WorkPackageCreateService} from "core-components/wp-new/wp-create.service";
+import {WorkPackageEditingService} from "core-components/wp-edit-form/work-package-editing-service";
+import {WorkPackageResource} from "core-app/modules/hal/resources/work-package-resource";
+import {TypeResource} from "core-app/modules/hal/resources/type-resource";
+import {HttpClientModule} from "@angular/common/http";
+import {States} from "core-components/states.service";
+import {I18nService} from "core-app/modules/common/i18n/i18n.service";
+import {NotificationsService} from "core-app/modules/common/notifications/notifications.service";
+import {ConfigurationService} from "core-app/modules/common/config/configuration.service";
+import {PathHelperService} from "core-app/modules/common/path-helper/path-helper.service";
+import {UIRouterModule} from "@uirouter/angular";
+import {WorkPackageDmService} from "core-app/modules/hal/dm-services/work-package-dm.service";
+import {LoadingIndicatorService} from "core-app/modules/common/loading-indicator/loading-indicator.service";
+import {OpenProjectFileUploadService} from "core-components/api/op-file-upload/op-file-upload.service";
+import {HookService} from "core-app/modules/plugins/hook-service";
+import {IsolatedQuerySpace} from "core-app/modules/work_packages/query-space/isolated-query-space";
+import {WorkPackageEventsService} from "core-app/modules/work_packages/events/work-package-events.service";
+import {TimezoneService} from "core-components/datetime/timezone.service";
+
+describe('WorkPackageFilterValues', () => {
+  let resource:WorkPackageResource;
+  let injector:Injector;
+  let halResourceService:HalResourceService;
+
+  let changeset:WorkPackageChangeset;
+  let subject:WorkPackageFilterValues;
+  let filters:any[];
+  let source:any;
+
+  function setupTestBed() {
+    // noinspection JSIgnoredPromiseFromCall
+    TestBed.configureTestingModule({
+      imports: [
+        UIRouterModule.forRoot({}),
+        HttpClientModule
+      ],
+      providers: [
+        I18nService,
+        States,
+        IsolatedQuerySpace,
+        WorkPackageEventsService,
+        TimezoneService,
+        PathHelperService,
+        ConfigurationService,
+        CurrentUserService,
+        HookService,
+        OpenProjectFileUploadService,
+        LoadingIndicatorService,
+        WorkPackageDmService,
+        HalResourceService,
+        NotificationsService,
+        WorkPackageNotificationService,
+        SchemaCacheService,
+        WorkPackageCacheService,
+        { provide: IWorkPackageCreateServiceToken, useClass: WorkPackageCreateService },
+        { provide: IWorkPackageEditingServiceToken, useClass: WorkPackageEditingService },
+        WorkPackagesActivityService,
+      ]
+    }).compileComponents();
+
+    injector = TestBed.get(Injector);
+    halResourceService = injector.get(HalResourceService);
+
+    resource = halResourceService.createHalResourceOfClass(WorkPackageResource, source, true);
+    changeset = new WorkPackageChangeset(injector, resource);
+
+    let type1 = halResourceService.createHalResourceOfClass(
+      TypeResource,
+      { _type: 'Type', id: '1', _links: { self: { href: '/api/v3/types/1', name: 'Task' } } }
+    );
+    let type2 = halResourceService.createHalResourceOfClass(
+      TypeResource,
+      { _type: 'Type', id: '2', _links: { self: { href: '/api/v3/types/2', name: 'Bug' } } }
+    );
+
+    filters = [
+      {
+        id: 'type',
+        operator: { id: '=' },
+        values: [type1, type2]
+      }
+    ];
+
+    subject = new WorkPackageFilterValues(injector, changeset, filters);
+  }
+
+  describe('when a filter value already exists in values', () => {
+    describe('with the first type applied', () => {
+      beforeEach(() => {
+        source = {
+          _type: 'WorkPackage',
+          id: '1234',
+          _links: {
+            type: {
+              href: '/api/v3/types/1',
+              name: 'Task'
+            }
+          }
+        };
+
+        setupTestBed();
+      });
+
+      it('it should not apply the first value (Regression #30817)', (() => {
+        subject.applyDefaultsFromFilters();
+
+        expect(changeset.changedAttributes.length).toEqual(0);
+        expect(changeset.value('type').href).toEqual('/api/v3/types/1');
+      }));
+    });
+
+    describe('with the second type applied', () => {
+      beforeEach(() => {
+        source = {
+          id: '1234',
+          _links: {
+            type: {
+              href: '/api/v3/types/2',
+              name: 'Bug'
+            }
+          }
+        };
+
+        setupTestBed();
+      });
+
+      it('it should not apply the first value (Regression #30817)', (() => {
+        subject.applyDefaultsFromFilters();
+
+        expect(changeset.changedAttributes.length).toEqual(0);
+        expect(changeset.value('type').href).toEqual('/api/v3/types/2');
+      }));
+    })
+
+  });
+});
+
+
+
--- a/frontend/src/app/components/wp-edit-form/work-package-filter-values.ts
+++ b/frontend/src/app/components/wp-edit-form/work-package-filter-values.ts
@ -4,6 +4,8 @@ import {QueryFilterInstanceResource} from 'core-app/modules/hal/resources/query-
 import {CurrentUserService} from "core-components/user/current-user.service";
 import {HalResourceService} from 'core-app/modules/hal/services/hal-resource.service';
 import {Injector} from '@angular/core';
+import {AngularTrackingHelpers} from "core-components/angular/tracking-functions";
+import compareByHrefOrString = AngularTrackingHelpers.compareByHrefOrString;

 export class WorkPackageFilterValues {

@ -29,6 +31,12 @@ export class WorkPackageFilterValues {
        return;
      }

+      // Avoid setting a value if current value is in filter list
+      // and more than one value selected
+      if (this.filterAlreadyApplied(filter)) {
+        return;
+      }
+
      // Select the first value
      let value = filter.values[0];

@ -64,4 +72,27 @@ export class WorkPackageFilterValues {

    return undefined;
  }
+
+  /**
+   * Avoid applying filter values when
+   *  - more than one filter value selected
+   *  - changeset already matches one of the selected values
+   * @param filter
+   */
+  private filterAlreadyApplied(filter:any):boolean {
+    // Only applicable if more than one selected
+    if (filter.values.length <= 1) {
+      return false;
+    }
+
+    const current = this.changeset.value(filter.id);
+
+    for (let i = 0; i < filter.values.length; i++) {
+      if (compareByHrefOrString(current, filter.values[i])) {
+        return true;
+      }
+    }
+
+    return false;
+  }
 }
--- a/frontend/src/app/modules/common/drag-and-drop/drag-and-drop.service.ts
+++ b/frontend/src/app/modules/common/drag-and-drop/drag-and-drop.service.ts
@ -163,7 +163,7 @@ export class DragAndDropService implements OnDestroy {
      try {
        await this.handleDrop(el, target, source, sibling);
      } catch (e) {
-        console.error("Failed to handle drop of %O", el);
+        console.error("Failed to handle drop of %O, %O", el, e);
      }
    });

--- a/modules/boards/spec/features/action_boards/status_type_moving_board_spec.rb
+++ b/modules/boards/spec/features/action_boards/status_type_moving_board_spec.rb
@ -0,0 +1,164 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2018 the OpenProject Foundation (OPF)
+#
+# 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-2017 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 docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+require_relative './../support//board_index_page'
+require_relative './../support/board_page'
+
+describe 'Status action board', type: :feature, js: true do
+  let(:user) do
+    FactoryBot.create(:user,
+                      member_in_project: project,
+                      member_through_role: role)
+  end
+  let(:permissions) {
+    %i[show_board_views manage_board_views add_work_packages
+       edit_work_packages view_work_packages manage_public_queries]
+  }
+  let(:role) { FactoryBot.create(:role, permissions: permissions) }
+
+
+  let(:type_bug) { FactoryBot.create(:type_bug) }
+  let(:type_task) { FactoryBot.create(:type_task) }
+
+  let(:project) { FactoryBot.create(:project, types: [type_task, type_bug], enabled_module_names: %i[work_package_tracking board_view]) }
+  let(:board_index) { Pages::BoardIndex.new(project) }
+
+  let!(:priority) { FactoryBot.create :default_priority }
+  let!(:open_status) { FactoryBot.create :default_status, name: 'Open' }
+  let!(:closed_status) { FactoryBot.create :status, is_closed: true, name: 'Closed' }
+
+  let(:task_wp) do
+    FactoryBot.create :work_package,
+                      project: project,
+                      type: type_task,
+                      subject: 'Open task item',
+                      status: open_status
+  end
+  let(:bug_wp) do
+    FactoryBot.create :work_package,
+                      project: project,
+                      type: type_bug,
+                      subject: 'Closed bug item',
+                      status: closed_status
+  end
+
+  let!(:workflow_task) {
+    FactoryBot.create(:workflow,
+                      type: type_task,
+                      role: role,
+                      old_status_id: open_status.id,
+                      new_status_id: closed_status.id)
+  }
+  let!(:workflow_task_back) {
+    FactoryBot.create(:workflow,
+                      type: type_task,
+                      role: role,
+                      old_status_id: closed_status.id,
+                      new_status_id: open_status.id)
+  }
+
+  let!(:workflow_bug) {
+    FactoryBot.create(:workflow,
+                      type: type_bug,
+                      role: role,
+                      old_status_id: open_status.id,
+                      new_status_id: closed_status.id)
+  }
+  let!(:workflow_bug_back) {
+    FactoryBot.create(:workflow,
+                      type: type_bug,
+                      role: role,
+                      old_status_id: closed_status.id,
+                      new_status_id: open_status.id)
+  }
+
+
+  let(:filters) { ::Components::WorkPackages::Filters.new }
+
+  before do
+    with_enterprise_token :board_view
+    task_wp
+    bug_wp
+    login_as(user)
+  end
+
+  it 'allows moving of types between lists without changing filters (Regression #30817)' do
+    board_index.visit!
+
+    # Create new board
+    board_page = board_index.create_board action: :Status
+
+    # expect lists of default status
+    board_page.expect_list 'Open'
+
+    board_page.add_list option: 'Closed'
+    board_page.expect_list 'Closed'
+
+    filters.expect_filter_count 0
+    filters.open
+
+    filters.add_filter_by('Type', 'is', [type_task.name, type_bug.name])
+    filters.expect_filter_by('Type', 'is', [type_task.name, type_bug.name])
+
+    # Wait a bit before saving the page to ensure both values are processed
+    sleep 2
+
+    board_page.expect_changed
+    board_page.save
+
+    # Move task to closed
+    board_page.move_card(0, from: 'Open', to: 'Closed')
+    board_page.expect_card('Open', 'Open task item', present: false)
+    board_page.expect_card('Closed', 'Open task item', present: true)
+
+    # Expect type unchanged
+    board_page.card_for(task_wp).expect_type 'Task'
+    board_page.card_for(bug_wp).expect_type 'Bug'
+
+    # Wait a bit before moving the items too fast
+    sleep 2
+
+    # Move bug to open
+    board_page.move_card(0, from: 'Closed', to: 'Open')
+    board_page.expect_card('Closed', 'Closed bug item', present: false)
+    board_page.expect_card('Open', 'Closed bug item', present: true)
+
+    # Expect type unchanged
+    board_page.card_for(task_wp).expect_type 'Task'
+    board_page.card_for(bug_wp).expect_type 'Bug'
+
+    sleep 2
+
+    task_wp.reload
+    bug_wp.reload
+
+    expect(task_wp.type).to eq(type_task)
+    expect(bug_wp.type).to eq(type_bug)
+  end
+end
--- a/modules/boards/spec/features/support/board_page.rb
+++ b/modules/boards/spec/features/support/board_page.rb
@ -220,6 +220,10 @@ module Pages
      page.find('.dropdown-menu a', text: action).click
    end

+    def card_for(work_package)
+      ::Pages::WorkPackageCard.new work_package
+    end
+
    def expect_list_option(name, present: true)
      open_and_fill_add_list_modal name

--- a/spec/support/components/work_packages/filters.rb
+++ b/spec/support/components/work_packages/filters.rb
@ -144,9 +144,11 @@ module Components
      def set_value(id, value)
        retry_block do
          if page.has_selector?("#filter_#{id} .ng-select-container")
-            select_autocomplete page.find("#filter_#{id}"),
-                                query: value,
-                                results_selector: '.advanced-filters--ng-select .ng-dropdown-panel-items'
+            Array(value).each do |val|
+              select_autocomplete page.find("#filter_#{id}"),
+                                  query: val,
+                                  results_selector: '.advanced-filters--ng-select .ng-dropdown-panel-items'
+            end
          else
            within_values(id) do
              page.all('input').each_with_index do |input, index|
--- a/spec/support/pages/work_packages/work_package_card.rb
+++ b/spec/support/pages/work_packages/work_package_card.rb
@ -34,8 +34,23 @@ module Pages

    def initialize(work_package, project = nil)
      @work_package = work_package
-      @project = project
+      @project = project || work_package.project
    end

+    def card_element
+      page.find(".wp-card-#{work_package.id}")
+    end
+
+    def expect_type(name)
+      page.within(card_element) do
+        expect(page).to have_selector('.wp-card--type', text: name.upcase)
+      end
+    end
+
+    def expect_subject(subject)
+      page.within(card_element) do
+        expect(page).to have_selector('.wp-card--subject', text: subject)
+      end
+    end
  end
 end