Address review comments

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)

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

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!_'

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