diff options
author | Sean McGivern <sean@gitlab.com> | 2019-02-04 11:52:22 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2019-02-04 11:52:22 +0300 |
commit | 70b92fb380659717e2f3a4f5aac802ec5460e78c (patch) | |
tree | f7e8132f13cdcb25b098924e92eff5d53022483d | |
parent | 200ebab1cf8a41585c94a15bcc37547abd73bc80 (diff) | |
parent | 863ee930a6260c42d3d0252a0310ac9adff24862 (diff) |
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
36 files changed, 918 insertions, 169 deletions
@@ -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' diff --git a/Gemfile.lock b/Gemfile.lock index 1c28176ac62..e6b563b5cb8 100644 --- a/Gemfile.lock +++ b/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) 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); + }); } } diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 3d64ae8b775..8ef3b6502df 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/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? diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 69f983f7ccd..f9a80aa3cfb 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index f8176facce9..0fee29bf7c7 100644 --- a/app/helpers/issuables_helper.rb +++ b/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, diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 002f3e17891..588204c7470 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0d363ec68b7..b1cf03551f6 100644 --- a/app/models/concerns/issuable.rb +++ b/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 diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index 603d4d62578..f147ce8ad6b 100644 --- a/app/models/concerns/taskable.rb +++ b/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 diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index 084627f9dbe..178e72f4f0a 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/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 diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb index 885e14bba8f..77f38f8882e 100644 --- a/app/services/issuable/common_system_notes_service.rb +++ b/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 diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 805bb5b510d..842d59d26a0 100644 --- a/app/services/issuable_base_service.rb +++ b/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 @@ -318,6 +375,10 @@ class IssuableBaseService < BaseService end # override if needed + def handle_task_changes(issuable) + end + + # override if needed def execute_hooks(issuable, action = 'open', params = {}) end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index e992d682c79..cec5b5734c0 100644 --- a/app/services/issues/update_service.rb +++ b/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 diff --git a/app/services/task_list_toggle_service.rb b/app/services/task_list_toggle_service.rb new file mode 100644 index 00000000000..2717fc9035a --- /dev/null +++ b/app/services/task_list_toggle_service.rb @@ -0,0 +1,84 @@ +# 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 diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index d6f340d0ee2..0b720e5d542 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/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 } } diff --git a/changelogs/unreleased/19745-forms-with-task-lists-can-be-overwritten-when-editing-simultaneously.yml b/changelogs/unreleased/19745-forms-with-task-lists-can-be-overwritten-when-editing-simultaneously.yml new file mode 100644 index 00000000000..b1177e1717e --- /dev/null +++ b/changelogs/unreleased/19745-forms-with-task-lists-can-be-overwritten-when-editing-simultaneously.yml @@ -0,0 +1,5 @@ +--- +title: Increase reliability and performance of toggling task items +merge_request: 23938 +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8caa876e6b0..fd7e4754a7a 100644 --- a/locale/gitlab.pot +++ b/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 "" diff --git a/package.json b/package.json index 13c0527c4a3..bd673b86fab 100644 --- a/package.json +++ b/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", diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 4743ad04339..c34d7c13d57 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/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 diff --git a/spec/features/task_lists_spec.rb b/spec/features/task_lists_spec.rb index 9c9127980a1..b549f2b5c62 100644 --- a/spec/features/task_lists_spec.rb +++ b/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 diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index 4c04c838cb8..3006b482d41 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/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 } diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 03e3a72a82f..af319e5ebfe 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/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, diff --git a/spec/javascripts/issue_show/components/app_spec.js b/spec/javascripts/issue_show/components/app_spec.js index 2bd1b3996dc..0ccf771c7ef 100644 --- a/spec/javascripts/issue_show/components/app_spec.js +++ b/spec/javascripts/issue_show/components/app_spec.js @@ -18,9 +18,13 @@ describe('Issuable output', () => { let realtimeRequestCount = 0; let vm; - document.body.innerHTML = '<span id="task_status"></span>'; - beforeEach(done => { + setFixtures(` + <div> + <div class="flash-container"></div> + <span id="task_status"></span> + </div> + `); spyOn(eventHub, '$emit'); const IssuableDescriptionComponent = Vue.extend(issuableApp); @@ -43,6 +47,7 @@ describe('Issuable output', () => { initialTitleText: '', initialDescriptionHtml: 'test', initialDescriptionText: 'test', + lockVersion: 1, markdownPreviewPath: '/', markdownDocsPath: '/', projectNamespace: '/', @@ -78,6 +83,7 @@ describe('Issuable output', () => { expect(formatText(editedText.innerText)).toMatch(/Edited[\s\S]+?by Some User/); expect(editedText.querySelector('.author-link').href).toMatch(/\/some_user$/); expect(editedText.querySelector('time')).toBeTruthy(); + expect(vm.state.lock_version).toEqual(1); }) .then(() => { vm.poll.makeRequest(); @@ -95,6 +101,7 @@ describe('Issuable output', () => { expect(editedText.querySelector('.author-link').href).toMatch(/\/other_user$/); expect(editedText.querySelector('time')).toBeTruthy(); + expect(vm.state.lock_version).toEqual(2); }) .then(done) .catch(done.fail); @@ -137,21 +144,17 @@ describe('Issuable output', () => { describe('updateIssuable', () => { it('fetches new data after update', done => { + spyOn(vm, 'updateStoreState').and.callThrough(); spyOn(vm.service, 'getData').and.callThrough(); - spyOn(vm.service, 'updateIssuable').and.callFake( - () => - new Promise(resolve => { - resolve({ - data: { - confidential: false, - web_url: window.location.pathname, - }, - }); - }), + spyOn(vm.service, 'updateIssuable').and.returnValue( + Promise.resolve({ + data: { web_url: window.location.pathname }, + }), ); vm.updateIssuable() .then(() => { + expect(vm.updateStoreState).toHaveBeenCalled(); expect(vm.service.getData).toHaveBeenCalled(); }) .then(done) @@ -159,11 +162,10 @@ describe('Issuable output', () => { }); it('correctly updates issuable data', done => { - spyOn(vm.service, 'updateIssuable').and.callFake( - () => - new Promise(resolve => { - resolve(); - }), + spyOn(vm.service, 'updateIssuable').and.returnValue( + Promise.resolve({ + data: { web_url: window.location.pathname }, + }), ); vm.updateIssuable() @@ -177,16 +179,13 @@ describe('Issuable output', () => { it('does not redirect if issue has not moved', done => { const visitUrl = spyOnDependency(issuableApp, 'visitUrl'); - spyOn(vm.service, 'updateIssuable').and.callFake( - () => - new Promise(resolve => { - resolve({ - data: { - web_url: window.location.pathname, - confidential: vm.isConfidential, - }, - }); - }), + spyOn(vm.service, 'updateIssuable').and.returnValue( + Promise.resolve({ + data: { + web_url: window.location.pathname, + confidential: vm.isConfidential, + }, + }), ); vm.updateIssuable(); @@ -199,16 +198,13 @@ describe('Issuable output', () => { it('redirects if returned web_url has changed', done => { const visitUrl = spyOnDependency(issuableApp, 'visitUrl'); - spyOn(vm.service, 'updateIssuable').and.callFake( - () => - new Promise(resolve => { - resolve({ - data: { - web_url: '/testing-issue-move', - confidential: vm.isConfidential, - }, - }); - }), + spyOn(vm.service, 'updateIssuable').and.returnValue( + Promise.resolve({ + data: { + web_url: '/testing-issue-move', + confidential: vm.isConfidential, + }, + }), ); vm.updateIssuable(); @@ -227,6 +223,7 @@ describe('Issuable output', () => { vm.handleBeforeUnloadEvent(e); Vue.nextTick(() => { expect(e.returnValue).not.toBeNull(); + done(); }); }); @@ -238,6 +235,7 @@ describe('Issuable output', () => { vm.handleBeforeUnloadEvent(e); Vue.nextTick(() => { expect(e.returnValue).not.toBeNull(); + done(); }); }); @@ -247,49 +245,61 @@ describe('Issuable output', () => { vm.handleBeforeUnloadEvent(e); Vue.nextTick(() => { expect(e.returnValue).toBeNull(); + done(); }); }); }); describe('error when updating', () => { - beforeEach(() => { - spyOn(window, 'Flash').and.callThrough(); - spyOn(vm.service, 'updateIssuable').and.callFake( - () => - new Promise((resolve, reject) => { - reject(); - }), - ); - }); - it('closes form on error', done => { + spyOn(vm.service, 'updateIssuable').and.callFake(() => Promise.reject()); vm.updateIssuable(); setTimeout(() => { - expect(eventHub.$emit).toHaveBeenCalledWith('close.form'); - - expect(window.Flash).toHaveBeenCalledWith('Error updating issue'); + expect(eventHub.$emit).not.toHaveBeenCalledWith('close.form'); + expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe( + `Error updating issue`, + ); done(); }); }); it('returns the correct error message for issuableType', done => { + spyOn(vm.service, 'updateIssuable').and.callFake(() => Promise.reject()); vm.issuableType = 'merge request'; Vue.nextTick(() => { vm.updateIssuable(); setTimeout(() => { - expect(eventHub.$emit).toHaveBeenCalledWith('close.form'); - - expect(window.Flash).toHaveBeenCalledWith('Error updating merge request'); + expect(eventHub.$emit).not.toHaveBeenCalledWith('close.form'); + expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe( + `Error updating merge request`, + ); done(); }); }); }); + + it('shows error mesage from backend if exists', done => { + const msg = 'Custom error message from backend'; + spyOn(vm.service, 'updateIssuable').and.callFake( + // eslint-disable-next-line prefer-promise-reject-errors + () => Promise.reject({ response: { data: { errors: [msg] } } }), + ); + + vm.updateIssuable(); + setTimeout(() => { + expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe( + `${vm.defaultErrorMessage}. ${msg}`, + ); + + done(); + }); + }); }); }); @@ -342,21 +352,19 @@ describe('Issuable output', () => { describe('deleteIssuable', () => { it('changes URL when deleted', done => { const visitUrl = spyOnDependency(issuableApp, 'visitUrl'); - spyOn(vm.service, 'deleteIssuable').and.callFake( - () => - new Promise(resolve => { - resolve({ - data: { - web_url: '/test', - }, - }); - }), + spyOn(vm.service, 'deleteIssuable').and.returnValue( + Promise.resolve({ + data: { + web_url: '/test', + }, + }), ); vm.deleteIssuable(); setTimeout(() => { expect(visitUrl).toHaveBeenCalledWith('/test'); + done(); }); }); @@ -364,40 +372,33 @@ describe('Issuable output', () => { it('stops polling when deleting', done => { spyOnDependency(issuableApp, 'visitUrl'); spyOn(vm.poll, 'stop').and.callThrough(); - spyOn(vm.service, 'deleteIssuable').and.callFake( - () => - new Promise(resolve => { - resolve({ - data: { - web_url: '/test', - }, - }); - }), + spyOn(vm.service, 'deleteIssuable').and.returnValue( + Promise.resolve({ + data: { + web_url: '/test', + }, + }), ); vm.deleteIssuable(); setTimeout(() => { expect(vm.poll.stop).toHaveBeenCalledWith(); + done(); }); }); it('closes form on error', done => { - spyOn(window, 'Flash').and.callThrough(); - spyOn(vm.service, 'deleteIssuable').and.callFake( - () => - new Promise((resolve, reject) => { - reject(); - }), - ); + spyOn(vm.service, 'deleteIssuable').and.returnValue(Promise.reject()); vm.deleteIssuable(); setTimeout(() => { - expect(eventHub.$emit).toHaveBeenCalledWith('close.form'); - - expect(window.Flash).toHaveBeenCalledWith('Error deleting issue'); + expect(eventHub.$emit).not.toHaveBeenCalledWith('close.form'); + expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe( + 'Error deleting issue', + ); done(); }); @@ -420,6 +421,7 @@ describe('Issuable output', () => { .then(vm.$nextTick) .then(() => { expect(vm.formState.lockedWarningVisible).toEqual(true); + expect(vm.formState.lock_version).toEqual(1); expect(vm.$el.querySelector('.alert')).not.toBeNull(); }) .then(done) @@ -438,4 +440,34 @@ describe('Issuable output', () => { expect(vm.$el.querySelector('.title-container .note-action-button')).toBeDefined(); }); }); + + describe('updateStoreState', () => { + it('should make a request and update the state of the store', done => { + const data = { foo: 1 }; + spyOn(vm.store, 'updateState'); + spyOn(vm.service, 'getData').and.returnValue(Promise.resolve({ data })); + + vm.updateStoreState() + .then(() => { + expect(vm.service.getData).toHaveBeenCalled(); + expect(vm.store.updateState).toHaveBeenCalledWith(data); + }) + .then(done) + .catch(done.fail); + }); + + it('should show error message if store update fails', done => { + spyOn(vm.service, 'getData').and.returnValue(Promise.reject()); + vm.issuableType = 'merge request'; + + vm.updateStoreState() + .then(() => { + expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe( + `Error updating ${vm.issuableType}`, + ); + }) + .then(done) + .catch(done.fail); + }); + }); }); diff --git a/spec/javascripts/issue_show/components/description_spec.js b/spec/javascripts/issue_show/components/description_spec.js index 463f3c89926..72716b97f5f 100644 --- a/spec/javascripts/issue_show/components/description_spec.js +++ b/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'); + }); + }); }); diff --git a/spec/javascripts/issue_show/mock_data.js b/spec/javascripts/issue_show/mock_data.js index 74b3efb014b..f4475aadb8b 100644 --- a/spec/javascripts/issue_show/mock_data.js +++ b/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, }, }; diff --git a/spec/javascripts/merge_request_spec.js b/spec/javascripts/merge_request_spec.js index 1cb49b49ca7..32623d1781a 100644 --- a/spec/javascripts/merge_request_spec.js +++ b/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(); }); }); diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index 694f581150f..7c869d4c326 100644 --- a/spec/javascripts/notes_spec.js +++ b/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(); }); }); diff --git a/spec/javascripts/task_list_spec.js b/spec/javascripts/task_list_spec.js new file mode 100644 index 00000000000..563f402de58 --- /dev/null +++ b/spec/javascripts/task_list_spec.js @@ -0,0 +1,156 @@ +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); + }); +}); diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index ef6af232999..925e2ab0955 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/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 diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index ce20bf2bef6..ef76e2311b1 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/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 } diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb new file mode 100644 index 00000000000..750ac4c40ba --- /dev/null +++ b/spec/services/task_list_toggle_service_spec.rb @@ -0,0 +1,126 @@ +# 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 diff --git a/yarn.lock b/yarn.lock index 5c9139fdbfa..ad21820fe76 100644 --- a/yarn.lock +++ b/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" |