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