Merge branch...

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

Gemfile

@@ -113,7 +113,7 @@ gem 'seed-fu', '~> 2.3.7'
 
 # Markdown and HTML processing
 gem 'html-pipeline', '~> 2.8'
-gem 'deckar01-task_list', '2.0.1'
+gem 'deckar01-task_list', '2.2.0'
 gem 'gitlab-markup', '~> 1.6.5'
 gem 'github-markup', '~> 1.7.0', require: 'github/markup'
 gem 'redcarpet', '~> 3.4'

Gemfile.lock

@@ -143,7 +143,7 @@ GEM
     database_cleaner (1.7.0)
     debug_inspector (0.0.3)
     debugger-ruby_core_source (1.3.8)
-    deckar01-task_list (2.0.1)
+    deckar01-task_list (2.2.0)
       html-pipeline
     declarative (0.0.10)
     declarative-option (0.1.0)
@@ -982,7 +982,7 @@ DEPENDENCIES
   connection_pool (~> 2.0)
   creole (~> 0.5.0)
   database_cleaner (~> 1.7.0)
-  deckar01-task_list (= 2.0.1)
+  deckar01-task_list (= 2.2.0)
   device_detector
   devise (~> 4.4)
   devise-two-factor (~> 3.0.0)

app/assets/javascripts/issue_show/components/app.vue

 <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"

app/assets/javascripts/issue_show/components/description.vue

 <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');

app/assets/javascripts/issue_show/stores/index.js

+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) {

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;

app/assets/javascripts/task_list.js

 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);
+      });
   }
 }

app/controllers/concerns/issuable_actions.rb

@@ -58,7 +58,8 @@ module IssuableActions
       title_text: issuable.title,
       description: view_context.markdown_field(issuable, :description),
       description_text: issuable.description,
-      task_status: issuable.task_status
+      task_status: issuable.task_status,
+      lock_version: issuable.lock_version
     }
 
     if issuable.edited?

app/controllers/projects/issues_controller.rb

@@ -246,7 +246,7 @@ class Projects::IssuesController < Projects::ApplicationController
       task_num
       lock_version
       discussion_locked
-    ] + [{ label_ids: [], assignee_ids: [] }]
+    ] + [{ label_ids: [], assignee_ids: [], update_task: [:index, :checked, :line_number, :line_source] }]
   end
 
   def store_uri

app/helpers/issuables_helper.rb

@@ -269,6 +269,7 @@ module IssuablesHelper
       markdownPreviewPath: preview_markdown_path(parent),
       markdownDocsPath: help_page_path('user/markdown'),
       markdownVersion: issuable.cached_markdown_version,
+      lockVersion: issuable.lock_version,
       issuableTemplates: issuable_templates(issuable),
       initialTitleHtml: markdown_field(issuable, :title),
       initialTitleText: issuable.title,

app/models/concerns/cache_markdown_field.rb

@@ -130,13 +130,17 @@ module CacheMarkdownField
   def latest_cached_markdown_version
     return CacheMarkdownField::CACHE_COMMONMARK_VERSION unless cached_markdown_version
 
-    if cached_markdown_version < CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
+    if legacy_markdown?
       CacheMarkdownField::CACHE_REDCARPET_VERSION
     else
       CacheMarkdownField::CACHE_COMMONMARK_VERSION
     end
   end
 
+  def legacy_markdown?
+    cached_markdown_version && cached_markdown_version.between?(1, CacheMarkdownField::CACHE_COMMONMARK_VERSION_START - 1)
+  end
+
   included do
     cattr_reader :cached_markdown_fields do
       FieldData.new
@@ -178,7 +182,9 @@ module CacheMarkdownField
       # author and project invalidate the cache in all circumstances.
       define_method(invalidation_method) do
         changed_fields = changed_attributes.keys
-        invalidations = changed_fields & [markdown_field.to_s, *INVALIDATED_BY]
+        invalidations  = changed_fields & [markdown_field.to_s, *INVALIDATED_BY]
+        invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html")
+
         !invalidations.empty? || !cached_html_up_to_date?(markdown_field)
       end
     end

app/models/concerns/issuable.rb

@@ -270,26 +270,29 @@ module Issuable
 
   def to_hook_data(user, old_associations: {})
     changes = previous_changes
-    old_labels = old_associations.fetch(:labels, [])
-    old_assignees = old_associations.fetch(:assignees, [])
 
-    if old_labels != labels
-      changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
-    end
+    if old_associations
+      old_labels = old_associations.fetch(:labels, [])
+      old_assignees = old_associations.fetch(:assignees, [])
 
-    if old_assignees != assignees
-      if self.is_a?(Issue)
-        changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
-      else
-        changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs]
+      if old_labels != labels
+        changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
       end
-    end
 
-    if self.respond_to?(:total_time_spent)
-      old_total_time_spent = old_associations.fetch(:total_time_spent, nil)
+      if old_assignees != assignees
+        if self.is_a?(Issue)
+          changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
+        else
+          changes[:assignee] = [old_assignees&.first&.hook_attrs, assignee&.hook_attrs]
+        end
+      end
+
+      if self.respond_to?(:total_time_spent)
+        old_total_time_spent = old_associations.fetch(:total_time_spent, nil)
 
-      if old_total_time_spent != total_time_spent
-        changes[:total_time_spent] = [old_total_time_spent, total_time_spent]
+        if old_total_time_spent != total_time_spent
+          changes[:total_time_spent] = [old_total_time_spent, total_time_spent]
+        end
       end
     end
 

app/models/concerns/taskable.rb

@@ -9,9 +9,11 @@ require 'task_list/filter'
 #
 # Used by MergeRequest and Issue
 module Taskable
-  COMPLETED    = 'completed'.freeze
-  INCOMPLETE   = 'incomplete'.freeze
-  ITEM_PATTERN = %r{
+  COMPLETED          = 'completed'.freeze
+  INCOMPLETE         = 'incomplete'.freeze
+  COMPLETE_PATTERN   = /(\[[xX]\])/.freeze
+  INCOMPLETE_PATTERN = /(\[[\s]\])/.freeze
+  ITEM_PATTERN       = %r{
     ^
     \s*(?:[-+*]|(?:\d+\.)) # list prefix required - task item has to be always in a list
     \s+                       # whitespace prefix has to be always presented for a list item

app/serializers/merge_request_basic_entity.rb

@@ -11,4 +11,5 @@ class MergeRequestBasicEntity < Grape::Entity
   expose :labels, using: LabelEntity
   expose :assignee, using: API::Entities::UserBasic
   expose :task_status, :task_status_short
+  expose :lock_version, :lock_version
 end

app/services/issuable/common_system_notes_service.rb

@@ -20,7 +20,7 @@ module Issuable
 
       create_due_date_note if issuable.previous_changes.include?('due_date')
       create_milestone_note if issuable.previous_changes.include?('milestone_id')
-      create_labels_note(old_labels) if issuable.labels != old_labels
+      create_labels_note(old_labels) if old_labels && issuable.labels != old_labels
     end
 
     private

app/services/issuable_base_service.rb

@@ -235,6 +235,63 @@ class IssuableBaseService < BaseService
     issuable
   end
 
+  def update_task(issuable)
+    filter_params(issuable)
+
+    if issuable.changed? || params.present?
+      issuable.assign_attributes(params.merge(updated_by: current_user,
+                                              last_edited_at: Time.now,
+                                              last_edited_by: current_user))
+
+      before_update(issuable)
+
+      if issuable.with_transaction_returning_status { issuable.save }
+        # We do not touch as it will affect a update on updated_at field
+        ActiveRecord::Base.no_touching do
+          Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels: nil)
+        end
+
+        handle_task_changes(issuable)
+        invalidate_cache_counts(issuable, users: issuable.assignees.to_a)
+        after_update(issuable)
+        execute_hooks(issuable, 'update', old_associations: nil)
+      end
+    end
+
+    issuable
+  end
+
+  # Handle the `update_task` event sent from UI.  Attempts to update a specific
+  # line in the markdown and cached html, bypassing any unnecessary updates or checks.
+  def update_task_event(issuable)
+    update_task_params = params.delete(:update_task)
+    return unless update_task_params
+
+    tasklist_toggler = TaskListToggleService.new(issuable.description, issuable.description_html,
+                                                 line_source: update_task_params[:line_source],
+                                                 line_number: update_task_params[:line_number].to_i,
+                                                 toggle_as_checked: update_task_params[:checked],
+                                                 index: update_task_params[:index].to_i,
+                                                 sourcepos: !issuable.legacy_markdown?)
+
+    unless tasklist_toggler.execute
+      # if we make it here, the data is much newer than we thought it was - fail fast
+      raise ActiveRecord::StaleObjectError
+    end
+
+    # by updating the description_html field at the same time,
+    # the markdown cache won't be considered invalid
+    params[:description]      = tasklist_toggler.updated_markdown
+    params[:description_html] = tasklist_toggler.updated_markdown_html
+
+    # since we're updating a very specific line, we don't care whether
+    # the `lock_version` sent from the FE is the same or not.  Just
+    # make sure the data hasn't changed since we queried it
+    params[:lock_version]     = issuable.lock_version
+
+    update_task(issuable)
+  end
+
   def labels_changing?(old_label_ids, new_label_ids)
     old_label_ids.sort != new_label_ids.sort
   end
@@ -317,6 +374,10 @@ class IssuableBaseService < BaseService
   def handle_changes(issuable, options)
   end
 
+  # override if needed
+  def handle_task_changes(issuable)
+  end
+
   # override if needed
   def execute_hooks(issuable, action = 'open', params = {})
   end

app/services/issues/update_service.rb

@@ -8,7 +8,7 @@ module Issues
       handle_move_between_ids(issue)
       filter_spam_check_params
       change_issue_duplicate(issue)
-      move_issue_to_new_project(issue) || update(issue)
+      move_issue_to_new_project(issue) || update_task_event(issue) || update(issue)
     end
 
     def update(issue)
@@ -63,6 +63,11 @@ module Issues
       end
     end
 
+    def handle_task_changes(issuable)
+      todo_service.mark_pending_todos_as_done(issuable, current_user)
+      todo_service.update_issue(issuable, current_user)
+    end
+
     def handle_move_between_ids(issue)
       return unless params[:move_between_ids]
 
@@ -78,6 +83,8 @@ module Issues
     # rubocop: disable CodeReuse/ActiveRecord
     def change_issue_duplicate(issue)
       canonical_issue_id = params.delete(:canonical_issue_id)
+      return unless canonical_issue_id
+
       canonical_issue = IssuesFinder.new(current_user).find_by(id: canonical_issue_id)
 
       if canonical_issue

app/services/task_list_toggle_service.rb

+# frozen_string_literal: true
+
+# Finds the correct checkbox in the passed in markdown/html and toggles it's state,
+# returning the updated markdown/html.
+# We don't care if the text has changed above or below the specific checkbox, as long
+# the checkbox still exists at exactly the same line number and the text is equal.
+# If successful, new values are available in `updated_markdown` and `updated_markdown_html`
+#
+# Note: once we've removed RedCarpet support, we can remove the `index` and `sourcepos`
+# parameters
+class TaskListToggleService
+  attr_reader :updated_markdown, :updated_markdown_html
+
+  def initialize(markdown, markdown_html, line_source:, line_number:, toggle_as_checked:, index:, sourcepos: true)
+    @markdown, @markdown_html  = markdown, markdown_html
+    @line_source, @line_number = line_source, line_number
+    @toggle_as_checked         = toggle_as_checked
+    @index, @use_sourcepos     = index, sourcepos
+
+    @updated_markdown, @updated_markdown_html = nil
+  end
+
+  def execute
+    return false unless markdown && markdown_html
+
+    toggle_markdown && toggle_markdown_html
+  end
+
+  private
+
+  attr_reader :markdown, :markdown_html, :index, :toggle_as_checked
+  attr_reader :line_source, :line_number, :use_sourcepos
+
+  def toggle_markdown
+    source_lines      = markdown.split("\n")
+    source_line_index = line_number - 1
+    markdown_task     = source_lines[source_line_index]
+
+    return unless markdown_task == line_source
+    return unless source_checkbox = Taskable::ITEM_PATTERN.match(markdown_task)
+
+    currently_checked = TaskList::Item.new(source_checkbox[1]).complete?
+
+    # Check `toggle_as_checked` to make sure we don't accidentally replace
+    # any `[ ]` or `[x]` in the middle of the text
+    if currently_checked
+      markdown_task.sub!(Taskable::COMPLETE_PATTERN, '[ ]') unless toggle_as_checked
+    else
+      markdown_task.sub!(Taskable::INCOMPLETE_PATTERN, '[x]') if toggle_as_checked
+    end
+
+    source_lines[source_line_index] = markdown_task
+    @updated_markdown = source_lines.join("\n")
+  end
+
+  def toggle_markdown_html
+    html          = Nokogiri::HTML.fragment(markdown_html)
+    html_checkbox = get_html_checkbox(html)
+    return unless html_checkbox
+
+    if toggle_as_checked
+      html_checkbox[:checked] = 'checked'
+    else
+      html_checkbox.remove_attribute('checked')
+    end
+
+    @updated_markdown_html = html.to_html
+  end
+
+  # When using CommonMark, we should be able to use the embedded `sourcepos` attribute to
+  # target the exact line in the DOM.  For RedCarpet, we need to use the index of the checkbox
+  # that was checked and match it with what we think is the same checkbox.
+  # The reason `sourcepos` is slightly more reliable is the case where a line of text is
+  # changed from a regular line into a checkbox (or vice versa).  Then the checked index
+  # in the UI will be off from the list of checkboxes we've calculated locally.
+  # It's a rare circumstance, but since we can account for it, we do.
+  def get_html_checkbox(html)
+    if use_sourcepos
+      html.css(".task-list-item[data-sourcepos^='#{line_number}:'] > input.task-list-item-checkbox").first
+    else
+      html.css('.task-list-item-checkbox')[index - 1]
+    end
+  end
+end

app/views/projects/merge_requests/show.html.haml

@@ -7,7 +7,7 @@
 - page_card_attributes @merge_request.card_attributes
 - suggest_changes_help_path = help_page_path('user/discussions/index.md', anchor: 'suggest-changes')
 
-.merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project) } }
+.merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project), lock_version: @merge_request.lock_version } }
   = render "projects/merge_requests/mr_title"
 
   .merge-request-details.issuable-details{ data: { id: @merge_request.project.id } }

changelogs/unreleased/19745-forms-with-task-lists-can-be-overwritten-when-editing-simultaneously.yml

+---
+title: Increase reliability and performance of toggling task items
+merge_request: 23938
+author:
+type: fixed

locale/gitlab.pot

@@ -2992,6 +2992,9 @@ msgstr ""
 msgid "Error Tracking"
 msgstr ""
 
+msgid "Error deleting  %{issuableType}"
+msgstr ""
+
 msgid "Error fetching contributors data."
 msgstr ""
 
@@ -3040,6 +3043,9 @@ msgstr ""
 msgid "Error saving label update."
 msgstr ""
 
+msgid "Error updating %{issuableType}"
+msgstr ""
+
 msgid "Error updating status for all todos."
 msgstr ""
 
@@ -6524,6 +6530,9 @@ msgstr ""
 msgid "Snippets"
 msgstr ""
 
+msgid "Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again."
+msgstr ""
+
 msgid "Something went wrong on our end"
 msgstr ""
 
@@ -7710,6 +7719,9 @@ msgstr ""
 msgid "Update"
 msgstr ""
 
+msgid "Update failed"
+msgstr ""
+
 msgid "Update now"
 msgstr ""
 

package.json

@@ -56,7 +56,7 @@
     "d3-time": "^1.0.8",
     "d3-time-format": "^2.1.1",
     "dateformat": "^3.0.3",
-    "deckar01-task_list": "^2.0.1",
+    "deckar01-task_list": "^2.2.0",
     "diff": "^3.4.0",
     "document-register-element": "1.3.0",
     "dropzone": "^4.2.0",

spec/controllers/projects/issues_controller_spec.rb

@@ -379,6 +379,23 @@ describe Projects::IssuesController do
         expect(response).to have_gitlab_http_status(200)
       end
     end
+
+    context 'when getting the changes' do
+      before do
+        project.add_developer(user)
+
+        sign_in(user)
+      end
+
+      it 'returns the necessary data' do
+        go(id: issue.iid)
+
+        data = JSON.parse(response.body)
+
+        expect(data).to include('title_text', 'description', 'description_text')
+        expect(data).to include('task_status', 'lock_version')
+      end
+    end
   end
 
   describe 'Confidential Issues' do

spec/features/task_lists_spec.rb

@@ -170,8 +170,11 @@ describe 'Task Lists' do
         expect(page).to have_content("2 of 7 tasks completed")
 
         page.find('li.task-list-item', text: 'Task b').find('input').click
+        wait_for_requests
         page.find('li.task-list-item ul li.task-list-item', text: 'Task a.2').find('input').click
+        wait_for_requests
         page.find('li.task-list-item ol li.task-list-item', text: 'Task 1.1').find('input').click
+        wait_for_requests
 
         expect(page).to have_content("5 of 7 tasks completed")
 
@@ -184,25 +187,24 @@ describe 'Task Lists' do
     end
 
     describe 'nested tasks', :js do
-      context 'with Redcarpet' do
-        let(:issue) { create(:issue, description: nested_tasks_markdown_redcarpet, author: user, project: project) }
+      let(:cache_version) { CacheMarkdownField::CACHE_COMMONMARK_VERSION }
+      let!(:issue) do
+        create(:issue, description: nested_tasks_markdown, author: user, project: project,
+                       cached_markdown_version: cache_version)
+      end
+
+      before do
+        visit_issue(project, issue)
+      end
 
-        before do
-          allow_any_instance_of(Banzai::Filter::MarkdownFilter).to receive(:engine).and_return('Redcarpet')
-          visit_issue(project, issue)
-        end
+      context 'with Redcarpet' do
+        let(:cache_version) { CacheMarkdownField::CACHE_REDCARPET_VERSION }
+        let(:nested_tasks_markdown) { nested_tasks_markdown_redcarpet }
 
         it_behaves_like 'shared nested tasks'
       end
 
       context 'with CommonMark' do
-        let(:issue) { create(:issue, description: nested_tasks_markdown, author: user, project: project) }
-
-        before do
-          allow_any_instance_of(Banzai::Filter::MarkdownFilter).to receive(:engine).and_return('CommonMark')
-          visit_issue(project, issue)
-        end
-
         it_behaves_like 'shared nested tasks'
       end
     end

spec/fixtures/api/schemas/entities/merge_request_basic.json

@@ -22,7 +22,8 @@
       "type": [ "array", "null" ]
     },
     "task_status": { "type": "string" },
-    "task_status_short": { "type": "string" }
+    "task_status_short": { "type": "string" },
+    "lock_version": { "type": ["string", "null"] }
   },
   "additionalProperties": false
 }

spec/helpers/issuables_helper_spec.rb

@@ -190,6 +190,7 @@ describe IssuablesHelper do
         markdownDocsPath: '/help/user/markdown',
         markdownVersion: CacheMarkdownField::CACHE_COMMONMARK_VERSION,
         issuableTemplates: [],
+        lockVersion: issue.lock_version,
         projectPath: @project.path,
         projectNamespace: @project.namespace.path,
         initialTitleHtml: issue.title,

spec/javascripts/issue_show/components/description_spec.js

@@ -123,7 +123,10 @@ describe('Description component', () => {
           fieldName: 'description',
           selector: '.detail-page-description',
           onSuccess: jasmine.any(Function),
+          onError: jasmine.any(Function),
+          lockVersion: 0,
         });
+
         done();
       });
     });
@@ -184,4 +187,18 @@ describe('Description component', () => {
   it('sets data-update-url', () => {
     expect(vm.$el.querySelector('textarea').dataset.updateUrl).toEqual(gl.TEST_HOST);
   });
+
+  describe('taskListUpdateError', () => {
+    it('should create flash notification and emit an event to parent', () => {
+      const msg =
+        'Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again.';
+      spyOn(window, 'Flash');
+      spyOn(vm, '$emit');
+
+      vm.taskListUpdateError();
+
+      expect(window.Flash).toHaveBeenCalledWith(msg);
+      expect(vm.$emit).toHaveBeenCalledWith('taskListUpdateFailed');
+    });
+  });
 });

spec/javascripts/issue_show/mock_data.js

@@ -8,6 +8,7 @@ export default {
     updated_at: '2015-05-15T12:31:04.428Z',
     updated_by_name: 'Some User',
     updated_by_path: '/some_user',
+    lock_version: 1,
   },
   secondRequest: {
     title: '<p>2</p>',
@@ -18,5 +19,6 @@ export default {
     updated_at: '2016-05-15T12:31:04.428Z',
     updated_by_name: 'Other User',
     updated_by_path: '/other_user',
+    lock_version: 2,
   },
 };

spec/javascripts/merge_request_spec.js

@@ -41,15 +41,28 @@ describe('MergeRequest', function() {
     });
 
     it('submits an ajax request on tasklist:changed', done => {
-      $('.js-task-list-field').trigger('tasklist:changed');
+      const lineNumber = 8;
+      const lineSource = '- [ ] item 8';
+      const index = 3;
+      const checked = true;
+
+      $('.js-task-list-field').trigger({
+        type: 'tasklist:changed',
+        detail: { lineNumber, lineSource, index, checked },
+      });
 
       setTimeout(() => {
         expect(axios.patch).toHaveBeenCalledWith(
           `${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`,
           {
-            merge_request: { description: '- [ ] Task List Item' },
+            merge_request: {
+              description: '- [ ] Task List Item',
+              lock_version: undefined,
+              update_task: { line_number: lineNumber, line_source: lineSource, index, checked },
+            },
           },
         );
+
         done();
       });
     });

spec/javascripts/notes_spec.js

@@ -89,10 +89,25 @@ describe('Notes', function() {
     });
 
     it('submits an ajax request on tasklist:changed', function(done) {
-      $('.js-task-list-container').trigger('tasklist:changed');
+      const lineNumber = 8;
+      const lineSource = '- [ ] item 8';
+      const index = 3;
+      const checked = true;
+
+      $('.js-task-list-container').trigger({
+        type: 'tasklist:changed',
+        detail: { lineNumber, lineSource, index, checked },
+      });
 
       setTimeout(() => {
-        expect(axios.patch).toHaveBeenCalled();
+        expect(axios.patch).toHaveBeenCalledWith(undefined, {
+          note: {
+            note: '',
+            lock_version: undefined,
+            update_task: { index, checked, line_number: lineNumber, line_source: lineSource },
+          },
+        });
+
         done();
       });
     });

spec/javascripts/task_list_spec.js

+import $ from 'jquery';
+import TaskList from '~/task_list';
+import axios from '~/lib/utils/axios_utils';
+
+describe('TaskList', () => {
+  let taskList;
+  let currentTarget;
+  const taskListOptions = {
+    selector: '.task-list',
+    dataType: 'issue',
+    fieldName: 'description',
+    lockVersion: 2,
+  };
+  const createTaskList = () => new TaskList(taskListOptions);
+
+  beforeEach(() => {
+    setFixtures(`
+      <div class="task-list">
+        <div class="js-task-list-container"></div>
+      </div>
+    `);
+
+    currentTarget = $('<div></div>');
+    taskList = createTaskList();
+  });
+
+  it('should call init when the class constructed', () => {
+    spyOn(TaskList.prototype, 'init').and.callThrough();
+    spyOn(TaskList.prototype, 'disable');
+    spyOn($.prototype, 'taskList');
+    spyOn($.prototype, 'on');
+
+    taskList = createTaskList();
+    const $taskListEl = $(taskList.taskListContainerSelector);
+
+    expect(taskList.init).toHaveBeenCalled();
+    expect(taskList.disable).toHaveBeenCalled();
+    expect($taskListEl.taskList).toHaveBeenCalledWith('enable');
+    expect($(document).on).toHaveBeenCalledWith(
+      'tasklist:changed',
+      taskList.taskListContainerSelector,
+      taskList.updateHandler,
+    );
+  });
+
+  describe('getTaskListTarget', () => {
+    it('should return currentTarget from event object if exists', () => {
+      const $target = taskList.getTaskListTarget({ currentTarget });
+
+      expect($target).toEqual(currentTarget);
+    });
+
+    it('should return element of the taskListContainerSelector', () => {
+      const $target = taskList.getTaskListTarget();
+
+      expect($target).toEqual($(taskList.taskListContainerSelector));
+    });
+  });
+
+  describe('disableTaskListItems', () => {
+    it('should call taskList method with disable param', () => {
+      spyOn($.prototype, 'taskList');
+
+      taskList.disableTaskListItems({ currentTarget });
+
+      expect(currentTarget.taskList).toHaveBeenCalledWith('disable');
+    });
+  });
+
+  describe('enableTaskListItems', () => {
+    it('should call taskList method with enable param', () => {
+      spyOn($.prototype, 'taskList');
+
+      taskList.enableTaskListItems({ currentTarget });
+
+      expect(currentTarget.taskList).toHaveBeenCalledWith('enable');
+    });
+  });
+
+  describe('disable', () => {
+    it('should disable task list items and off document event', () => {
+      spyOn(taskList, 'disableTaskListItems');
+      spyOn($.prototype, 'off');
+
+      taskList.disable();
+
+      expect(taskList.disableTaskListItems).toHaveBeenCalled();
+      expect($(document).off).toHaveBeenCalledWith(
+        'tasklist:changed',
+        taskList.taskListContainerSelector,
+      );
+    });
+  });
+
+  describe('update', () => {
+    it('should disable task list items and make a patch request then enable them again', done => {
+      const response = { data: { lock_version: 3 } };
+      spyOn(taskList, 'enableTaskListItems');
+      spyOn(taskList, 'disableTaskListItems');
+      spyOn(taskList, 'onSuccess');
+      spyOn(axios, 'patch').and.returnValue(Promise.resolve(response));
+
+      const value = 'hello world';
+      const endpoint = '/foo';
+      const target = $(`<input data-update-url="${endpoint}" value="${value}" />`);
+      const detail = {
+        index: 2,
+        checked: true,
+        lineNumber: 8,
+        lineSource: '- [ ] check item',
+      };
+      const event = { target, detail };
+      const patchData = {
+        [taskListOptions.dataType]: {
+          [taskListOptions.fieldName]: value,
+          lock_version: taskListOptions.lockVersion,
+          update_task: {
+            index: detail.index,
+            checked: detail.checked,
+            line_number: detail.lineNumber,
+            line_source: detail.lineSource,
+          },
+        },
+      };
+
+      taskList
+        .update(event)
+        .then(() => {
+          expect(taskList.disableTaskListItems).toHaveBeenCalledWith(event);
+          expect(axios.patch).toHaveBeenCalledWith(endpoint, patchData);
+          expect(taskList.enableTaskListItems).toHaveBeenCalledWith(event);
+          expect(taskList.onSuccess).toHaveBeenCalledWith(response.data);
+          expect(taskList.lockVersion).toEqual(response.data.lock_version);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+
+  it('should handle request error and enable task list items', done => {
+    const response = { data: { error: 1 } };
+    spyOn(taskList, 'enableTaskListItems');
+    spyOn(taskList, 'onError');
+    spyOn(axios, 'patch').and.returnValue(Promise.reject({ response })); // eslint-disable-line prefer-promise-reject-errors
+
+    const event = { detail: {} };
+    taskList
+      .update(event)
+      .then(() => {
+        expect(taskList.enableTaskListItems).toHaveBeenCalledWith(event);
+        expect(taskList.onError).toHaveBeenCalledWith(response.data);
+      })
+      .then(done)
+      .catch(done.fail);
+  });
+});

spec/models/concerns/cache_markdown_field_spec.rb

@@ -133,6 +133,15 @@ describe CacheMarkdownField do
     end
   end
 
+  context 'when a markdown field and html field are both changed' do
+    it do
+      expect(thing).not_to receive(:refresh_markdown_cache)
+      thing.foo = '_look over there!_'
+      thing.foo_html = '<em>look over there!</em>'
+      thing.save
+    end
+  end
+
   context 'a non-markdown field changed' do
     shared_examples 'with cache version' do |cache_version|
       let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
@@ -242,6 +251,30 @@ describe CacheMarkdownField do
     end
   end
 
+  describe '#legacy_markdown?' do
+    subject { thing.legacy_markdown? }
+
+    it 'returns true for redcarpet versions' do
+      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START - 1
+      is_expected.to be_truthy
+    end
+
+    it 'returns false for commonmark versions' do
+      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
+      is_expected.to be_falsey
+    end
+
+    it 'returns false if nil' do
+      thing.cached_markdown_version = nil
+      is_expected.to be_falsey
+    end
+
+    it 'returns false if 0' do
+      thing.cached_markdown_version = 0
+      is_expected.to be_falsey
+    end
+  end
+
   describe '#refresh_markdown_cache' do
     before do
       thing.foo = updated_markdown

spec/services/issues/update_service_spec.rb

@@ -543,6 +543,76 @@ describe Issues::UpdateService, :mailer do
       end
     end
 
+    context 'when updating a single task' do
+      before do
+        update_issue(description: "- [ ] Task 1\n- [ ] Task 2")
+      end
+
+      it { expect(issue.tasks?).to eq(true) }
+
+      context 'when a task is marked as completed' do
+        before do
+          update_issue(update_task: { index: 1, checked: true, line_source: '- [ ] Task 1', line_number: 1 })
+        end
+
+        it 'creates system note about task status change' do
+          note1 = find_note('marked the task **Task 1** as completed')
+
+          expect(note1).not_to be_nil
+
+          description_notes = find_notes('description')
+          expect(description_notes.length).to eq(1)
+        end
+      end
+
+      context 'when a task is marked as incomplete' do
+        before do
+          update_issue(description: "- [x] Task 1\n- [X] Task 2")
+          update_issue(update_task: { index: 2, checked: false, line_source: '- [X] Task 2', line_number: 2 })
+        end
+
+        it 'creates system note about task status change' do
+          note1 = find_note('marked the task **Task 2** as incomplete')
+
+          expect(note1).not_to be_nil
+
+          description_notes = find_notes('description')
+          expect(description_notes.length).to eq(1)
+        end
+      end
+
+      context 'when the task position has been modified' do
+        before do
+          update_issue(description: "- [ ] Task 1\n- [ ] Task 3\n- [ ] Task 2")
+        end
+
+        it 'raises an exception' do
+          expect(Note.count).to eq(2)
+          expect do
+            update_issue(update_task: { index: 2, checked: true, line_source: '- [ ] Task 2', line_number: 2 })
+          end.to raise_error(ActiveRecord::StaleObjectError)
+          expect(Note.count).to eq(2)
+        end
+      end
+
+      context 'when the content changes but not task line number' do
+        before do
+          update_issue(description: "Paragraph\n\n- [ ] Task 1\n- [x] Task 2")
+          update_issue(description: "Paragraph with more words\n\n- [ ] Task 1\n- [x] Task 2")
+          update_issue(update_task: { index: 2, checked: false, line_source: '- [x] Task 2', line_number: 4 })
+        end
+
+        it 'creates system note about task status change' do
+          note1 = find_note('marked the task **Task 2** as incomplete')
+
+          expect(note1).not_to be_nil
+
+          description_notes = find_notes('description')
+          expect(description_notes.length).to eq(2)
+        end
+      end
+    end
+
     context 'updating labels' do
       let(:label3) { create(:label, project: project) }
       let(:result) { described_class.new(project, user, params).execute(issue).reload }

spec/services/task_list_toggle_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe TaskListToggleService do
+  let(:sourcepos) { true }
+  let(:markdown) do
+    <<-EOT.strip_heredoc
+      * [ ] Task 1
+      * [x] Task 2
+
+      A paragraph
+
+      1. [X] Item 1
+         - [ ] Sub-item 1
+    EOT
+  end
+
+  let(:markdown_html) do
+    <<-EOT.strip_heredoc
+      <ul data-sourcepos="1:1-3:0" class="task-list" dir="auto">
+        <li data-sourcepos="1:1-1:12" class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled> Task 1
+        </li>
+        <li data-sourcepos="2:1-3:0" class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled checked> Task 2
+        </li>
+      </ul>
+      <p data-sourcepos="4:1-4:11" dir="auto">A paragraph</p>
+      <ol data-sourcepos="6:1-7:19" class="task-list" dir="auto">
+        <li data-sourcepos="6:1-7:19" class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled checked> Item 1
+          <ul data-sourcepos="7:4-7:19" class="task-list">
+            <li data-sourcepos="7:4-7:19" class="task-list-item">
+              <input type="checkbox" class="task-list-item-checkbox" disabled> Sub-item 1
+            </li>
+          </ul>
+        </li>
+      </ol>
+    EOT
+  end
+
+  shared_examples 'task lists' do
+    it 'checks Task 1' do
+      toggler = described_class.new(markdown, markdown_html,
+                                    index: 1, toggle_as_checked: true,
+                                    line_source: '* [ ] Task 1', line_number: 1,
+                                    sourcepos: sourcepos)
+
+      expect(toggler.execute).to be_truthy
+      expect(toggler.updated_markdown.lines[0]).to eq "* [x] Task 1\n"
+      expect(toggler.updated_markdown_html).to include('disabled checked> Task 1')
+    end
+
+    it 'unchecks Item 1' do
+      toggler = described_class.new(markdown, markdown_html,
+                                    index: 3, toggle_as_checked: false,
+                                    line_source: '1. [X] Item 1', line_number: 6,
+                                    sourcepos: sourcepos)
+
+      expect(toggler.execute).to be_truthy
+      expect(toggler.updated_markdown.lines[5]).to eq "1. [ ] Item 1\n"
+      expect(toggler.updated_markdown_html).to include('disabled> Item 1')
+    end
+
+    it 'returns false if line_source does not match the text' do
+      toggler = described_class.new(markdown, markdown_html,
+                                    index: 2, toggle_as_checked: false,
+                                    line_source: '* [x] Task Added', line_number: 2,
+                                    sourcepos: sourcepos)
+
+      expect(toggler.execute).to be_falsey
+    end
+
+    it 'returns false if markdown is nil' do
+      toggler = described_class.new(nil, markdown_html,
+                                    index: 2, toggle_as_checked: false,
+                                    line_source: '* [x] Task Added', line_number: 2,
+                                    sourcepos: sourcepos)
+
+      expect(toggler.execute).to be_falsey
+    end
+
+    it 'returns false if markdown_html is nil' do
+      toggler = described_class.new(markdown, nil,
+                                    index: 2, toggle_as_checked: false,
+                                    line_source: '* [x] Task Added', line_number: 2,
+                                    sourcepos: sourcepos)
+
+      expect(toggler.execute).to be_falsey
+    end
+  end
+
+  context 'when using sourcepos' do
+    it_behaves_like 'task lists'
+  end
+
+  context 'when using checkbox indexing' do
+    let(:sourcepos) { false }
+    let(:markdown_html) do
+      <<-EOT.strip_heredoc
+      <ul class="task-list" dir="auto">
+        <li class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled> Task 1
+        </li>
+        <li class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled checked> Task 2
+        </li>
+      </ul>
+      <p dir="auto">A paragraph</p>
+      <ol class="task-list" dir="auto">
+        <li class="task-list-item">
+          <input type="checkbox" class="task-list-item-checkbox" disabled checked> Item 1
+          <ul class="task-list">
+            <li class="task-list-item">
+              <input type="checkbox" class="task-list-item-checkbox" disabled> Sub-item 1
+            </li>
+          </ul>
+        </li>
+      </ol>
+      EOT
+    end
+
+    it_behaves_like 'task lists'
+  end
+end

yarn.lock

@@ -3032,10 +3032,10 @@ decamelize@^2.0.0:
   dependencies:
     xregexp "4.0.0"
 
-deckar01-task_list@^2.0.1:
-  version "2.0.1"
-  resolved "https://registry.yarnpkg.com/deckar01-task_list/-/deckar01-task_list-2.0.1.tgz#fdcfb6ab5717055a82f29e863a49990a043a06a9"
-  integrity sha512-i5fT8QxJ9iV6dfgy5U0NHW91O5cKsvDc4u8JNMnZ6efQc356bA9vKuXO3732agSry+bO6TolzTmuqSRi4tkkeA==
+deckar01-task_list@^2.2.0:
+  version "2.2.0"
+  resolved "https://registry.yarnpkg.com/deckar01-task_list/-/deckar01-task_list-2.2.0.tgz#5cc3ea06f01d3d786b1a667064a462eb5d069bd3"
+  integrity sha512-NUfu5ARoD9SC2k+fBT5cBer59iKfEdawPrmfqp5+zAahTECb8z9dsuS1Xnx7jzFAmCCLnEs3z/aYucYXzNrKkQ==
 
 decode-uri-component@^0.2.0:
   version "0.2.0"