Merge pull request #4818 from ulferts/fix/remove_unnecessary_query_load

avoid query initialization on html request

app/controllers/work_packages_controller.rb

@@ -51,8 +51,10 @@ class WorkPackagesController < ApplicationController
   # before_filter :disable_api # TODO re-enable once API is used for any JSON request
   before_filter :authorize_on_work_package, only: :show
   before_filter :find_optional_project,
-                :protect_from_unauthorized_export,
-                :load_query, only: :index
+                :protect_from_unauthorized_export, only: :index
+
+  before_filter :load_query,
+                :load_work_packages, only: :index, unless: ->() { request.format.html? }
 
   def show
     respond_to do |format|
@@ -82,8 +84,6 @@ class WorkPackagesController < ApplicationController
   end
 
   def index
-    load_work_packages unless request.format.html?
-
     respond_to do |format|
       format.html do
         gon.settings = client_preferences

config/locales/js-en.yml

@@ -450,6 +450,7 @@ en:
         display_sums: "Display Sums"
         errors:
           unretrievable_query: "Unable to retrieve query from URL"
+          not_found: "There is no such query"
       table:
         summary: "Table with rows of work package and columns of work package attributes."
         text_inline_edit: "Most cells of this table are buttons that activate inline-editing functionality of that attribute."

frontend/app/components/work-packages/work-package.service.js

@@ -88,9 +88,15 @@ function WorkPackageService($http,
             return response.data;
           },
           function (failedResponse) {
-            NotificationsService.addError(
-                I18n.t('js.work_packages.query.errors.unretrievable_query')
-            );
+            var error = '';
+            if (failedResponse.status === 404) {
+              error = I18n.t('js.work_packages.query.errors.not_found');
+            }
+            else {
+              error = I18n.t('js.work_packages.query.errors.unretrievable_query');
+            }
+
+            NotificationsService.addError(error);
           }
       );
     },

spec/controllers/work_packages_controller_spec.rb

@@ -165,18 +165,6 @@ describe WorkPackagesController, type: :controller do
             expect(response).to render_template('work_packages/index')
           end
         end
-
-        context 'when a query has been previously selected' do
-          let(:query) do
-            FactoryGirl.build_stubbed(:query).tap { |q| q.filters = [Queries::WorkPackages::Filter.new('done_ratio', operator: '>=', values: [10])] }
-          end
-
-          before do allow(session).to receive(:query).and_return query end
-
-          it 'preserves the query' do
-            expect(assigns['query'].filters).to eq(query.filters)
-          end
-        end
       end
 
       describe 'csv' do
@@ -210,24 +198,46 @@ describe WorkPackagesController, type: :controller do
         let(:call_action) { get('index', params.merge(format: 'pdf')) }
 
         requires_export_permission do
-          before do
-            pdf_data = 'pdfdata'
-            mock_pdf = double('pdf export')
-            mock_pdf.stub(:render).and_return(pdf_data)
-
-            expect(WorkPackage::Exporter).to receive(:pdf).and_return(mock_pdf)
+          context 'w/ a valid query' do
+            before do
+              pdf_data = 'pdfdata'
+              mock_pdf = double('pdf export')
+              allow(mock_pdf)
+                .to receive(:render)
+                .and_return(pdf_data)
+
+              expect(WorkPackage::Exporter).to receive(:pdf).and_return(mock_pdf)
+
+              expect(controller).to receive(:send_data).with(pdf_data,
+                                                             type: 'application/pdf',
+                                                             filename: 'export.pdf') do |*_args|
+                # We need to render something because otherwise
+                # the controller will and he will not find a suitable template
+                controller.render text: 'success'
+              end
+            end
 
-            expect(controller).to receive(:send_data).with(pdf_data,
-                                                           type: 'application/pdf',
-                                                           filename: 'export.pdf') do |*_args|
-              # We need to render something because otherwise
-              # the controller will and he will not find a suitable template
-              controller.render text: 'success'
+            it 'should fulfill the defined should_receives' do
+              call_action
             end
           end
 
-          it 'should fulfill the defined should_receives' do
-            call_action
+          context 'with invalid query' do
+            let(:params) { { query_id: 'hokusbogus' } }
+
+            context 'when a non-existant query has been previously selected' do
+              before do
+                allow(controller)
+                  .to receive(:retrieve_query)
+                  .and_raise(ActiveRecord::RecordNotFound)
+
+                call_action
+              end
+
+              it 'renders a 404' do
+                expect(response.response_code).to be === 404
+              end
+            end
           end
         end
       end
@@ -251,21 +261,6 @@ describe WorkPackagesController, type: :controller do
         end
       end
     end
-
-    describe 'with invalid query' do
-      context 'when a non-existant query has been previously selected' do
-        let(:call_action) { get('index', project_id: project.id, query_id: 'hokusbogus') }
-        before do call_action end
-
-        it 'renders a 404' do
-          expect(response.response_code).to be === 404
-        end
-
-        it 'preserves the project' do
-          expect(assigns['project']).to be === project
-        end
-      end
-    end
   end
 
   describe 'index with actual data' do
@@ -329,7 +324,9 @@ describe WorkPackagesController, type: :controller do
       it 'respond with a pdf' do
         pdf_data = 'foobar'
         pdf = double('pdf')
-        pdf.stub(:render).and_return(pdf_data)
+        allow(pdf)
+          .to receive(:render)
+          .and_return(pdf_data)
 
         expected_name = "#{stub_work_package.project.identifier}-#{stub_work_package.id}.pdf"
         expect(WorkPackage::Exporter).to receive(:work_package_to_pdf).and_return(pdf)

spec/features/work_packages/export_spec.rb

@@ -88,6 +88,8 @@ describe 'work package export', type: :feature do
     select 'Progress (%)', from: 'add_filter_select'
     fill_in 'values-percentageDone', with: '25'
 
+    expect(page).not_to have_text(wp_2.description) # safeguard
+
     export!
 
     expect(page).to have_text(wp_1.description)
@@ -99,6 +101,8 @@ describe 'work package export', type: :feature do
     select 'Type', from: 'add_filter_select'
     select wp_3.type.name, from: 'values-type'
 
+    expect(page).not_to have_text(wp_2.description) # safeguard
+
     export!
 
     expect(page).not_to have_text(wp_1.description)