diff options
| -rw-r--r-- | app/models/concerns/cache_markdown_field.rb | 8 | ||||
| -rw-r--r-- | app/services/issuable_base_service.rb | 10 | ||||
| -rw-r--r-- | app/services/task_list_toggle_service.rb | 9 | ||||
| -rw-r--r-- | spec/models/concerns/cache_markdown_field_spec.rb | 24 |
4 files changed, 43 insertions, 8 deletions
diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 0ccc03db15a..588204c7470 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -15,7 +15,7 @@ module CacheMarkdownField # Increment this number every time the renderer changes its output CACHE_REDCARPET_VERSION = 3 CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 13 + CACHE_COMMONMARK_VERSION = 14 # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze @@ -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 diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 5719a4778f0..2835d598311 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -262,16 +262,16 @@ class IssuableBaseService < BaseService # 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(issue) + def update_task_event(issuable) update_task_params = params.delete(:update_task) return unless update_task_params - toggler = TaskListToggleService.new(issue.description, issue.description_html, + toggler = TaskListToggleService.new(issuable.description, issuable.description_html, line_source: update_task_params[:line_source], line_number: update_task_params[:line_number], currently_checked: !update_task_params[:checked], index: update_task_params[:index], - sourcepos: false) + sourcepos: !issuable.legacy_markdown?) if toggler.execute # by updating the description_html field at the same time, @@ -282,9 +282,9 @@ class IssuableBaseService < BaseService # 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] = issue.lock_version + params[:lock_version] = issuable.lock_version - update_task(issue) + update_task(issuable) else # if we make it here, the data is much newer than we thought it was - fail fast raise ActiveRecord::StaleObjectError diff --git a/app/services/task_list_toggle_service.rb b/app/services/task_list_toggle_service.rb index 1b0a5965235..3ba0faa5009 100644 --- a/app/services/task_list_toggle_service.rb +++ b/app/services/task_list_toggle_service.rb @@ -11,7 +11,7 @@ class TaskListToggleService attr_reader :updated_markdown, :updated_markdown_html - def initialize(markdown, markdown_html, line_source:, line_number:, currently_checked:, index:, sourcepos: false) + def initialize(markdown, markdown_html, line_source:, line_number:, currently_checked:, index:, sourcepos: true) @markdown, @markdown_html = markdown, markdown_html @line_source, @line_number = line_source, line_number @currently_checked = currently_checked @@ -62,6 +62,13 @@ class TaskListToggleService @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 diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index a8a5521a5fc..1c2646c60a4 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -251,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 |