Merge pull request #2456 from tf/comment-quote

Preserve the wording each comment refers to

Author: tf

app/controllers/pageflow/review/comment_threads_controller.rb

@@ -19,10 +19,8 @@ def create
         @comment_thread = entry.comment_threads.build(thread_params)
         @comment_thread.creator = current_user
 
-        @comment_thread.comments.build(
-          body: params[:comment_thread][:comment][:body],
-          creator: current_user
-        )
+        first_comment = @comment_thread.comments.build(first_comment_params)
+        first_comment.creator = current_user
 
         @comment_thread.save!
         render :create, status: :created
@@ -51,6 +49,10 @@ def thread_params
         permitted[:subject_range] = params[:comment_thread][:subject_range]&.permit!
         permitted
       end
+
+      def first_comment_params
+        params.require(:comment_thread).require(:comment).permit(:body, :quote)
+      end
     end
   end
 end

app/controllers/pageflow/review/comments_controller.rb

@@ -20,7 +20,7 @@ def create
       private
 
       def comment_params
-        params.require(:comment).permit(:body)
+        params.require(:comment).permit(:body, :quote)
       end
     end
   end

app/models/pageflow/comment.rb

@@ -3,13 +3,28 @@ module Pageflow
   class Comment < ApplicationRecord
     include NestedRevisionComponent
 
+    # Quotes are recorded by the client, so cut rather than reject: an
+    # unexpectedly long selection must not keep a comment from being saved.
+    # Kept in sync with the limit quote extraction applies in
+    # entry_types/scrolled/package/src/review/subjectQuote.js, so that a
+    # recorded quote stays comparable to the text as it reads later on.
+    QUOTE_LIMIT = 4_000
+
     belongs_to :comment_thread
     belongs_to :creator, class_name: 'User'
 
     validates :body, presence: true
 
+    before_validation :truncate_quote
+
     def entry_for_auto_generated_perma_id
       comment_thread.revision.entry
     end
+
+    private
+
+    def truncate_quote
+      self.quote = quote[0, QUOTE_LIMIT] if quote
+    end
   end
 end

app/views/pageflow/review/comments/_comment.json.jbuilder

@@ -5,6 +5,7 @@ json.call(comment,
           :perma_id,
           :creator_id,
           :body,
+          :quote,
           :created_at,
           :updated_at)
 

db/migrate/20260727000000_add_quote_to_comments.rb

@@ -0,0 +1,5 @@
+class AddQuoteToComments < ActiveRecord::Migration[7.1]
+  def change
+    add_column :pageflow_comments, :quote, :text
+  end
+end

entry_types/scrolled/package/spec/contentElements/heading/review-spec.js

@@ -0,0 +1,71 @@
+import 'contentElements/heading/review';
+import {review} from 'review';
+
+describe('contentElements/heading/review', () => {
+  const extractQuote = review.contentElementTypes.findExtractQuote('heading');
+
+  function inlineValue(...texts) {
+    return [{children: texts.map(text => ({text}))}];
+  }
+
+  it('returns the heading text', () => {
+    expect(extractQuote({value: inlineValue('A headline')})).toEqual('A headline');
+  });
+
+  it('joins the text of adjacent marks', () => {
+    expect(extractQuote({value: inlineValue('A ', 'bold', ' headline')}))
+      .toEqual('A bold headline');
+  });
+
+  it('includes tagline and subtitle in reading order', () => {
+    const configuration = {
+      tagline: inlineValue('Tagline'),
+      value: inlineValue('A headline'),
+      subtitle: inlineValue('Subtitle')
+    };
+
+    expect(extractQuote(configuration)).toEqual('Tagline\nA headline\nSubtitle');
+  });
+
+  it('skips blank taglines and subtitles', () => {
+    const configuration = {
+      tagline: inlineValue(''),
+      value: inlineValue('A headline'),
+      subtitle: undefined
+    };
+
+    expect(extractQuote(configuration)).toEqual('A headline');
+  });
+
+  it('falls back to the legacy string value', () => {
+    expect(extractQuote({children: 'A legacy headline'})).toEqual('A legacy headline');
+  });
+
+  it('prefers the inline text value over the legacy string value', () => {
+    const configuration = {
+      value: inlineValue('A headline'),
+      children: 'A legacy headline'
+    };
+
+    expect(extractQuote(configuration)).toEqual('A headline');
+  });
+
+  it('does not fall back to the legacy value once the heading was emptied', () => {
+    const configuration = {
+      value: inlineValue(''),
+      children: 'A legacy headline'
+    };
+
+    expect(extractQuote(configuration)).toBeNull();
+  });
+
+  it('returns null for headings without any text', () => {
+    expect(extractQuote({})).toBeNull();
+  });
+
+  it('ignores a range since heading comments cover the whole element', () => {
+    const range = {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 1}};
+
+    expect(extractQuote({value: inlineValue('A headline')}, range)).toEqual('A headline');
+  });
+});

entry_types/scrolled/package/spec/contentElements/review-spec.js

@@ -0,0 +1,9 @@
+import 'contentElements/review';
+import {review} from 'review';
+
+describe('contentElements/review', () => {
+  it('registers quote extraction of content element types in review api', () => {
+    expect(review.contentElementTypes.findExtractQuote('heading')).toBeDefined();
+    expect(review.contentElementTypes.findExtractQuote('textBlock')).toBeDefined();
+  });
+});

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

@@ -46,4 +46,45 @@ describe('contentElements/textBlock/review', () => {
     expect(compareRanges(r, undefined)).toBeLessThan(0);
     expect(compareRanges(undefined, undefined)).toBe(0);
   });
+
+  describe('extractQuote', () => {
+    const extractQuote = review.contentElementTypes.findExtractQuote('textBlock');
+
+    const value = [
+      {type: 'paragraph', children: [{text: 'The quick brown fox'}]},
+      {type: 'paragraph', children: [{text: 'jumps over the lazy dog'}]}
+    ];
+
+    it('returns the text covered by the range', () => {
+      expect(extractQuote({value}, range([0, 0, 4], [0, 0, 15]))).toEqual('quick brown');
+    });
+
+    it('joins the text of blocks the range spans', () => {
+      expect(extractQuote({value}, range([0, 0, 16], [1, 0, 5]))).toEqual('fox\njumps');
+    });
+
+    it('handles ranges whose focus precedes their anchor', () => {
+      expect(extractQuote({value}, range([0, 0, 15], [0, 0, 4]))).toEqual('quick brown');
+    });
+
+    it('trims surrounding whitespace', () => {
+      expect(extractQuote({value}, range([0, 0, 3], [0, 0, 16]))).toEqual('quick brown');
+    });
+
+    it('returns null for ranges pointing past the end of the value', () => {
+      expect(extractQuote({value}, range([5, 0, 0], [5, 0, 3]))).toBeNull();
+    });
+
+    it('returns null for collapsed ranges', () => {
+      expect(extractQuote({value}, range([0, 0, 4], [0, 0, 4]))).toBeNull();
+    });
+
+    it('returns null for content elements without a value', () => {
+      expect(extractQuote({}, range([0, 0, 0], [0, 0, 3]))).toBeNull();
+    });
+
+    it('returns null for element wide comments without a range', () => {
+      expect(extractQuote({value})).toBeNull();
+    });
+  });
 });

entry_types/scrolled/package/spec/review/Thread-spec.js

@@ -3,6 +3,7 @@ import '@testing-library/jest-dom/extend-expect';
 import {useFakeTranslations} from 'pageflow/testHelpers';
 
 import {Thread} from 'review/Thread';
+import {review} from 'review/api';
 import {renderWithReviewState} from 'support/renderWithReviewState';
 
 describe('Thread', () => {
@@ -42,4 +43,131 @@ describe('Thread', () => {
     expect(hint.compareDocumentPosition(comment) & Node.DOCUMENT_POSITION_FOLLOWING)
       .toBeTruthy();
   });
+
+  describe('comment quotes', () => {
+    const seed = {
+      sections: [{id: 1, permaId: 1}],
+      contentElements: [
+        {
+          id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock',
+          configuration: {value: 'Current wording'}
+        }
+      ]
+    };
+
+    const quotingThread = {
+      ...thread,
+      subjectType: 'ContentElement',
+      subjectId: 10,
+      subjectRange: {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 7}}
+    };
+
+    function threadWithQuotes(...quotes) {
+      return {
+        ...quotingThread,
+        comments: quotes.map((quote, index) => ({
+          id: 10 + index,
+          body: `Comment ${index + 1}`,
+          creatorName: 'Bob',
+          creatorId: 2,
+          quote
+        }))
+      };
+    }
+
+    beforeEach(() => {
+      review.contentElementTypes.register('textBlock', {
+        extractQuote: configuration => configuration.value
+      });
+    });
+
+    afterEach(() => {
+      review.contentElementTypes.types = {};
+    });
+
+    it('renders a quote for a comment whose text has since changed', () => {
+      const {getByText} = renderWithReviewState(
+        <Thread thread={threadWithQuotes('Original wording')} interactive={false} />,
+        {seed}
+      );
+
+      expect(getByText('Original wording')).toBeInTheDocument();
+    });
+
+    it('renders no quote while the text still reads the same', () => {
+      const {container} = renderWithReviewState(
+        <Thread thread={threadWithQuotes('Current wording')} interactive={false} />,
+        {seed}
+      );
+
+      expect(container.querySelector('blockquote')).toBeNull();
+    });
+
+    it('renders the quote inside the comment, above its body', () => {
+      const {getByText} = renderWithReviewState(
+        <Thread thread={threadWithQuotes('Original wording')} interactive={false} />,
+        {seed}
+      );
+
+      const quote = getByText('Original wording');
+      const body = getByText('Comment 1');
+
+      expect(quote.parentNode).toBe(body.parentNode);
+      expect(quote.compareDocumentPosition(body) & Node.DOCUMENT_POSITION_FOLLOWING)
+        .toBeTruthy();
+    });
+
+    it('renders a quote per version once the text changes mid thread', () => {
+      const {getByText} = renderWithReviewState(
+        <Thread thread={threadWithQuotes('First wording', 'Second wording')}
+                interactive={false} />,
+        {seed}
+      );
+
+      expect(getByText('First wording')).toBeInTheDocument();
+      expect(getByText('Second wording')).toBeInTheDocument();
+    });
+
+    it('renders the quote once for a run of replies about the same wording', () => {
+      const {queryAllByText} = renderWithReviewState(
+        <Thread thread={threadWithQuotes('Same wording', 'Same wording')}
+                interactive={false} />,
+        {seed}
+      );
+
+      expect(queryAllByText('Same wording')).toHaveLength(1);
+    });
+
+    it('renders no quote for the last comment when it matches the current text', () => {
+      const {getByText, queryByText} = renderWithReviewState(
+        <Thread thread={threadWithQuotes('Original wording', 'Current wording')}
+                interactive={false} />,
+        {seed}
+      );
+
+      expect(getByText('Original wording')).toBeInTheDocument();
+      expect(queryByText('Current wording')).not.toBeInTheDocument();
+    });
+
+    it('renders quotes for threads whose content element was deleted', () => {
+      const {getByText} = renderWithReviewState(
+        <Thread thread={{...threadWithQuotes('Original wording'),
+                         subjectId: 999,
+                         orphaned: true}}
+                interactive={false} />,
+        {seed}
+      );
+
+      expect(getByText('Original wording')).toBeInTheDocument();
+    });
+
+    it('renders no quote for comments recorded without one', () => {
+      const {container} = renderWithReviewState(
+        <Thread thread={quotingThread} interactive={false} />,
+        {seed}
+      );
+
+      expect(container.querySelector('blockquote')).toBeNull();
+    });
+  });
 });