[26778] Pass empty groupBy to query params

https://community.openproject.com/wp/26778

app/services/api/v3/parse_query_params_service.rb

@@ -31,8 +31,9 @@ module API
     class ParseQueryParamsService
       def call(params)
         parsed_params = {}
+        skip_empty = []
 
-        parsed_params[:group_by] = group_by_from_params(params)
+        parsed_params[:group_by] = group_by_from_params(params, skip_empty)
 
         error_result = with_service_error_on_json_parse_error do
           parsed_params[:filters] = filters_from_params(params)
@@ -53,12 +54,21 @@ module API
 
         parsed_params[:show_hierarchies] = boolearize(params[:showHierarchies])
 
-        ServiceResult.new(success: true,
-                          result: without_empty(parsed_params, params.keys))
+        allow_empty = params.keys + skip_empty
+        ServiceResult.new(success: true, result: without_empty(parsed_params, allow_empty))
       end
 
-      def group_by_from_params(params)
-        convert_attribute(params[:group_by] || params[:groupBy] || params[:g])
+      def group_by_from_params(params, skip_empty)
+        return nil unless params_exist?(params, %i(group_by groupBy g))
+
+        attribute = params[:group_by] || params[:groupBy] || params[:g]
+        if attribute.present?
+          convert_attribute attribute
+        else
+          # Group by explicitly set to empty
+          skip_empty << :group_by
+          nil
+        end
       end
 
       def sort_by_from_params(params)
@@ -175,6 +185,10 @@ module API
         return result
       end
 
+      def params_exist?(params, symbols)
+        params.detect { |k, _| symbols.include? k.to_sym }
+      end
+
       def without_empty(hash, exceptions)
         exceptions = exceptions.map(&:to_sym)
         hash.select { |k, v| v.present? || v == false || exceptions.include?(k) }

app/services/update_query_from_params_service.rb

@@ -60,7 +60,7 @@ class UpdateQueryFromParamsService
   private
 
   def apply_group_by(params)
-    query.group_by = params[:group_by] if params[:group_by]
+    query.group_by = params[:group_by] if params.key?(:group_by)
   end
 
   def apply_sort_by(params)

frontend/app/components/wp-query/url-params-helper.test.ts

@@ -261,6 +261,7 @@ describe('UrlParamsHelper', function() {
         filters: [filter1],
         sortBy: [],
         columns: [],
+        groupBy: '',
         sums: false
       };
 
@@ -278,6 +279,7 @@ describe('UrlParamsHelper', function() {
             }
           }
         ]),
+        groupBy: '',
         showSums: false,
         sortBy: '[]'
       };

frontend/app/components/wp-query/url-params-helper.ts

@@ -77,10 +77,7 @@ export class UrlParamsHelperService {
 
     paramsData.tzl = query.timelineZoomLevel;
     paramsData.hi = !!query.showHierarchies;
-
-    if (query.groupBy) {
-      paramsData.g = query.groupBy.id;
-    }
+    paramsData.g = _.get(query.groupBy, 'id', '');
     if (query.sortBy) {
       paramsData.t = query
         .sortBy
@@ -145,9 +142,7 @@ export class UrlParamsHelperService {
       queryData.showHierarchies = properties.hi;
     }
 
-    if (properties.g) {
-      queryData.groupBy = properties.g;
-    }
+    queryData.groupBy = _.get(properties, 'g', '');
 
     // Filters
     if (properties.f) {
@@ -195,10 +190,7 @@ export class UrlParamsHelperService {
     queryData.timelineVisible = query.timelineVisible;
     queryData.timelineZoomLevel = query.timelineZoomLevel;
     queryData.showHierarchies = query.showHierarchies;
-
-    if (query.groupBy) {
-      queryData.groupBy = query.groupBy.id;
-    }
+    queryData.groupBy = _.get(query.groupBy, 'id', '');
 
     // Filters
     const filters = query.filters.map((filter:any) => {

spec/features/work_packages/table/group_by/group_by_progress_spec.rb

@@ -11,6 +11,7 @@ describe 'Work Package group by progress', js: true do
   let!(:wp_4) { FactoryGirl.create(:work_package, project: project, done_ratio: 50) }
 
   let(:wp_table) { Pages::WorkPackagesTable.new(project) }
+  let(:group_by) { ::Components::WorkPackages::GroupBy.new }
 
   let!(:query) do
     query              = FactoryGirl.build(:query, user: user, project: project)
@@ -50,4 +51,31 @@ describe 'Work Package group by progress', js: true do
     expect(page).to have_selector('.group--value', text: '10 (2)')
     expect(page).to have_selector('.group--value', text: '50 (2)')
   end
+
+  context 'with grouped query' do
+    let!(:query) do
+      query              = FactoryGirl.build(:query, user: user, project: project)
+      query.column_names = ['subject', 'done_ratio']
+      query.group_by = 'done_ratio'
+
+      query.save!
+      query
+    end
+
+    it 'keeps the disabled group by when reloading (Regression WP#26778)' do
+      # Expect table to be grouped as WP created above
+      expect(page).to have_selector('.group--value .count', count: 3)
+
+      group_by.disable_via_menu
+      expect(page).to have_no_selector('.group--value')
+
+      # Expect disabled group by to be kept after reload
+      page.driver.browser.navigate.refresh
+      expect(page).to have_no_selector('.group--value')
+
+      # But query has not been changed
+      query.reload
+      expect(query.group_by).to eq 'done_ratio'
+    end
+  end
 end

spec/services/api/v3/parse_query_params_service_spec.rb

@@ -71,6 +71,31 @@ describe ::API::V3::ParseQueryParamsService,
         end
       end
 
+      context 'set to empty string' do
+        it_behaves_like 'transforms' do
+          let(:params) { { g: '' } }
+          let(:expected) { { group_by: nil } }
+        end
+
+        it_behaves_like 'transforms' do
+          let(:params) { { group_by: '' } }
+          let(:expected) { { group_by: nil } }
+        end
+
+        it_behaves_like 'transforms' do
+          let(:params) { { groupBy: '' } }
+          let(:expected) { { group_by: nil } }
+        end
+      end
+
+      context 'not given' do
+        let(:params) { { bla: 'foo' } }
+        it 'does not set group_by' do
+          expect(subject).to be_success
+          expect(subject.result).not_to have_key(:group_by)
+        end
+      end
+
       context 'with an attribute called differently in v3' do
         it_behaves_like 'transforms' do
           let(:params) { { groupBy: 'assignee' } }

spec/services/update_query_from_params_service_spec.rb

@@ -52,6 +52,17 @@ describe UpdateQueryFromParamsService,
             .to eql('status')
         end
       end
+
+      context 'for an explicitly nil value' do
+        let(:params) { { group_by: nil } }
+
+        it 'sets the value' do
+          subject
+
+          expect(query.group_by)
+            .to eql(nil)
+        end
+      end
     end
 
     context 'filters' do

spec/support/components/work_packages/group_by.rb

@@ -47,6 +47,10 @@ module Components
         click_button 'Apply'
       end
 
+      def disable_via_menu
+        enable_via_menu '-'
+      end
+
       def expect_not_grouped_by(name)
         open_table_column_context_menu(name)