Skip to content

Commit

Permalink
Only apply selector filters to threads, not replies
Browse files Browse the repository at this point in the history 
Add a property to filter facets which indicate whether they apply to annotations
and replies or only threads. Use this property to apply the filter either to all
annotations or only the root annotation in a thread respectively.

Queries based on the user or annotation body for example can apply to both.
Queries based on a selector such as page number, book chapter or quote only make
sense as thread-level filters.

Fixes hypothesis/support#158
  • Loading branch information
@robertknight
robertknight committed Sep 20, 2024
1 parent 19fc6f7 commit 3c90f0c
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 6 deletions.
18 changes: 18 additions & 0 deletions src/sidebar/helpers/query-parser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,15 @@ export type Facet = {
*/
operator: 'and' | 'or';
terms: string[] | number[];

/**
* Whether this facet is relevant for replies.
*
* This is true for facets such as annotation bodies, which both annotations
* and replies have, but false for eg. page numbers, which only annotations
* have.
*/
filterReplies: boolean;
};

export type ParsedQuery = Record<FilterField | 'any', Facet>;
Expand Down Expand Up @@ -236,38 +245,47 @@ export function parseFilterQuery(
any: {
terms: any,
operator: 'and',
filterReplies: true,
},
cfi: {
terms: cfi,
operator: 'or',
filterReplies: false,
},
quote: {
terms: quote,
operator: 'and',
filterReplies: false,
},
page: {
terms: page,
operator: 'or',
filterReplies: false,
},
since: {
terms: since,
operator: 'and',
filterReplies: true,
},
tag: {
terms: tag,
operator: 'and',
filterReplies: true,
},
text: {
terms: text,
operator: 'and',
filterReplies: true,
},
uri: {
terms: uri,
operator: 'or',
filterReplies: false,
},
user: {
terms: user,
operator: 'or',
filterReplies: true,
},
};
}
18 changes: 18 additions & 0 deletions src/sidebar/helpers/test/query-parser-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ describe('sidebar/helpers/query-parser', () => {
any: {
operator: 'and',
terms: ['one', 'two', 'three'],
filterReplies: true,
},
},
},
Expand All @@ -128,6 +129,7 @@ describe('sidebar/helpers/query-parser', () => {
tag: {
operator: 'and',
terms: ['foo', 'bar'],
filterReplies: true,
},
},
},
Expand All @@ -137,10 +139,12 @@ describe('sidebar/helpers/query-parser', () => {
quote: {
operator: 'and',
terms: ['inthequote'],
filterReplies: false,
},
text: {
operator: 'and',
terms: ['inthetext'],
filterReplies: true,
},
},
},
Expand All @@ -150,6 +154,7 @@ describe('sidebar/helpers/query-parser', () => {
user: {
operator: 'or',
terms: ['john', 'james'],
filterReplies: true,
},
},
},
Expand All @@ -159,6 +164,7 @@ describe('sidebar/helpers/query-parser', () => {
uri: {
operator: 'or',
terms: ['https://example.org/article.html'],
filterReplies: false,
},
},
},
Expand All @@ -168,6 +174,7 @@ describe('sidebar/helpers/query-parser', () => {
page: {
operator: 'or',
terms: ['5-10'],
filterReplies: false,
},
},
},
Expand All @@ -177,6 +184,7 @@ describe('sidebar/helpers/query-parser', () => {
cfi: {
operator: 'or',
terms: ['/2-/4'],
filterReplies: false,
},
},
},
Expand All @@ -188,6 +196,7 @@ describe('sidebar/helpers/query-parser', () => {
any: {
operator: 'and',
terms: ['group:abcd'],
filterReplies: true,
},
},
},
Expand All @@ -197,6 +206,7 @@ describe('sidebar/helpers/query-parser', () => {
any: {
operator: 'and',
terms: ['any:foo'],
filterReplies: true,
},
},
},
Expand Down Expand Up @@ -269,6 +279,7 @@ describe('sidebar/helpers/query-parser', () => {
user: {
operator: 'or',
terms: ['fakeusername'],
filterReplies: true,
},
},
},
Expand All @@ -278,6 +289,7 @@ describe('sidebar/helpers/query-parser', () => {
page: {
operator: 'or',
terms: ['2-4'],
filterReplies: false,
},
},
},
Expand All @@ -287,6 +299,7 @@ describe('sidebar/helpers/query-parser', () => {
cfi: {
operator: 'or',
terms: ['/2/2-/2/6'],
filterReplies: false,
},
},
},
Expand All @@ -298,6 +311,7 @@ describe('sidebar/helpers/query-parser', () => {
user: {
operator: 'or',
terms: ['fakeusername', 'otheruser'],
filterReplies: true,
},
},
},
Expand All @@ -308,10 +322,12 @@ describe('sidebar/helpers/query-parser', () => {
any: {
operator: 'and',
terms: ['foo'],
filterReplies: true,
},
user: {
operator: 'or',
terms: ['fakeusername'],
filterReplies: true,
},
},
},
Expand All @@ -322,6 +338,7 @@ describe('sidebar/helpers/query-parser', () => {
page: {
operator: 'or',
terms: ['2-4', '6'],
filterReplies: false,
},
},
},
Expand All @@ -332,6 +349,7 @@ describe('sidebar/helpers/query-parser', () => {
cfi: {
operator: 'or',
terms: ['/2/2-/2/6', '/4/2-/4/4'],
filterReplies: false,
},
},
},
Expand Down
16 changes: 12 additions & 4 deletions src/sidebar/helpers/test/thread-annotations-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('sidebar/helpers/thread-annotations', () => {
.callsFake(() => structuredClone(fixtures.emptyThread));
fakeFilterAnnotations = sinon.stub();
fakeQueryParser = {
parseFilterQuery: sinon.stub(),
parseFilterQuery: sinon.stub().returns({}),
};

$imports.$mock({
Expand Down Expand Up @@ -293,21 +293,29 @@ describe('sidebar/helpers/thread-annotations', () => {

it('should filter annotations if a filter query is set', () => {
fakeThreadState.selection.filterQuery = 'anything';
fakeQueryParser.parseFilterQuery.returns({
termA: { terms: ['bar'], filterReplies: true },
termB: { terms: ['foo'], filterReplies: false },
});
const annotation = annotationFixtures.defaultAnnotation();
fakeFilterAnnotations.returns([annotation]);

threadAnnotations(fakeThreadState);

const filterFn = fakeBuildThread.args[0][1].filterFn;

assert.isFunction(filterFn);
assert.calledOnce(fakeQueryParser.parseFilterQuery);
assert.calledWith(
fakeQueryParser.parseFilterQuery,
fakeThreadState.selection.filterQuery,
fakeThreadState.selection.filters,
);

const filterFn = fakeBuildThread.args[0][1].filterFn;
assert.isFunction(filterFn);
assert.isTrue(filterFn(annotation));

const threadFilterFn = fakeBuildThread.args[0][1].threadFilterFn;
assert.isFunction(threadFilterFn);
assert.isTrue(threadFilterFn({ annotation }));
});

it('should filter annotations if there is an applied focus filter', () => {
Expand Down
24 changes: 22 additions & 2 deletions src/sidebar/helpers/thread-annotations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { buildThread } from './build-thread';
import type { Thread, BuildThreadOptions } from './build-thread';
import { filterAnnotations } from './filter-annotations';
import { parseFilterQuery } from './query-parser';
import type { FilterField } from './query-parser';
import type { FilterField, ParsedQuery } from './query-parser';
import { tabForAnnotation } from './tabs';
import { sorters } from './thread-sorters';

Expand Down Expand Up @@ -67,7 +67,27 @@ function threadAnnotationsImpl(
selection.filterQuery || '',
selection.filters,
);
options.filterFn = ann => filterAnnotations([ann], filters).length > 0;

// Split filters into those which apply to both annotations and replies,
// and those which should only be applied to threads.
const annotationFilters: Partial<ParsedQuery> = {};
const threadFilters: Partial<ParsedQuery> = {};
for (const [field, facet] of Object.entries(filters)) {
if (facet.filterReplies) {
annotationFilters[field as FilterField] = facet;
} else {
threadFilters[field as FilterField] = facet;
}
}

options.filterFn = ann =>
filterAnnotations([ann], annotationFilters).length > 0;

if (Object.values(threadFilters).some(facet => facet.terms.length > 0)) {
options.threadFilterFn = thread =>
!!thread.annotation &&
filterAnnotations([thread.annotation], threadFilters).length > 0;
}
}

const rootThread = buildThread(threadState.annotations, options);
Expand Down

0 comments on commit 3c90f0c

Please sign in to comment.