Merge pull request #2410 from tf/split-merge-comment-ranges

Split merge comment ranges

Author: tf

app/models/pageflow/comment_thread.rb

@@ -21,6 +21,14 @@ def unresolve
       update!(resolved_at: nil, resolver: nil)
     end
 
+    def self.migrate_to_subject(revision:, subject_type:, subject_id:, thread_ids:)
+      return if thread_ids.blank?
+
+      where(revision_id: revision.id,
+            subject_type:,
+            id: thread_ids).update_all(subject_id:, updated_at: Time.current)
+    end
+
     def self.update_subject_ranges_for(revision:, subject_type:, subject_id:, ranges:)
       return if ranges.blank?
 

entry_types/scrolled/app/controllers/pageflow_scrolled/editor/content_elements_controller.rb

@@ -17,10 +17,15 @@ def batch
         section = Section.all_for_revision(@entry.draft).find(params[:section_id])
 
         items = params.require(:content_elements).map do |item|
-          item.transform_keys(&:underscore).permit(:id, :type_name, :_delete, configuration: {})
+          item.transform_keys(&:underscore).permit(:id, :type_name, :_delete,
+                                                   configuration: {},
+                                                   migrate_comment_threads: [])
         end
 
-        @content_elements = ContentElement::Batch.new(section, items).save!
+        @content_elements = ContentElement::Batch.new(
+          section, items,
+          comment_thread_subject_ranges: permitted_subject_ranges
+        ).save!
       rescue ActiveRecord::RecordNotFound
         head :not_found
       end

entry_types/scrolled/app/models/pageflow_scrolled/content_element.rb

@@ -33,22 +33,26 @@ def self.select_used_type_names(revision, type_names)
 
     # @api private
     class Batch
-      def initialize(section, items)
+      def initialize(section, items, comment_thread_subject_ranges: nil)
         @section = section
         @storyline = section.chapter.storyline
         @items = items
+        @comment_thread_subject_ranges = comment_thread_subject_ranges
       end
 
       def save!
         ContentElement.transaction do
-          @items.map.with_index { |item, index|
+          result = @items.map.with_index { |item, index|
             if item[:_delete]
               @section.content_elements.delete(item[:id])
               nil
             else
               create_or_update(item, index)
             end
           }.compact
+
+          update_subject_ranges
+          result
         end
       end
 
@@ -60,11 +64,34 @@ def create_or_update(item, index)
           position: index
         }.merge(item.slice(:type_name, :configuration))
 
-        if item[:id]
-          @storyline.content_elements.update(item[:id], attributes)
-        else
-          @section.content_elements.create(attributes)
-        end
+        content_element = if item[:id]
+                            @storyline.content_elements.update(item[:id], attributes)
+                          else
+                            @section.content_elements.create(attributes)
+                          end
+
+        migrate_comment_threads(content_element, item[:migrate_comment_threads])
+        content_element
+      end
+
+      def migrate_comment_threads(content_element, thread_ids)
+        Pageflow::CommentThread.migrate_to_subject(
+          revision: @storyline.revision,
+          subject_type: 'ContentElement',
+          subject_id: content_element.perma_id,
+          thread_ids:
+        )
+      end
+
+      def update_subject_ranges
+        return if @comment_thread_subject_ranges.blank?
+
+        Pageflow::CommentThread.update_subject_ranges_for(
+          revision: @storyline.revision,
+          subject_type: 'ContentElement',
+          subject_id: @section.content_elements.pluck(:perma_id),
+          ranges: @comment_thread_subject_ranges
+        )
       end
     end
   end

entry_types/scrolled/package/spec/contentElements/textBlock/editor-spec.js

@@ -0,0 +1,131 @@
+import 'contentElements/textBlock/editor';
+import {editor} from 'editor';
+
+describe('contentElements/textBlock/editor', () => {
+  const type = editor.contentElementTypes.findByTypeName('textBlock');
+
+  function paragraph(text) {
+    return {type: 'paragraph', children: [{text}]};
+  }
+
+  describe('#split', () => {
+    it('splits configuration at the given block index', () => {
+      const configuration = {value: [paragraph('first'), paragraph('second')]};
+
+      const {before, after} = type.split(configuration, 1, {ranges: {}});
+
+      expect(before.configuration.value).toEqual([paragraph('first')]);
+      expect(after.configuration.value).toEqual([paragraph('second')]);
+    });
+
+    it('leaves thread on the before side when range is fully before split', () => {
+      const configuration = {
+        value: [paragraph('first'), paragraph('second'), paragraph('third')]
+      };
+      const ranges = {
+        7: {anchor: {path: [0, 0], offset: 0},
+            focus: {path: [0, 0], offset: 3}}
+      };
+
+      const {before, after} = type.split(configuration, 2, {ranges});
+
+      expect(before.ranges).toEqual(ranges);
+      expect(after.ranges).toEqual({});
+    });
+
+    it('rebases thread on the after side when range is fully after split', () => {
+      const configuration = {
+        value: [paragraph('first'), paragraph('second'), paragraph('third')]
+      };
+      const ranges = {
+        7: {anchor: {path: [2, 0], offset: 0},
+            focus: {path: [2, 0], offset: 3}}
+      };
+
+      const {before, after} = type.split(configuration, 2, {ranges});
+
+      expect(before.ranges).toEqual({});
+      expect(after.ranges).toEqual({
+        7: {anchor: {path: [0, 0], offset: 0},
+            focus: {path: [0, 0], offset: 3}}
+      });
+    });
+
+    it('clamps thread straddling the split point to the end of last before-block', () => {
+      const configuration = {
+        value: [paragraph('first'), paragraph('second'), paragraph('third')]
+      };
+      const ranges = {
+        7: {anchor: {path: [0, 0], offset: 1},
+            focus: {path: [2, 0], offset: 2}}
+      };
+
+      const {before, after} = type.split(configuration, 2, {ranges});
+
+      expect(before.ranges).toEqual({
+        7: {anchor: {path: [0, 0], offset: 1},
+            focus: {path: [1, 0], offset: 'second'.length}}
+      });
+      expect(after.ranges).toEqual({});
+    });
+  });
+
+  describe('#merge', () => {
+    it('concatenates values of both configurations', () => {
+      const configA = {value: [paragraph('a')]};
+      const configB = {value: [paragraph('b')]};
+
+      const {configuration} = type.merge(configA, configB,
+                                         {rangesA: {}, rangesB: {}});
+
+      expect(configuration.value).toEqual([paragraph('a'), paragraph('b')]);
+    });
+
+    it('keeps A-side ranges unchanged', () => {
+      const configA = {value: [paragraph('one'), paragraph('two')]};
+      const configB = {value: [paragraph('three')]};
+      const rangesA = {
+        7: {anchor: {path: [0, 0], offset: 0},
+            focus: {path: [0, 0], offset: 3}}
+      };
+
+      const {ranges} = type.merge(configA, configB, {rangesA, rangesB: {}});
+
+      expect(ranges).toEqual(rangesA);
+    });
+
+    it('shifts B-side ranges by the length of A', () => {
+      const configA = {value: [paragraph('one'), paragraph('two')]};
+      const configB = {value: [paragraph('three'), paragraph('four')]};
+      const rangesB = {
+        8: {anchor: {path: [0, 0], offset: 0},
+            focus: {path: [1, 0], offset: 4}}
+      };
+
+      const {ranges} = type.merge(configA, configB, {rangesA: {}, rangesB});
+
+      expect(ranges).toEqual({
+        8: {anchor: {path: [2, 0], offset: 0},
+            focus: {path: [3, 0], offset: 4}}
+      });
+    });
+
+    it('combines both sides when both have ranges', () => {
+      const configA = {value: [paragraph('one')]};
+      const configB = {value: [paragraph('two')]};
+      const rangesA = {
+        7: {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 3}}
+      };
+      const rangesB = {
+        8: {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 3}}
+      };
+
+      const {ranges} = type.merge(configA, configB, {rangesA, rangesB});
+
+      expect(ranges).toEqual({
+        7: {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 3}},
+        8: {anchor: {path: [1, 0], offset: 0}, focus: {path: [1, 0], offset: 3}}
+      });
+    });
+  });
+});

entry_types/scrolled/package/spec/editor/models/ScrolledEntry/deleteContentElement-spec.js

@@ -256,5 +256,64 @@ describe('ScrolledEntry', () => {
         });
       });
     });
+
+    describe('for range-aware content elements with comment threads', () => {
+      beforeEach(() => {
+        editor.contentElementTypes.register('rangeAware', {
+          getLength(configuration) {
+            return configuration.items.length;
+          },
+
+          merge(configurationA, configurationB, {rangesA, rangesB}) {
+            const offset = configurationA.items.length;
+            const ranges = {...rangesA};
+            Object.entries(rangesB).forEach(([id, {start, end}]) => {
+              ranges[id] = {start: start + offset, end: end + offset};
+            });
+            return {
+              configuration: {items: configurationA.items.concat(configurationB.items)},
+              ranges
+            };
+          }
+        });
+
+        testContext.entry = factories.entry(ScrolledEntry, {id: 100}, {
+          entryTypeSeed: normalizeSeed({
+            contentElements: [
+              {id: 4, permaId: 40, position: 0, typeName: 'rangeAware',
+               configuration: {items: ['a', 'b']}},
+              {id: 5, permaId: 50, position: 1},
+              {id: 6, permaId: 60, position: 2, typeName: 'rangeAware',
+               configuration: {items: ['c', 'd']}}
+            ]
+          })
+        });
+
+        testContext.entry.reviewSession = factories.reviewSession({
+          commentThreads: [
+            {id: 7, subjectType: 'ContentElement', subjectId: 40,
+             subjectRange: {start: 0, end: 1}, comments: []},
+            {id: 8, subjectType: 'ContentElement', subjectId: 60,
+             subjectRange: {start: 0, end: 1}, comments: []}
+          ]
+        });
+      });
+
+      setupGlobals({entry: () => testContext.entry});
+
+      it('migrates threads from the removed block onto the surviving merged block', () => {
+        const {entry, requests} = testContext;
+
+        entry.deleteContentElement(entry.contentElements.get(5));
+
+        const body = JSON.parse(requests[0].requestBody);
+        const survivor = body.content_elements.find(item => item.id === 4);
+
+        expect(survivor.migrate_comment_threads).toEqual([8]);
+        expect(body.comment_thread_subject_ranges).toEqual({
+          8: {start: 2, end: 3}
+        });
+      });
+    });
   });
 });

entry_types/scrolled/package/spec/editor/models/ScrolledEntry/insertContentElement-spec.js

@@ -886,5 +886,79 @@ describe('ScrolledEntry', () => {
         );
       });
     });
+
+    describe('for range-aware content elements with comment threads', () => {
+      beforeEach(() => {
+        editor.contentElementTypes.register('rangeAware', {
+          getLength(configuration) {
+            return configuration.items.length;
+          },
+
+          split(configuration, at, {ranges}) {
+            const beforeRanges = {};
+            const afterRanges = {};
+            Object.entries(ranges).forEach(([id, {start, end}]) => {
+              if (end <= at) {
+                beforeRanges[id] = {start, end};
+              }
+              else if (start >= at) {
+                afterRanges[id] = {start: start - at, end: end - at};
+              }
+              else {
+                beforeRanges[id] = {start, end: at};
+              }
+            });
+            return {
+              before: {
+                configuration: {items: configuration.items.slice(0, at)},
+                ranges: beforeRanges
+              },
+              after: {
+                configuration: {items: configuration.items.slice(at)},
+                ranges: afterRanges
+              }
+            };
+          }
+        });
+
+        testContext.entry = factories.entry(ScrolledEntry, {id: 100}, {
+          entryTypeSeed: normalizeSeed({
+            contentElements: [
+              {id: 5, permaId: 50, position: 0, typeName: 'rangeAware',
+               configuration: {items: ['a', 'b', 'c']}}
+            ]
+          })
+        });
+
+        testContext.entry.reviewSession = factories.reviewSession({
+          commentThreads: [
+            {id: 7, subjectType: 'ContentElement', subjectId: 50,
+             subjectRange: {start: 0, end: 1}, comments: []},
+            {id: 8, subjectType: 'ContentElement', subjectId: 50,
+             subjectRange: {start: 2, end: 3}, comments: []}
+          ]
+        });
+      });
+
+      setupGlobals({entry: () => testContext.entry});
+
+      it('migrates after-split threads onto the new split-off element', () => {
+        const {entry, requests} = testContext;
+
+        entry.insertContentElement({typeName: 'inlineImage'},
+                                   {at: 'split', id: 5, splitPoint: 2});
+
+        const body = JSON.parse(requests[0].requestBody);
+        const splitOff = body.content_elements.find(
+          item => item.typeName === 'rangeAware' && !item.id
+        );
+
+        expect(splitOff).toBeDefined();
+        expect(splitOff.migrate_comment_threads).toEqual([8]);
+        expect(body.comment_thread_subject_ranges).toEqual({
+          8: {start: 0, end: 1}
+        });
+      });
+    });
   });
 });