From 53337e4a49065ff858dd973c3c4e02f9ceaa1bd6 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Mon, 27 Jul 2026 16:46:57 +0200 Subject: [PATCH] Preserve the wording each comment refers to Editing text after a comment was written left the conversation without a point of reference: nothing showed what the wording had been when someone objected to it. Longer threads made this worse, since later replies can refer to yet another intermediate version. Each comment now keeps the wording it responded to and shows it once the text has changed, so threads stay readable after the fact. Comments on text that still reads the same show nothing, leaving the common case as compact as before. Available for text blocks, where a comment covers a selected range, and for headings, where it covers the whole element. Comments written earlier have no recorded wording and show none. REDMINE-21261 --- .../review/comment_threads_controller.rb | 10 +- .../pageflow/review/comments_controller.rb | 2 +- app/models/pageflow/comment.rb | 15 ++ .../review/comments/_comment.json.jbuilder | 1 + .../20260727000000_add_quote_to_comments.rb | 5 + .../contentElements/heading/review-spec.js | 71 ++++++++++ .../spec/contentElements/review-spec.js | 9 ++ .../contentElements/textBlock/review-spec.js | 41 ++++++ .../package/spec/review/Thread-spec.js | 128 ++++++++++++++++++ .../package/spec/review/ThreadList-spec.js | 88 +++++++++++- .../spec/review/outdatedQuotes-spec.js | 67 +++++++++ .../package/spec/review/subjectQuote-spec.js | 105 ++++++++++++++ .../src/contentElements/heading/review.js | 30 ++++ .../package/src/contentElements/review.js | 1 + .../src/contentElements/textBlock/review.js | 19 ++- .../scrolled/package/src/review/Comment.js | 4 +- .../package/src/review/Comment.module.css | 11 ++ .../package/src/review/NewThreadForm.js | 9 +- .../scrolled/package/src/review/ReplyForm.js | 9 +- .../scrolled/package/src/review/Thread.js | 23 +++- .../review/api/ContentElementTypeRegistry.js | 10 +- .../package/src/review/outdatedQuotes.js | 18 +++ .../package/src/review/postMessage.js | 10 +- .../package/src/review/subjectQuote.js | 31 +++++ package/spec/review/ReviewSession-spec.js | 119 ++++++++++++++++ package/src/review/ReviewSession.js | 10 +- .../review/comment_threads_controller_spec.rb | 38 ++++++ .../review/comments_controller_spec.rb | 17 +++ spec/models/pageflow/comment_spec.rb | 31 +++++ 29 files changed, 906 insertions(+), 26 deletions(-) create mode 100644 db/migrate/20260727000000_add_quote_to_comments.rb create mode 100644 entry_types/scrolled/package/spec/contentElements/heading/review-spec.js create mode 100644 entry_types/scrolled/package/spec/contentElements/review-spec.js create mode 100644 entry_types/scrolled/package/spec/review/outdatedQuotes-spec.js create mode 100644 entry_types/scrolled/package/spec/review/subjectQuote-spec.js create mode 100644 entry_types/scrolled/package/src/contentElements/heading/review.js create mode 100644 entry_types/scrolled/package/src/review/outdatedQuotes.js create mode 100644 entry_types/scrolled/package/src/review/subjectQuote.js create mode 100644 spec/models/pageflow/comment_spec.rb diff --git a/app/controllers/pageflow/review/comment_threads_controller.rb b/app/controllers/pageflow/review/comment_threads_controller.rb index 62174e5873..b579f968ee 100644 --- a/app/controllers/pageflow/review/comment_threads_controller.rb +++ b/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 diff --git a/app/controllers/pageflow/review/comments_controller.rb b/app/controllers/pageflow/review/comments_controller.rb index 621b7372dc..4d02801dc8 100644 --- a/app/controllers/pageflow/review/comments_controller.rb +++ b/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 diff --git a/app/models/pageflow/comment.rb b/app/models/pageflow/comment.rb index 6ba052bc05..11761314fd 100644 --- a/app/models/pageflow/comment.rb +++ b/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 diff --git a/app/views/pageflow/review/comments/_comment.json.jbuilder b/app/views/pageflow/review/comments/_comment.json.jbuilder index 612cada537..a9f8966c6f 100644 --- a/app/views/pageflow/review/comments/_comment.json.jbuilder +++ b/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) diff --git a/db/migrate/20260727000000_add_quote_to_comments.rb b/db/migrate/20260727000000_add_quote_to_comments.rb new file mode 100644 index 0000000000..ed8e23a85f --- /dev/null +++ b/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 diff --git a/entry_types/scrolled/package/spec/contentElements/heading/review-spec.js b/entry_types/scrolled/package/spec/contentElements/heading/review-spec.js new file mode 100644 index 0000000000..732d246aa3 --- /dev/null +++ b/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'); + }); +}); diff --git a/entry_types/scrolled/package/spec/contentElements/review-spec.js b/entry_types/scrolled/package/spec/contentElements/review-spec.js new file mode 100644 index 0000000000..fca6e8831a --- /dev/null +++ b/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(); + }); +}); diff --git a/entry_types/scrolled/package/spec/contentElements/textBlock/review-spec.js b/entry_types/scrolled/package/spec/contentElements/textBlock/review-spec.js index 6ac080a81f..dedcdd7151 100644 --- a/entry_types/scrolled/package/spec/contentElements/textBlock/review-spec.js +++ b/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(); + }); + }); }); diff --git a/entry_types/scrolled/package/spec/review/Thread-spec.js b/entry_types/scrolled/package/spec/review/Thread-spec.js index d80a9b9bdb..56843ee2d9 100644 --- a/entry_types/scrolled/package/spec/review/Thread-spec.js +++ b/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( + , + {seed} + ); + + expect(getByText('Original wording')).toBeInTheDocument(); + }); + + it('renders no quote while the text still reads the same', () => { + const {container} = renderWithReviewState( + , + {seed} + ); + + expect(container.querySelector('blockquote')).toBeNull(); + }); + + it('renders the quote inside the comment, above its body', () => { + const {getByText} = renderWithReviewState( + , + {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( + , + {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( + , + {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( + , + {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( + , + {seed} + ); + + expect(getByText('Original wording')).toBeInTheDocument(); + }); + + it('renders no quote for comments recorded without one', () => { + const {container} = renderWithReviewState( + , + {seed} + ); + + expect(container.querySelector('blockquote')).toBeNull(); + }); + }); }); diff --git a/entry_types/scrolled/package/spec/review/ThreadList-spec.js b/entry_types/scrolled/package/spec/review/ThreadList-spec.js index 67b7ac5f1c..e5cf50009f 100644 --- a/entry_types/scrolled/package/spec/review/ThreadList-spec.js +++ b/entry_types/scrolled/package/spec/review/ThreadList-spec.js @@ -4,6 +4,7 @@ import userEvent from '@testing-library/user-event'; import {useFakeTranslations} from 'pageflow/testHelpers'; import {ThreadList} from 'review/ThreadList'; +import {review} from 'review/api'; import {ScrollHighlightedThreadIntoViewProvider} from 'review/scrollHighlightedThreadIntoView'; import {renderWithReviewState} from 'support/renderWithReviewState'; @@ -36,6 +37,10 @@ describe('ThreadList', () => { 'pageflow_scrolled.review.refers_to_deleted_element': 'Refers to a deleted element' }); + afterEach(() => { + review.contentElementTypes.types = {}; + }); + it('displays comments of threads for the subject', () => { const {getByText} = renderThreadList( , @@ -431,6 +436,44 @@ describe('ThreadList', () => { postMessage.mockRestore(); }); + it('includes the quote of the selected range in create thread message', async () => { + const user = userEvent.setup(); + const postMessage = jest.spyOn(window.top, 'postMessage').mockImplementation(() => {}); + + review.contentElementTypes.register('textBlock', { + extractQuote: configuration => configuration.value + }); + + const {getByPlaceholderText, getByRole} = renderWithReviewState( + , + { + seed: { + sections: [{id: 1, permaId: 1}], + contentElements: [{ + id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock', + configuration: {value: 'Quoted text'} + }] + } + } + ); + + await user.type(getByPlaceholderText('Add a comment...'), 'New thread'); + await user.click(getByRole('button', {name: 'Send'})); + + expect(postMessage).toHaveBeenCalledWith( + { + type: 'CREATE_COMMENT_THREAD', + payload: expect.objectContaining({quote: 'Quoted text'}) + }, + window.location.origin + ); + + postMessage.mockRestore(); + }); + it('sends the section perma id itself for section subjects', async () => { const user = userEvent.setup(); const postMessage = jest.spyOn(window.top, 'postMessage').mockImplementation(() => {}); @@ -568,7 +611,50 @@ describe('ThreadList', () => { expect(postMessage).toHaveBeenCalledWith( { type: 'CREATE_COMMENT', - payload: {threadId: 1, body: 'My reply'} + payload: expect.objectContaining({threadId: 1, body: 'My reply'}) + }, + window.location.origin + ); + + postMessage.mockRestore(); + }); + + it('includes the quote of the thread range in create comment message', async () => { + const user = userEvent.setup(); + const postMessage = jest.spyOn(window.top, 'postMessage').mockImplementation(() => {}); + + review.contentElementTypes.register('textBlock', { + extractQuote: configuration => configuration.value + }); + + const subjectRange = {anchor: {path: [0, 0], offset: 0}, + focus: {path: [0, 0], offset: 6}}; + + const {getByPlaceholderText, getByRole} = renderWithReviewState( + , + { + seed: { + sections: [{id: 1, permaId: 1}], + contentElements: [{ + id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock', + configuration: {value: 'Quoted text'} + }] + }, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, subjectRange, comments: [ + {id: 10, body: 'Start', creatorName: 'Bob', creatorId: 2} + ]} + ] + } + ); + + await user.type(getByPlaceholderText('Reply...'), 'My reply'); + await user.click(getByRole('button', {name: 'Send'})); + + expect(postMessage).toHaveBeenCalledWith( + { + type: 'CREATE_COMMENT', + payload: expect.objectContaining({quote: 'Quoted text'}) }, window.location.origin ); diff --git a/entry_types/scrolled/package/spec/review/outdatedQuotes-spec.js b/entry_types/scrolled/package/spec/review/outdatedQuotes-spec.js new file mode 100644 index 0000000000..8f6d11b53f --- /dev/null +++ b/entry_types/scrolled/package/spec/review/outdatedQuotes-spec.js @@ -0,0 +1,67 @@ +import {commentsWithOutdatedQuote} from 'review/outdatedQuotes'; + +describe('commentsWithOutdatedQuote', () => { + function ids(comments, currentQuote) { + return [...commentsWithOutdatedQuote(comments, currentQuote)]; + } + + it('picks comments whose quote differs from the current text', () => { + const comments = [{id: 1, quote: 'old wording'}]; + + expect(ids(comments, 'new wording')).toEqual([1]); + }); + + it('skips comments whose quote still matches the current text', () => { + const comments = [{id: 1, quote: 'same wording'}]; + + expect(ids(comments, 'same wording')).toEqual([]); + }); + + it('only marks the first of consecutive comments sharing a quote', () => { + const comments = [ + {id: 1, quote: 'first wording'}, + {id: 2, quote: 'first wording'}, + {id: 3, quote: 'second wording'} + ]; + + expect(ids(comments, 'current wording')).toEqual([1, 3]); + }); + + it('skips a changed quote that matches the current text', () => { + const comments = [ + {id: 1, quote: 'old wording'}, + {id: 2, quote: 'current wording'} + ]; + + expect(ids(comments, 'current wording')).toEqual([1]); + }); + + it('marks a quote returning to an earlier wording after an intermediate one', () => { + const comments = [ + {id: 1, quote: 'old wording'}, + {id: 2, quote: 'current wording'}, + {id: 3, quote: 'old wording'} + ]; + + expect(ids(comments, 'current wording')).toEqual([1, 3]); + }); + + it('marks every quote when the text is gone', () => { + const comments = [ + {id: 1, quote: 'first wording'}, + {id: 2, quote: 'second wording'} + ]; + + expect(ids(comments, null)).toEqual([1, 2]); + }); + + it('ignores comments recorded without a quote', () => { + const comments = [ + {id: 1}, + {id: 2, quote: 'some wording'}, + {id: 3} + ]; + + expect(ids(comments, 'current wording')).toEqual([2]); + }); +}); diff --git a/entry_types/scrolled/package/spec/review/subjectQuote-spec.js b/entry_types/scrolled/package/spec/review/subjectQuote-spec.js new file mode 100644 index 0000000000..ac16083c05 --- /dev/null +++ b/entry_types/scrolled/package/spec/review/subjectQuote-spec.js @@ -0,0 +1,105 @@ +import {renderHookInEntry} from 'pageflow-scrolled/testHelpers'; + +import {review} from 'review/api'; +import {useSubjectQuote} from 'review/subjectQuote'; + +describe('useSubjectQuote', () => { + const seed = { + sections: [{id: 1, permaId: 1}], + contentElements: [ + { + id: 1, permaId: 10, sectionId: 1, typeName: 'textBlock', + configuration: {value: 'Some text'} + }, + {id: 2, permaId: 11, sectionId: 1, typeName: 'image'}, + { + id: 3, permaId: 12, sectionId: 1, typeName: 'heading', + configuration: {value: 'A headline'} + } + ] + }; + + const subjectRange = { + anchor: {path: [0, 0], offset: 0}, + focus: {path: [0, 0], offset: 4} + }; + + beforeEach(() => { + review.contentElementTypes.register('textBlock', { + extractQuote: (configuration, range) => + (range ? `${configuration.value}@${range.focus.offset}` : null) + }); + + review.contentElementTypes.register('heading', { + extractQuote: configuration => configuration.value + }); + }); + + afterEach(() => { + review.contentElementTypes.types = {}; + }); + + function renderSubjectQuote(options) { + return renderHookInEntry(() => useSubjectQuote(options), {seed}); + } + + it("passes configuration and range to the content element type's extractQuote", () => { + const {result} = renderSubjectQuote({ + subjectType: 'ContentElement', subjectId: 10, subjectRange + }); + + expect(result.current).toEqual('Some text@4'); + }); + + it('lets range-less types extract a quote without a subject range', () => { + const {result} = renderSubjectQuote({ + subjectType: 'ContentElement', subjectId: 12 + }); + + expect(result.current).toEqual('A headline'); + }); + + it('lets ranged types skip subjects without a range', () => { + const {result} = renderSubjectQuote({ + subjectType: 'ContentElement', subjectId: 10 + }); + + expect(result.current).toBeNull(); + }); + + it('returns null for content element types that do not support quotes', () => { + const {result} = renderSubjectQuote({ + subjectType: 'ContentElement', subjectId: 11, subjectRange + }); + + expect(result.current).toBeNull(); + }); + + it('returns null for section subjects', () => { + const {result} = renderSubjectQuote({ + subjectType: 'Section', subjectId: 1, subjectRange + }); + + expect(result.current).toBeNull(); + }); + + it('cuts quotes to the length the server stores', () => { + review.contentElementTypes.register('heading', { + extractQuote: () => 'a'.repeat(4010) + }); + + const {result} = renderSubjectQuote({ + subjectType: 'ContentElement', subjectId: 12 + }); + + expect(result.current).toEqual('a'.repeat(4000)); + }); + + it('returns null for deleted content elements', () => { + const {result} = renderSubjectQuote({ + subjectType: 'ContentElement', subjectId: 999, subjectRange + }); + + expect(result.current).toBeNull(); + }); +}); diff --git a/entry_types/scrolled/package/src/contentElements/heading/review.js b/entry_types/scrolled/package/src/contentElements/heading/review.js new file mode 100644 index 0000000000..9fa839be27 --- /dev/null +++ b/entry_types/scrolled/package/src/contentElements/heading/review.js @@ -0,0 +1,30 @@ +import {Node} from 'slate'; + +import {review} from 'pageflow-scrolled/review'; + +// Headings carry no ranged comments since EditableInlineText has no +// commenting alternative, so every comment refers to the header as a whole +// and the quote covers all three of its texts. +review.contentElementTypes.register('heading', { + extractQuote(configuration) { + const parts = [ + inlineText(configuration.tagline), + mainText(configuration), + inlineText(configuration.subtitle) + ]; + + return parts.map(part => part.trim()).filter(Boolean).join('\n') || null; + } +}); + +// Mirrors how EditableInlineText picks the legacy string value, so the quote +// never contains text the heading does not display. +function mainText(configuration) { + return configuration.value ? + inlineText(configuration.value) : + configuration.children || ''; +} + +function inlineText(value) { + return value ? value.map(node => Node.string(node)).join(' ') : ''; +} diff --git a/entry_types/scrolled/package/src/contentElements/review.js b/entry_types/scrolled/package/src/contentElements/review.js index 12792af7bd..19adbaab67 100644 --- a/entry_types/scrolled/package/src/contentElements/review.js +++ b/entry_types/scrolled/package/src/contentElements/review.js @@ -1 +1,2 @@ +import './heading/review'; import './textBlock/review'; diff --git a/entry_types/scrolled/package/src/contentElements/textBlock/review.js b/entry_types/scrolled/package/src/contentElements/textBlock/review.js index 02835221ca..1f86659448 100644 --- a/entry_types/scrolled/package/src/contentElements/textBlock/review.js +++ b/entry_types/scrolled/package/src/contentElements/textBlock/review.js @@ -1,4 +1,4 @@ -import {Point} from 'slate'; +import {Node, Point} from 'slate'; import {review} from 'pageflow-scrolled/review'; @@ -8,6 +8,23 @@ review.contentElementTypes.register('textBlock', { if (!a) return 1; if (!b) return -1; return Point.compare(rangeStart(a), rangeStart(b)); + }, + + // Element wide comments stay unquoted: the whole text is too long to serve + // as a point of reference, and any edit anywhere in it would mark the quote + // outdated. + extractQuote(configuration, range) { + if (!range) return null; + + const root = {children: configuration.value || []}; + + if (!Node.has(root, range.anchor.path) || !Node.has(root, range.focus.path)) { + return null; + } + + const text = Node.fragment(root, range).map(node => Node.string(node)).join('\n').trim(); + + return text || null; } }); diff --git a/entry_types/scrolled/package/src/review/Comment.js b/entry_types/scrolled/package/src/review/Comment.js index 02b7a1aa10..6b11497255 100644 --- a/entry_types/scrolled/package/src/review/Comment.js +++ b/entry_types/scrolled/package/src/review/Comment.js @@ -4,7 +4,7 @@ import {Avatar} from './Avatar'; import styles from './Comment.module.css'; -export function Comment({comment}) { +export function Comment({comment, showQuote}) { return (
@@ -13,6 +13,8 @@ export function Comment({comment}) { {comment.createdAt && }
+ {showQuote && +
{comment.quote}
}

{comment.body}

); diff --git a/entry_types/scrolled/package/src/review/Comment.module.css b/entry_types/scrolled/package/src/review/Comment.module.css index e21f94b58b..13eb3a6583 100644 --- a/entry_types/scrolled/package/src/review/Comment.module.css +++ b/entry_types/scrolled/package/src/review/Comment.module.css @@ -15,6 +15,17 @@ color: var(--ui-on-surface-color-light); } +.quote { + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 3; + overflow: hidden; + margin: 0 0 space(2) space(8); + padding-left: space(2); + border-left: solid 2px var(--ui-on-surface-color-lightest); + color: var(--ui-on-surface-color-light); +} + .body { margin: 0; padding-left: space(8); diff --git a/entry_types/scrolled/package/src/review/NewThreadForm.js b/entry_types/scrolled/package/src/review/NewThreadForm.js index ca29975295..bec4dba95e 100644 --- a/entry_types/scrolled/package/src/review/NewThreadForm.js +++ b/entry_types/scrolled/package/src/review/NewThreadForm.js @@ -4,6 +4,7 @@ import {useSectionPermaIdOfSubject} from 'pageflow-scrolled/entryState'; import {useI18n} from '../frontend/i18n'; import {postCreateCommentThreadMessage} from './postMessage'; +import {useSubjectQuote} from './subjectQuote'; import {autoGrow, autoResize} from './autoGrow'; import {isSubmitShortcut} from './submitShortcut'; @@ -17,6 +18,10 @@ export function NewThreadForm({subjectType, subjectId, subjectRange, onSubmit}) const sectionPermaId = useSectionPermaIdOfSubject({subjectType, subjectId}); + // Recorded now since the commented text can change afterwards, leaving the + // comment without the wording it referred to. + const quote = useSubjectQuote({subjectType, subjectId, subjectRange}); + // preventScroll keeps focus from yanking the page to the top before the // portaled popover has been positioned by floating-ui. const setInputRef = useCallback(node => { @@ -44,7 +49,9 @@ export function NewThreadForm({subjectType, subjectId, subjectRange, onSubmit}) function createThread() { if (!hasText) return; - postCreateCommentThreadMessage({subjectType, subjectId, subjectRange, sectionPermaId, body}); + postCreateCommentThreadMessage({ + subjectType, subjectId, subjectRange, sectionPermaId, body, quote + }); setBody(''); if (onSubmit) onSubmit(); diff --git a/entry_types/scrolled/package/src/review/ReplyForm.js b/entry_types/scrolled/package/src/review/ReplyForm.js index 064b7cd98d..d7683aa525 100644 --- a/entry_types/scrolled/package/src/review/ReplyForm.js +++ b/entry_types/scrolled/package/src/review/ReplyForm.js @@ -2,17 +2,22 @@ import React, {useState} from 'react'; import {useI18n} from '../frontend/i18n'; import {postCreateCommentMessage} from './postMessage'; +import {useSubjectQuote} from './subjectQuote'; import {autoGrow, autoResize} from './autoGrow'; import {isSubmitShortcut} from './submitShortcut'; import SendIcon from './images/send.svg'; import styles from './ReplyForm.module.css'; -export function ReplyForm({threadId}) { +export function ReplyForm({threadId, subjectType, subjectId, subjectRange}) { const {t} = useI18n({locale: 'ui'}); const [body, setBody] = useState(''); const hasText = body.trim().length > 0; + // Each reply records the wording it responds to, so a thread spanning + // several edits keeps every comment next to its own version of the text. + const quote = useSubjectQuote({subjectType, subjectId, subjectRange}); + function handleChange(e) { setBody(e.target.value); autoGrow(e.target); @@ -33,7 +38,7 @@ export function ReplyForm({threadId}) { function createReply() { if (!hasText) return; - postCreateCommentMessage({threadId, body}); + postCreateCommentMessage({threadId, body, quote}); setBody(''); } diff --git a/entry_types/scrolled/package/src/review/Thread.js b/entry_types/scrolled/package/src/review/Thread.js index cfa3a33db4..3cc07c48bc 100644 --- a/entry_types/scrolled/package/src/review/Thread.js +++ b/entry_types/scrolled/package/src/review/Thread.js @@ -1,10 +1,12 @@ -import React, {useEffect, useRef} from 'react'; +import React, {useEffect, useMemo, useRef} from 'react'; import classNames from 'classnames'; import {useI18n} from '../frontend/i18n'; import {AvatarStack} from './Avatar'; import {Comment} from './Comment'; import {ReplyForm} from './ReplyForm'; +import {useSubjectQuote} from './subjectQuote'; +import {commentsWithOutdatedQuote} from './outdatedQuotes'; import {useScrollHighlightedThreadIntoView} from './scrollHighlightedThreadIntoView'; import ChevronIcon from './images/chevron.svg'; @@ -18,6 +20,12 @@ export function Thread({thread, collapsed, onToggle, onResolve, onClick, highlig const replies = thread.comments.slice(1); const repliesCollapsed = collapsed && replies.length > 0; + const currentQuote = useSubjectQuote(thread); + const outdatedQuotes = useMemo( + () => commentsWithOutdatedQuote(thread.comments, currentQuote), + [thread.comments, currentQuote] + ); + const ref = useRef(); const scrollHighlightedIntoView = useScrollHighlightedThreadIntoView(); @@ -48,7 +56,9 @@ export function Thread({thread, collapsed, onToggle, onResolve, onClick, highlig {t('pageflow_scrolled.review.refers_to_deleted_element')}

} - {firstComment && } + {firstComment && + } {repliesCollapsed && } {!collapsed && replies.map(comment => ( - + ))} {interactive && !thread.resolvedAt && !repliesCollapsed && - } + } {interactive && onResolve && !repliesCollapsed &&
diff --git a/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js b/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js index b589e9fb39..a67a997c77 100644 --- a/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js +++ b/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js @@ -1,8 +1,8 @@ // Holds per content element type behavior needed to display comment // threads. Available both in the editor and in the frontend commenting -// mode. Kept separate from the editor and frontend registries so comment -// ordering logic (comparing Slate ranges) can be shared by both without -// pulling Slate into the always-on frontend bundle. +// mode. Kept separate from the editor and frontend registries so subject +// range logic (ordering ranges, reading the text they cover) can be shared +// by both without pulling Slate into the always-on frontend bundle. export class ContentElementTypeRegistry { constructor() { this.types = {}; @@ -15,4 +15,8 @@ export class ContentElementTypeRegistry { findCompareRanges(typeName) { return this.types[typeName]?.compareRanges; } + + findExtractQuote(typeName) { + return this.types[typeName]?.extractQuote; + } } diff --git a/entry_types/scrolled/package/src/review/outdatedQuotes.js b/entry_types/scrolled/package/src/review/outdatedQuotes.js new file mode 100644 index 0000000000..5ef7cfe1cc --- /dev/null +++ b/entry_types/scrolled/package/src/review/outdatedQuotes.js @@ -0,0 +1,18 @@ +// Picks the comments whose recorded quote no longer matches the text as it +// reads now. While a comment still refers to the current wording, the +// highlight in the entry is the better point of reference. Consecutive +// comments sharing a referent only show it on the first of them, so a run of +// replies about the same wording does not repeat it. +export function commentsWithOutdatedQuote(comments, currentQuote) { + const ids = new Set(); + + comments.forEach((comment, index) => { + const previousQuote = index > 0 ? comments[index - 1].quote : undefined; + + if (comment.quote && comment.quote !== currentQuote && comment.quote !== previousQuote) { + ids.add(comment.id); + } + }); + + return ids; +} diff --git a/entry_types/scrolled/package/src/review/postMessage.js b/entry_types/scrolled/package/src/review/postMessage.js index 0274908878..8f1ccef233 100644 --- a/entry_types/scrolled/package/src/review/postMessage.js +++ b/entry_types/scrolled/package/src/review/postMessage.js @@ -1,16 +1,18 @@ -export function postCreateCommentThreadMessage({subjectType, subjectId, subjectRange, sectionPermaId, body}) { +export function postCreateCommentThreadMessage({ + subjectType, subjectId, subjectRange, sectionPermaId, body, quote +}) { window.top.postMessage( { type: 'CREATE_COMMENT_THREAD', - payload: {subjectType, subjectId, subjectRange, sectionPermaId, body} + payload: {subjectType, subjectId, subjectRange, sectionPermaId, body, quote} }, window.location.origin ); } -export function postCreateCommentMessage({threadId, body}) { +export function postCreateCommentMessage({threadId, body, quote}) { window.top.postMessage( - {type: 'CREATE_COMMENT', payload: {threadId, body}}, + {type: 'CREATE_COMMENT', payload: {threadId, body, quote}}, window.location.origin ); } diff --git a/entry_types/scrolled/package/src/review/subjectQuote.js b/entry_types/scrolled/package/src/review/subjectQuote.js new file mode 100644 index 0000000000..6a700f78ed --- /dev/null +++ b/entry_types/scrolled/package/src/review/subjectQuote.js @@ -0,0 +1,31 @@ +import {useMemo} from 'react'; + +import {useContentElement} from 'pageflow-scrolled/entryState'; + +import {review} from './api'; + +// Matches Pageflow::Comment::QUOTE_LIMIT. Capping here rather than only on +// the server keeps a recorded quote comparable to the text as it reads later +// on: a quote the server had cut would differ from the full text forever and +// count as outdated even while the wording was untouched. +const QUOTE_LIMIT = 4000; + +// The range is passed on as is, including when there is none: types with +// ranged comments quote the selection and skip range-less subjects, types +// whose comments always cover the whole element quote its text either way. +/** + * @private + */ +export function useSubjectQuote({subjectType, subjectId, subjectRange}) { + const contentElement = useContentElement({permaId: subjectId}); + const isContentElement = subjectType === 'ContentElement'; + + return useMemo(() => { + if (!isContentElement || !contentElement) return null; + + const extractQuote = review.contentElementTypes.findExtractQuote(contentElement.type); + const quote = extractQuote ? extractQuote(contentElement.props, subjectRange) : null; + + return quote ? quote.slice(0, QUOTE_LIMIT) : null; + }, [isContentElement, subjectRange, contentElement]); +} diff --git a/package/spec/review/ReviewSession-spec.js b/package/spec/review/ReviewSession-spec.js index 8222ea39a9..4c0383bf32 100644 --- a/package/spec/review/ReviewSession-spec.js +++ b/package/spec/review/ReviewSession-spec.js @@ -193,6 +193,77 @@ describe('ReviewSession', () => { }); }); + it('includes quote in the first comment of a createThread request', async () => { + const request = jest.fn() + .mockResolvedValueOnce({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [] + }) + .mockResolvedValueOnce({ + id: 1, + subjectType: 'ContentElement', + subjectId: 10, + comments: [{id: 100, body: 'About this text', quote: 'quick brown', creatorId: 42}] + }); + + const session = new ReviewSession({entryId: 5, request}); + await session.fetch(); + + await session.createThread({ + subjectType: 'ContentElement', + subjectId: 10, + body: 'About this text', + quote: 'quick brown' + }); + + expect(request).toHaveBeenLastCalledWith({ + url: '/review/entries/5/comment_threads', + method: 'POST', + payload: { + comment_thread: { + subject_type: 'ContentElement', + subject_id: 10, + comment: {body: 'About this text', quote: 'quick brown'} + } + } + }); + }); + + it('omits quote from createThread request when not given', async () => { + const request = jest.fn() + .mockResolvedValueOnce({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [] + }) + .mockResolvedValueOnce({ + id: 1, + subjectType: 'ContentElement', + subjectId: 10, + comments: [{id: 100, body: 'Looks good!', creatorId: 42}] + }); + + const session = new ReviewSession({entryId: 5, request}); + await session.fetch(); + + await session.createThread({ + subjectType: 'ContentElement', + subjectId: 10, + body: 'Looks good!' + }); + + expect(request).toHaveBeenLastCalledWith({ + url: '/review/entries/5/comment_threads', + method: 'POST', + payload: { + comment_thread: { + subject_type: 'ContentElement', + subject_id: 10, + comment: {body: 'Looks good!'} + } + } + }); + }); + it('omits section_perma_id from createThread request when not given', async () => { const request = jest.fn() .mockResolvedValueOnce({ @@ -336,6 +407,54 @@ describe('ReviewSession', () => { ); }); + it('includes quote in createComment request', async () => { + const request = jest.fn() + .mockResolvedValueOnce({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'CE', subjectId: 10, comments: [ + {id: 100, body: 'First', creatorId: 42} + ]} + ] + }) + .mockResolvedValueOnce({id: 101, body: 'Reply', quote: 'lazy dog', creatorId: 42}); + + const session = new ReviewSession({entryId: 5, request}); + await session.fetch(); + + await session.createComment({threadId: 1, body: 'Reply', quote: 'lazy dog'}); + + expect(request).toHaveBeenLastCalledWith({ + url: '/review/entries/5/comment_threads/1/comments', + method: 'POST', + payload: {comment: {body: 'Reply', quote: 'lazy dog'}} + }); + }); + + it('omits quote from createComment request when not given', async () => { + const request = jest.fn() + .mockResolvedValueOnce({ + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'CE', subjectId: 10, comments: [ + {id: 100, body: 'First', creatorId: 42} + ]} + ] + }) + .mockResolvedValueOnce({id: 101, body: 'Reply', creatorId: 42}); + + const session = new ReviewSession({entryId: 5, request}); + await session.fetch(); + + await session.createComment({threadId: 1, body: 'Reply'}); + + expect(request).toHaveBeenLastCalledWith({ + url: '/review/entries/5/comment_threads/1/comments', + method: 'POST', + payload: {comment: {body: 'Reply'}} + }); + }); + it('updates state after createComment', async () => { const request = jest.fn() .mockResolvedValueOnce({ diff --git a/package/src/review/ReviewSession.js b/package/src/review/ReviewSession.js index 71974e589c..79dd05e11e 100644 --- a/package/src/review/ReviewSession.js +++ b/package/src/review/ReviewSession.js @@ -11,7 +11,9 @@ export class ReviewSession { return this._state; } - async createThread({subjectType, subjectId, subjectRange, sectionPermaId, body}) { + async createThread({ + subjectType, subjectId, subjectRange, sectionPermaId, body, quote + }) { const thread = await this._request({ url: `/review/entries/${this._entryId}/comment_threads`, method: 'POST', @@ -21,7 +23,7 @@ export class ReviewSession { subject_id: subjectId, ...(subjectRange && {subject_range: subjectRange}), ...(sectionPermaId != null && {section_perma_id: sectionPermaId}), - comment: {body} + comment: {body, ...(quote && {quote})} } } }); @@ -41,11 +43,11 @@ export class ReviewSession { this.trigger('change:thread', thread); } - async createComment({threadId, body}) { + async createComment({threadId, body, quote}) { const comment = await this._request({ url: `/review/entries/${this._entryId}/comment_threads/${threadId}/comments`, method: 'POST', - payload: {comment: {body}} + payload: {comment: {body, ...(quote && {quote})}} }); const thread = this._findThread(threadId); diff --git a/spec/controllers/pageflow/review/comment_threads_controller_spec.rb b/spec/controllers/pageflow/review/comment_threads_controller_spec.rb index 6ab8d805c3..416a4ea2cf 100644 --- a/spec/controllers/pageflow/review/comment_threads_controller_spec.rb +++ b/spec/controllers/pageflow/review/comment_threads_controller_spec.rb @@ -79,6 +79,23 @@ module Pageflow ) end + it 'returns quote of comments' do + user = create(:user) + entry = create(:entry, with_previewer: user) + + thread = create(:comment_thread, revision: entry.draft, creator: user) + create(:comment, comment_thread: thread, creator: user, quote: 'quick brown') + + sign_in(user, scope: :user) + get(:index, params: {entry_id: entry.id}, format: 'json') + + expect(response.body).to include_json( + commentThreads: [ + {comments: [{quote: 'quick brown'}]} + ] + ) + end + it 'does not have N+1 queries' do user = create(:user) entry = create(:entry, with_previewer: user) @@ -159,6 +176,27 @@ module Pageflow expect(Pageflow::CommentThread.last.section_perma_id).to eq(12) end + it 'creates first comment with quote' do + user = create(:user) + entry = create(:entry, with_previewer: user) + + sign_in(user, scope: :user) + post(:create, params: { + entry_id: entry.id, + comment_thread: { + subject_type: 'ContentElement', + subject_id: 5, + comment: {body: 'About this text', quote: 'quick brown'} + } + }, format: 'json') + + expect(response.status).to eq(201) + expect(response.body).to include_json( + comments: [{body: 'About this text', quote: 'quick brown'}] + ) + expect(Pageflow::Comment.last.quote).to eq('quick brown') + end + it 'creates thread with subject_range' do user = create(:user) entry = create(:entry, with_previewer: user) diff --git a/spec/controllers/pageflow/review/comments_controller_spec.rb b/spec/controllers/pageflow/review/comments_controller_spec.rb index 44ac20521c..46dbd4e0d7 100644 --- a/spec/controllers/pageflow/review/comments_controller_spec.rb +++ b/spec/controllers/pageflow/review/comments_controller_spec.rb @@ -25,6 +25,23 @@ module Pageflow ) end + it 'creates comment with quote' do + user = create(:user) + entry = create(:entry, with_previewer: user) + thread = create(:comment_thread, revision: entry.draft, creator: user) + + sign_in(user, scope: :user) + post(:create, params: { + entry_id: entry.id, + comment_thread_id: thread.id, + comment: {body: 'Still wrong', quote: 'lazy dog'} + }, format: 'json') + + expect(response.status).to eq(201) + expect(response.body).to include_json(quote: 'lazy dog') + expect(Pageflow::Comment.last.quote).to eq('lazy dog') + end + it 'requires user to be signed in' do entry = create(:entry) thread = create(:comment_thread, revision: entry.draft) diff --git a/spec/models/pageflow/comment_spec.rb b/spec/models/pageflow/comment_spec.rb new file mode 100644 index 0000000000..a30bbc028c --- /dev/null +++ b/spec/models/pageflow/comment_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +module Pageflow + describe Comment do + describe 'quote' do + it 'is stored as given when within the limit' do + comment = create(:comment, quote: 'quick brown') + + expect(comment.reload.quote).to eq('quick brown') + end + + it 'is cut to the limit instead of keeping the comment from being saved' do + comment = create(:comment, quote: 'a' * (Comment::QUOTE_LIMIT + 10)) + + expect(comment.reload.quote.length).to eq(Comment::QUOTE_LIMIT) + end + + it 'is cut without appending an omission marker' do + comment = create(:comment, quote: "#{'a' * Comment::QUOTE_LIMIT}bcd") + + expect(comment.reload.quote).to eq('a' * Comment::QUOTE_LIMIT) + end + + it 'stays blank when not given' do + comment = create(:comment) + + expect(comment.reload.quote).to be_nil + end + end + end +end