Merge branch '19745-forms-with-task-lists-can-be-overwritten-when-editing-simultaneously' into 'master'

Forms with task lists can be overwritten when editing simultaneously

See merge request gitlab-org/gitlab-ce!23938

5 files changed, 118 insertions, 37 deletions

diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue
index cd569eb3045..fea7f0d77a5 100644
--- a/app/assets/javascripts/issue_show/components/app.vue
+++ b/app/assets/javascripts/issue_show/components/app.vue
@@ -1,5 +1,7 @@
 <script>
 import Visibility from 'visibilityjs';
+import { __, s__, sprintf } from '~/locale';
+import createFlash from '~/flash';
 import { visitUrl } from '../../lib/utils/url_utility';
 import Poll from '../../lib/utils/poll';
 import eventHub from '../event_hub';
@@ -10,7 +12,6 @@ import descriptionComponent from './description.vue';
 import editedComponent from './edited.vue';
 import formComponent from './form.vue';
 import recaptchaModalImplementor from '../../vue_shared/mixins/recaptcha_modal_implementor';
-import { __ } from '~/locale';
 
 export default {
   components: {
@@ -130,6 +131,11 @@ export default {
       required: false,
       default: true,
     },
+    lockVersion: {
+      type: Number,
+      required: false,
+      default: 0,
+    },
   },
   data() {
     const store = new Store({
@@ -141,6 +147,7 @@ export default {
       updatedByName: this.updatedByName,
       updatedByPath: this.updatedByPath,
       taskStatus: this.initialTaskStatus,
+      lock_version: this.lockVersion,
     });
 
     return {
@@ -161,6 +168,9 @@ export default {
       const titleChanged = this.initialTitleText !== this.store.formState.title;
       return descriptionChanged || titleChanged;
     },
+    defaultErrorMessage() {
+      return sprintf(s__('Error updating %{issuableType}'), { issuableType: this.issuableType });
+    },
   },
   created() {
     this.service = new Service(this.endpoint);
@@ -207,6 +217,17 @@ export default {
       }
       return undefined;
     },
+    updateStoreState() {
+      return this.service
+        .getData()
+        .then(res => res.data)
+        .then(data => {
+          this.store.updateState(data);
+        })
+        .catch(() => {
+          createFlash(this.defaultErrorMessage);
+        });
+    },
 
     openForm() {
       if (!this.showForm) {
@@ -214,6 +235,7 @@ export default {
         this.store.setFormState({
           title: this.state.titleText,
           description: this.state.descriptionText,
+          lock_version: this.state.lock_version,
           lockedWarningVisible: false,
           updateLoading: false,
         });
@@ -232,20 +254,24 @@ export default {
           if (window.location.pathname !== data.web_url) {
             visitUrl(data.web_url);
           }
-
-          return this.service.getData();
         })
-        .then(res => res.data)
-        .then(data => {
-          this.store.updateState(data);
+        .then(this.updateStoreState)
+        .then(() => {
           eventHub.$emit('close.form');
         })
-        .catch(error => {
-          if (error && error.name === 'SpamError') {
+        .catch((error = {}) => {
+          const { name, response = {} } = error;
+
+          if (name === 'SpamError') {
             this.openRecaptcha();
           } else {
-            eventHub.$emit('close.form');
-            window.Flash(`Error updating ${this.issuableType}`);
+            let errMsg = this.defaultErrorMessage;
+
+            if (response.data && response.data.errors) {
+              errMsg += `. ${response.data.errors.join(' ')}`;
+            }
+
+            createFlash(errMsg);
           }
         });
     },
@@ -269,8 +295,9 @@ export default {
           visitUrl(data.web_url);
         })
         .catch(() => {
-          eventHub.$emit('close.form');
-          window.Flash(`Error deleting ${this.issuableType}`);
+          createFlash(
+            sprintf(s__('Error deleting  %{issuableType}'), { issuableType: this.issuableType }),
+          );
         });
     },
   },
@@ -314,6 +341,8 @@ export default {
         :task-status="state.taskStatus"
         :issuable-type="issuableType"
         :update-url="updateEndpoint"
+        :lock-version="state.lock_version"
+        @taskListUpdateFailed="updateStoreState"
       />
       <edited-component
         v-if="hasUpdated"
diff --git a/app/assets/javascripts/issue_show/components/description.vue b/app/assets/javascripts/issue_show/components/description.vue
index 5ca88d75063..e664269b199 100644
--- a/app/assets/javascripts/issue_show/components/description.vue
+++ b/app/assets/javascripts/issue_show/components/description.vue
@@ -1,5 +1,6 @@
 <script>
 import $ from 'jquery';
+import { __ } from '~/locale';
 import animateMixin from '../mixins/animate';
 import TaskList from '../../task_list';
 import recaptchaModalImplementor from '../../vue_shared/mixins/recaptcha_modal_implementor';
@@ -35,6 +36,11 @@ export default {
       required: false,
       default: null,
     },
+    lockVersion: {
+      type: Number,
+      required: false,
+      default: 0,
+    },
   },
   data() {
     return {
@@ -67,8 +73,10 @@ export default {
         new TaskList({
           dataType: this.issuableType,
           fieldName: 'description',
+          lockVersion: this.lockVersion,
           selector: '.detail-page-description',
           onSuccess: this.taskListUpdateSuccess.bind(this),
+          onError: this.taskListUpdateError.bind(this),
         });
       }
     },
@@ -82,6 +90,16 @@ export default {
       }
     },
 
+    taskListUpdateError() {
+      window.Flash(
+        __(
+          'Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again.',
+        ),
+      );
+
+      this.$emit('taskListUpdateFailed');
+    },
+
     updateTaskStatusText() {
       const taskRegexMatches = this.taskStatus.match(/(\d+) of ((?!0)\d+)/);
       const $issuableHeader = $('.issuable-meta');
diff --git a/app/assets/javascripts/issue_show/stores/index.js b/app/assets/javascripts/issue_show/stores/index.js
index 32044d6da25..3c17e73ccec 100644
--- a/app/assets/javascripts/issue_show/stores/index.js
+++ b/app/assets/javascripts/issue_show/stores/index.js
@@ -1,3 +1,5 @@
+import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
+
 export default class Store {
   constructor(initialState) {
     this.state = initialState;
@@ -6,6 +8,7 @@ export default class Store {
       description: '',
       lockedWarningVisible: false,
       updateLoading: false,
+      lock_version: 0,
     };
   }
 
@@ -14,14 +17,10 @@ export default class Store {
       this.formState.lockedWarningVisible = true;
     }
 
+    Object.assign(this.state, convertObjectPropsToCamelCase(data));
     this.state.titleHtml = data.title;
-    this.state.titleText = data.title_text;
     this.state.descriptionHtml = data.description;
-    this.state.descriptionText = data.description_text;
-    this.state.taskStatus = data.task_status;
-    this.state.updatedAt = data.updated_at;
-    this.state.updatedByName = data.updated_by_name;
-    this.state.updatedByPath = data.updated_by_path;
+    this.state.lock_version = data.lock_version;
   }
 
   stateShouldUpdate(data) {
diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js
index 0deae478deb..ac3b47cd218 100644
--- a/app/assets/javascripts/merge_request.js
+++ b/app/assets/javascripts/merge_request.js
@@ -35,6 +35,7 @@ function MergeRequest(opts) {
       dataType: 'merge_request',
       fieldName: 'description',
       selector: '.detail-page-description',
+      lockVersion: this.$el.data('lockVersion'),
       onSuccess: result => {
         document.querySelector('#task_status').innerText = result.task_status;
         document.querySelector('#task_status_short').innerText = result.task_status_short;
diff --git a/app/assets/javascripts/task_list.js b/app/assets/javascripts/task_list.js
index edefb3735d7..5172a1ef3d6 100644
--- a/app/assets/javascripts/task_list.js
+++ b/app/assets/javascripts/task_list.js
@@ -1,5 +1,6 @@
 import $ from 'jquery';
 import 'deckar01-task_list';
+import { __ } from '~/locale';
 import axios from './lib/utils/axios_utils';
 import Flash from './flash';
 
@@ -8,46 +9,79 @@ export default class TaskList {
     this.selector = options.selector;
     this.dataType = options.dataType;
     this.fieldName = options.fieldName;
+    this.lockVersion = options.lockVersion;
+    this.taskListContainerSelector = `${this.selector} .js-task-list-container`;
+    this.updateHandler = this.update.bind(this);
     this.onSuccess = options.onSuccess || (() => {});
-    this.onError = function showFlash(e) {
-      let errorMessages = '';
+    this.onError =
+      options.onError ||
+      function showFlash(e) {
+        let errorMessages = '';
 
-      if (e.response.data && typeof e.response.data === 'object') {
-        errorMessages = e.response.data.errors.join(' ');
-      }
+        if (e.response.data && typeof e.response.data === 'object') {
+          errorMessages = e.response.data.errors.join(' ');
+        }
 
-      return new Flash(errorMessages || 'Update failed', 'alert');
-    };
+        return new Flash(errorMessages || __('Update failed'), 'alert');
+      };
 
     this.init();
   }
 
   init() {
-    // Prevent duplicate event bindings
-    this.disable();
-    $(`${this.selector} .js-task-list-container`).taskList('enable');
-    $(document).on(
-      'tasklist:changed',
-      `${this.selector} .js-task-list-container`,
-      this.update.bind(this),
-    );
+    this.disable(); // Prevent duplicate event bindings
+
+    $(this.taskListContainerSelector).taskList('enable');
+    $(document).on('tasklist:changed', this.taskListContainerSelector, this.updateHandler);
+  }
+
+  getTaskListTarget(e) {
+    return e && e.currentTarget ? $(e.currentTarget) : $(this.taskListContainerSelector);
+  }
+
+  disableTaskListItems(e) {
+    this.getTaskListTarget(e).taskList('disable');
+  }
+
+  enableTaskListItems(e) {
+    this.getTaskListTarget(e).taskList('enable');
   }
 
   disable() {
-    $(`${this.selector} .js-task-list-container`).taskList('disable');
-    $(document).off('tasklist:changed', `${this.selector} .js-task-list-container`);
+    this.disableTaskListItems();
+    $(document).off('tasklist:changed', this.taskListContainerSelector);
   }
 
   update(e) {
     const $target = $(e.target);
+    const { index, checked, lineNumber, lineSource } = e.detail;
     const patchData = {};
+
     patchData[this.dataType] = {
       [this.fieldName]: $target.val(),
+      lock_version: this.lockVersion,
+      update_task: {
+        index,
+        checked,
+        line_number: lineNumber,
+        line_source: lineSource,
+      },
     };
 
+    this.disableTaskListItems(e);
+
     return axios
       .patch($target.data('updateUrl') || $('form.js-issuable-update').attr('action'), patchData)
-      .then(({ data }) => this.onSuccess(data))
-      .catch(err => this.onError(err));
+      .then(({ data }) => {
+        this.lockVersion = data.lock_version;
+        this.enableTaskListItems(e);
+
+        return this.onSuccess(data);
+      })
+      .catch(({ response }) => {
+        this.enableTaskListItems(e);
+
+        return this.onError(response.data);
+      });
   }
 }