Skip to content

Commit

Permalink
Allow draft annotations to match filters
Browse files Browse the repository at this point in the history
The internal function that filters annotations returned a list of matching IDs.
Unsaved annotations do not have IDs, so were excluded from the matches. Change
the function to return a list of matching annotations instead, lifting this
restriction.

No callers of the function had to change because currently they only test the
length of the returned array.

A caveat with this change is that it only matches the data stored in the
annotation object, not data stored in separate "draft" objects which are
combined when rendering unsaved annotations. The result is that a filter will
only match the fields of the annotation that are populated when the new
annotation initially appears in the sidebar. This for example includes the user
and page number, but not text added in the text field later.
  • Loading branch information
robertknight committed Sep 20, 2024
1 parent 76040e7 commit 19fc6f7
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 18 deletions.
10 changes: 2 additions & 8 deletions src/sidebar/helpers/filter-annotations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,11 @@ const fieldMatchers: Record<string, Matcher | Matcher<number>> = {

/**
* Filter a set of annotations against a parsed query.
*
* @return IDs of matching annotations.
*/
export function filterAnnotations(
annotations: Annotation[],
filters: Record<string, Facet>,
): string[] {
): Annotation[] {
const makeTermFilter = <TermType>(field: string, term: TermType) =>
new TermFilter(
term,
Expand Down Expand Up @@ -191,9 +189,5 @@ export function filterAnnotations(

const rootFilter = new BooleanOpFilter('and', fieldFilters);

return annotations
.filter(ann => {
return ann.id && rootFilter.matches(ann);
})
.map(ann => ann.id!);
return annotations.filter(ann => rootFilter.matches(ann));
}
22 changes: 12 additions & 10 deletions src/sidebar/helpers/test/filter-annotations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('sidebar/helpers/filter-annotations', () => {

const result = filterAnnotations(annotations, filters);

assert.deepEqual(result, [1]);
assert.deepEqual(result, annotations.slice(0, 1));
});

it('requires at least one term to match for "or" operator', () => {
Expand Down Expand Up @@ -142,7 +142,7 @@ describe('sidebar/helpers/filter-annotations', () => {

const result = filterAnnotations([annotation], filters);

assert.deepEqual(result, [1]);
assert.deepEqual(result, [annotation]);
});
});

Expand Down Expand Up @@ -171,7 +171,7 @@ describe('sidebar/helpers/filter-annotations', () => {
];
const result = filterAnnotations(anns, userQuery('john'));

assert.deepEqual(result, [anns[0].id, anns[2].id]);
assert.deepEqual(result, [anns[0], anns[2]]);
});

it("matches user's display name if present", () => {
Expand All @@ -190,7 +190,7 @@ describe('sidebar/helpers/filter-annotations', () => {
];
const result = filterAnnotations(anns, userQuery('james'));

assert.deepEqual(result, [anns[1].id, anns[2].id, anns[3].id]);
assert.deepEqual(result, [anns[1], anns[2], anns[3]]);
});

it('matches username and display name independently', () => {
Expand Down Expand Up @@ -219,7 +219,7 @@ describe('sidebar/helpers/filter-annotations', () => {

const result = filterAnnotations([annotation], filters);

assert.deepEqual(result, [1]);
assert.deepEqual(result, [annotation]);
});

it('does not match if the annotation is older than the query', () => {
Expand Down Expand Up @@ -257,7 +257,7 @@ describe('sidebar/helpers/filter-annotations', () => {
it('matches if annotation is in page range', () => {
const filters = { page: { terms: ['4-6'], operator: 'or' } };
const result = filterAnnotations([annotation], filters);
assert.deepEqual(result, [1]);
assert.deepEqual(result, [annotation]);
});

it('does not match if annotation is outside of page range', () => {
Expand Down Expand Up @@ -291,7 +291,7 @@ describe('sidebar/helpers/filter-annotations', () => {
it('matches if annotation is in range', () => {
const filters = { cfi: { terms: [range], operator: 'or' } };
const result = filterAnnotations([annotation], filters);
assert.deepEqual(result, [1]);
assert.deepEqual(result, [annotation]);
});
});

Expand Down Expand Up @@ -331,13 +331,15 @@ describe('sidebar/helpers/filter-annotations', () => {

const result = filterAnnotations([annotation], filters);

assert.deepEqual(result, [1]);
assert.deepEqual(result, [annotation]);
});

it('ignores annotations (drafts) with no id', () => {
it('can match annotations (drafts) with no id', () => {
const annotation = {
tags: ['foo'],
target: [{}],
text: '',
url: 'https://example.com',
};
const filters = {
any: {
Expand All @@ -348,6 +350,6 @@ describe('sidebar/helpers/filter-annotations', () => {

const result = filterAnnotations([annotation], filters);

assert.deepEqual(result, []);
assert.deepEqual(result, [annotation]);
});
});

0 comments on commit 19fc6f7

Please sign in to comment.