Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions app/controllers/pageflow/review/comment_threads_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion app/controllers/pageflow/review/comments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def create
private

def comment_params
params.require(:comment).permit(:body)
params.require(:comment).permit(:body, :quote)
end
end
end
Expand Down
15 changes: 15 additions & 0 deletions app/models/pageflow/comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions app/views/pageflow/review/comments/_comment.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ json.call(comment,
:perma_id,
:creator_id,
:body,
:quote,
:created_at,
:updated_at)

Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20260727000000_add_quote_to_comments.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddQuoteToComments < ActiveRecord::Migration[7.1]
def change
add_column :pageflow_comments, :quote, :text
end
end
Original file line number Diff line number Diff line change
@@ -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');
});
});
Original file line number Diff line number Diff line change
@@ -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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});
128 changes: 128 additions & 0 deletions entry_types/scrolled/package/spec/review/Thread-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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();
});
});
});
Loading
Loading