From bb8fdcc17700e029e2a5bb7e195b07b9550b4e34 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Thu, 31 Jan 2019 09:33:38 -0600 Subject: [PATCH] Address review comments --- app/services/issuable_base_service.rb | 35 +-- app/services/task_list_toggle_service.rb | 2 +- .../concerns/cache_markdown_field_spec.rb | 2 +- .../services/task_list_toggle_service_spec.rb | 214 +++++++++--------- 4 files changed, 126 insertions(+), 127 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 93dd6893e42..842d59d26a0 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -239,8 +239,9 @@ class IssuableBaseService < BaseService filter_params(issuable) if issuable.changed? || params.present? - issuable.assign_attributes(params.merge(updated_by: current_user)) - issuable.assign_attributes(last_edited_at: Time.now, last_edited_by: current_user) + issuable.assign_attributes(params.merge(updated_by: current_user, + last_edited_at: Time.now, + last_edited_by: current_user)) before_update(issuable) @@ -268,27 +269,27 @@ class IssuableBaseService < BaseService tasklist_toggler = TaskListToggleService.new(issuable.description, issuable.description_html, line_source: update_task_params[:line_source], - line_number: update_task_params[:line_number], + line_number: update_task_params[:line_number].to_i, toggle_as_checked: update_task_params[:checked], - index: update_task_params[:index], + index: update_task_params[:index].to_i, sourcepos: !issuable.legacy_markdown?) - if tasklist_toggler.execute - # 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) - else + 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) diff --git a/app/services/task_list_toggle_service.rb b/app/services/task_list_toggle_service.rb index 92f42a92c6b..2717fc9035a 100644 --- a/app/services/task_list_toggle_service.rb +++ b/app/services/task_list_toggle_service.rb @@ -23,7 +23,7 @@ class TaskListToggleService def execute return false unless markdown && markdown_html - !!(toggle_markdown && toggle_markdown_html) + toggle_markdown && toggle_markdown_html end private diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 1c2646c60a4..925e2ab0955 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -133,7 +133,7 @@ describe CacheMarkdownField do end end - context 'when a markdown field and html field are both set' do + 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!_' diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb index 6b92d4e14d0..750ac4c40ba 100644 --- a/spec/services/task_list_toggle_service_spec.rb +++ b/spec/services/task_list_toggle_service_spec.rb @@ -3,126 +3,124 @@ require 'spec_helper' describe TaskListToggleService do - context 'when ' do - let(:sourcepos) { true } - let(:markdown) do - <<-EOT.strip_heredoc - * [ ] Task 1 - * [x] Task 2 + let(:sourcepos) { true } + let(:markdown) do + <<-EOT.strip_heredoc + * [ ] Task 1 + * [x] Task 2 - A paragraph + A paragraph - 1. [X] Item 1 - - [ ] Sub-item 1 - EOT + 1. [X] Item 1 + - [ ] Sub-item 1 + EOT + end + + let(:markdown_html) do + <<-EOT.strip_heredoc + +

A paragraph

+
    +
  1. + Item 1 +
      +
    • + Sub-item 1 +
      • +
    +
    2. +
+ 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 - let(:markdown_html) do - <<-EOT.strip_heredoc - -

A paragraph

-
    -
  1. - Item 1 -
      -
    • - Sub-item 1 -
      • -
    -
    2. -
- EOT + 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 - 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) + 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_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 + expect(toggler.execute).to be_falsey end - context 'when using sourcepos' do - it_behaves_like 'task lists' + 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 - context 'when using checkbox indexing' do - let(:sourcepos) { false } - let(:markdown_html) do - <<-EOT.strip_heredoc - -

A paragraph

-
    -
  1. - Item 1 -
      -
    • - Sub-item 1 -
      • -
    -
    2. -
- EOT - 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) - it_behaves_like 'task lists' + 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 + +

A paragraph

+
    +
  1. + Item 1 +
      +
    • + Sub-item 1 +
      • +
    +
    2. +
+ EOT + end + + it_behaves_like 'task lists' + end end