Fix issuable stale object error handler for js when updating tasklistsissue_24815

8 files changed, 111 insertions, 19 deletions

diff --git a/app/assets/javascripts/task_list.js b/app/assets/javascripts/task_list.js
index dfe24d1fb33..b1402c0a880 100644
--- a/app/assets/javascripts/task_list.js
+++ b/app/assets/javascripts/task_list.js
@@ -1,3 +1,4 @@
+/* global Flash */
 require('vendor/task_list');
 
 class TaskList {
@@ -6,6 +7,16 @@ class TaskList {
     this.dataType = options.dataType;
     this.fieldName = options.fieldName;
     this.onSuccess = options.onSuccess || (() => {});
+    this.onError = function showFlash(response) {
+      let errorMessages = '';
+
+      if (response.responseJSON) {
+        errorMessages = response.responseJSON.errors.join(' ');
+      }
+
+      return new Flash(errorMessages || 'Update failed', 'alert');
+    };
+
     this.init();
   }
 
@@ -32,6 +43,7 @@ class TaskList {
       url: $target.data('update-url') || $('form.js-issuable-update').attr('action'),
       data: patchData,
       success: this.onSuccess,
+      error: this.onError,
     });
   }
 }
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 0821974aa93..3ccf2a9ce33 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -26,6 +26,23 @@ module IssuableActions
 
   private
 
+  def render_conflict_response
+    respond_to do |format|
+      format.html do
+        @conflict = true
+        render :edit
+      end
+
+      format.json do
+        render json: {
+          errors: [
+            "Someone edited this #{issuable.human_class_name} at the same time you did. Please refresh your browser and make sure your changes will not unintentionally remove theirs."
+          ]
+        }, status: 409
+      end
+    end
+  end
+
   def labels
     @labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
   end
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index ca5e81100da..1151555b8fa 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -134,8 +134,7 @@ class Projects::IssuesController < Projects::ApplicationController
     end
 
   rescue ActiveRecord::StaleObjectError
-    @conflict = true
-    render :edit
+    render_conflict_response
   end
 
   def referenced_merge_requests
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 365c49a20d4..dda7a92550a 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -296,22 +296,21 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def update
     @merge_request = MergeRequests::UpdateService.new(project, current_user, merge_request_params).execute(@merge_request)
 
-    if @merge_request.valid?
-      respond_to do |format|
-        format.html do
-          redirect_to([@merge_request.target_project.namespace.becomes(Namespace),
-                       @merge_request.target_project, @merge_request])
-        end
-        format.json do
-          render json: @merge_request.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } }, methods: [:task_status, :task_status_short])
+    respond_to do |format|
+      format.html do
+        if @merge_request.valid?
+          redirect_to([@merge_request.target_project.namespace.becomes(Namespace), @merge_request.target_project, @merge_request])
+        else
+          render :edit
         end
       end
-    else
-      render "edit"
+
+      format.json do
+        render json: @merge_request.to_json(include: { milestone: {}, assignee: { methods: :avatar_url }, labels: { methods: :text_color } }, methods: [:task_status, :task_status_short])
+      end
     end
   rescue ActiveRecord::StaleObjectError
-    @conflict = true
-    render :edit
+    render_conflict_response
   end
 
   def remove_wip
diff --git a/changelogs/unreleased/issue_24815.yml b/changelogs/unreleased/issue_24815.yml
new file mode 100644
index 00000000000..916e47d36a9
--- /dev/null
+++ b/changelogs/unreleased/issue_24815.yml
@@ -0,0 +1,4 @@
+---
+title: Fix issuable stale object error handler for js when updating tasklists
+merge_request:
+author:
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 7871b6a9e10..147c3a60c06 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -125,14 +125,16 @@ describe Projects::IssuesController do
   end
 
   describe 'PUT #update' do
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
+
+    it_behaves_like 'update invalid issuable', Issue
+
     context 'when moving issue to another private project' do
       let(:another_project) { create(:empty_project, :private) }
 
-      before do
-        sign_in(user)
-        project.team << [user, :developer]
-      end
-
       context 'when user has access to move issue' do
         before { another_project.team << [user, :reporter] }
 
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index f84f922ba5e..b6f329afd97 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -254,6 +254,8 @@ describe Projects::MergeRequestsController do
 
         expect { merge_request.reload.target_branch }.not_to change { merge_request.target_branch }
       end
+
+      it_behaves_like 'update invalid issuable', MergeRequest
     end
   end
 
diff --git a/spec/support/update_invalid_issuable.rb b/spec/support/update_invalid_issuable.rb
new file mode 100644
index 00000000000..f984ac7bfa7
--- /dev/null
+++ b/spec/support/update_invalid_issuable.rb
@@ -0,0 +1,57 @@
+shared_examples 'update invalid issuable' do |klass|
+  let(:params) do
+    {
+      namespace_id: project.namespace.path,
+      project_id: project.path,
+      id: issuable.iid
+    }
+  end
+
+  let(:issuable) do
+    klass == Issue ? issue : merge_request
+  end
+
+  before do
+    if klass == Issue
+      params.merge!(issue: { title: "any" })
+    else
+      params.merge!(merge_request: { title: "any" })
+    end
+  end
+
+  context 'when updating causes conflicts' do
+    before do
+      allow_any_instance_of(issuable.class).to receive(:save).
+        and_raise(ActiveRecord::StaleObjectError.new(issuable, :save))
+    end
+
+    it 'renders edit when format is html' do
+      put :update, params
+
+      expect(response).to render_template(:edit)
+      expect(assigns[:conflict]).to be_truthy
+    end
+
+    it 'renders json error message when format is json' do
+      params.merge!(format: "json")
+
+      put :update, params
+
+      expect(response.status).to eq(409)
+      expect(JSON.parse(response.body)).to have_key('errors')
+    end
+  end
+
+  context 'when updating an invalid issuable' do
+    before do
+      key = klass == Issue ? :issue : :merge_request
+      params[key][:title] = ""
+    end
+
+    it 'renders edit when merge request is invalid' do
+      put :update, params
+
+      expect(response).to render_template(:edit)
+    end
+  end
+end