BL-9697 Fix publisher/originalPublisher searches

Author: Gordon Martin

src/connection/LibraryQueryHooks.test.ts

@@ -107,7 +107,7 @@ async function cleanup() {
     await deleteUser(fredId, sessionToken);
 }
 
-const title1 = "Bloom library 2 test book 1 anunlikelykeyword title";
+const title1 = "Bloom library 2 test about 1 anunlikelykeyword title";
 const title2 =
     " test Enabling writers workshops book about topic:Math in the Pacific from bookdash.org";
 const title3 = "Another book with anunlikelykeyword";
@@ -136,6 +136,7 @@ beforeAll(async () => {
                 "bookshelf:Enabling writers workshops",
             ],
             copyright: "Copyright © 2014, Nicole and bookdash.org",
+            publisher: "Unlikely Publisher",
         });
         // This is there specifically to NOT be found by tag searches or the copyright search
         // It SHOULD be found by the uploader search, however.
@@ -159,6 +160,8 @@ beforeAll(async () => {
                 className: "_User",
                 objectId: fredId,
             },
+            publisher: "Unlikely Publisher",
+            originalPublisher: "Unlikely Original Publisher",
         });
     } catch (error) {
         console.log(JSON.stringify(error));
@@ -209,8 +212,9 @@ it("retrieves a book with topic:Math in tags, but not one with that string in ti
     expect(result.data.results[0].title).toBe(title1);
 });
 
+// Previously this test failed because one of the two words was "book", which gets removed.
 it("retrieves a book with a quoted string, but not one with the two words separately", async () => {
-    const result = await getBook({ search: '"test book"' });
+    const result = await getBook({ search: '"test about"' });
     expect(result.data.results.length).toBe(1);
     expect(result.data.results[0].title).toBe(title1);
 });
@@ -222,3 +226,29 @@ it("retrieves a book with quoted tag value", async () => {
     expect(result.data.results.length).toBe(1);
     expect(result.data.results[0].title).toBe(title1);
 });
+
+it("retrieves a book with quoted publisher value", async () => {
+    const result = await getBook({
+        search: 'publisher: "Unlikely Publisher"',
+    });
+    expect(result.data.results.length).toBe(2);
+    expect(result.data.results[0].title).toBe(title1);
+    expect(result.data.results[1].title).toBe(title3);
+});
+
+it("retrieves a book with quoted originalPublisher value", async () => {
+    const result = await getBook({
+        search: 'originalPublisher: "Unlikely Original Publisher"',
+    });
+    expect(result.data.results.length).toBe(1);
+    expect(result.data.results[0].title).toBe(title3);
+});
+
+it("doesn't crash with a facet non-value", async () => {
+    const result = await getBook({
+        search: "publisher:",
+    });
+    expect(result.data.results.length).toBe(2);
+    expect(result.data.results[0].title).toBe(title1);
+    expect(result.data.results[1].title).toBe(title3);
+});

src/connection/LibraryQueryHooks.ts

@@ -777,7 +777,7 @@ export function splitString(
     // these would be things like "system:Incoming"
     allTagsInDatabase: string[]
 ): { otherSearchTerms: string; specialParts: string[] } {
-    /*  JH/AP removed April 6 202 because tags was optional (fine),
+    /*  JH/AP removed April 6 2020 because tags was optional (fine),
     but then this method would essentially bail out if you didn't provide tags.
 
     if (!allTagsInDatabase) {
@@ -794,6 +794,8 @@ export function splitString(
         "phash:",
         "level:",
         "feature:",
+        "originalPublisher:", // must come before "publisher:", since "originalPublisher:" includes the other as a substring
+        "publisher:",
     ];
 
     const possibleParts = [...facets, ...allTagsInDatabase];
@@ -814,13 +816,16 @@ export function splitString(
             gotOne = true;
             let end = index + possiblePart.length;
             const isFacet = facets.includes(possiblePart);
+            let part = otherSearchTerms.substring(index, end);
             if (isFacet) {
-                end = otherSearchTerms.indexOf(" ", end);
-                if (end < 0) {
-                    end = otherSearchTerms.length;
-                }
+                const result = getFacetPartWithOrWithoutQuotes(
+                    otherSearchTerms,
+                    index,
+                    end
+                );
+                part = result.part;
+                end = result.end;
             }
-            let part = otherSearchTerms.substring(index, end);
             // If possibleParts contains an exact match for the part, we'll keep that case.
             // So for example if we have both system:Incoming and system:incoming, it's
             // possible to search for either. Otherwise, we want to switch to the case
@@ -847,6 +852,43 @@ export function splitString(
     return { otherSearchTerms, specialParts };
 }
 
+function getFacetPartWithOrWithoutQuotes(
+    searchString: string,
+    startIndex: number,
+    endIndex: number
+): { part: string; end: number } {
+    const facet = searchString.substring(startIndex, endIndex); // e.g. publisher:
+    const len = searchString.length;
+    // Handle corner case of nothing following the facet's "colon".
+    if (len === endIndex) {
+        return { part: facet, end: endIndex };
+    }
+    // Start looking for the value of the facet.
+    let start = endIndex;
+    let end: number;
+    let facetPart: string;
+    if (searchString[start] === '"') {
+        // handle double quote subcase
+        start++; // don't include the quotes in our result
+        end = searchString.indexOf('"', start);
+        if (end < 0) {
+            end = len;
+        }
+        facetPart = searchString.substring(start, end);
+        end = start + facetPart.length;
+        if (end < len && searchString[end] === '"') {
+            end++;
+        }
+    } else {
+        end = searchString.indexOf(" ", start);
+        if (end < 0) {
+            end = len;
+        }
+        facetPart = searchString.substring(start, end);
+    }
+    return { part: facet + facetPart, end };
+}
+
 function regexCaseSensitive(value: string) {
     return {
         $regex: processRegExp(value),
@@ -1151,11 +1193,15 @@ export function constructParseBookQuery(
         };
     }
 
-    delete params.where.publisher;
+    // We (gjm and jt) aren't sure why these two "delete" lines were ever needed, but now that we
+    // add params for both publisher and originalPublisher, it doesn't work to delete them here.
+    // If removing the delete lines causes a problem, we'll need to look for a different solution.
+
+    //delete params.where.publisher;
     if (f.publisher) {
         params.where.publisher = f.publisher;
     }
-    delete params.where.originalPublisher;
+    //delete params.where.originalPublisher;
     if (f.originalPublisher) {
         params.where.originalPublisher = f.originalPublisher;
     }

src/connection/SplitString.test.ts

@@ -16,7 +16,7 @@ it("a mixture of otherSearchTerms and special parts", () => {
     ]);
     expect(otherSearchTerms).toEqual("dogs cats");
     expect(specialParts.length).toBe(1);
-    expect(specialParts[0]).toBe("topic:Animals");
+    expect(specialParts.includes("topic:Animals"));
 });
 
 it("topic at start", () => {
@@ -30,7 +30,7 @@ it("topic at start", () => {
     ]);
     expect(otherSearchTerms).toEqual("dogs cats");
     expect(specialParts.length).toBe(1);
-    expect(specialParts[0]).toBe("topic:Animals");
+    expect(specialParts.includes("topic:Animals"));
 });
 
 it("topic at end", () => {
@@ -44,7 +44,7 @@ it("topic at end", () => {
     ]);
     expect(otherSearchTerms).toEqual("dogs cats");
     expect(specialParts.length).toBe(1);
-    expect(specialParts[0]).toBe("topic:Animals");
+    expect(specialParts.includes("topic:Animals"));
 });
 
 it("topic and bookshelf name and otherSearchTerms in quotes", () => {
@@ -61,8 +61,8 @@ it("topic and bookshelf name and otherSearchTerms in quotes", () => {
     );
     expect(otherSearchTerms).toEqual('dogs "black birds"');
     expect(specialParts.length).toBe(2);
-    expect(specialParts[0]).toBe("topic:Animal stories");
-    expect(specialParts[1]).toBe("bookshelf:enabling writers workshop");
+    expect(specialParts.includes("topic:Animal stories"));
+    expect(specialParts.includes("bookshelf:enabling writers workshop"));
 });
 
 it("ignores various unhelpful spaces", () => {
@@ -75,10 +75,8 @@ it("ignores various unhelpful spaces", () => {
     );
     expect(otherSearchTerms).toEqual('dogs "black birds"');
     expect(specialParts.length).toBe(2);
-    // Note that the order in which the parts are extracted depends on their order
-    // in the topic list, not in the input string.
-    expect(specialParts[0]).toBe("topic:Math");
-    expect(specialParts[1]).toBe("bookshelf:enabling writers workshop");
+    expect(specialParts.includes("topic:Math"));
+    expect(specialParts.includes("bookshelf:enabling writers workshop"));
 });
 
 it("finds uploader and copyright", () => {
@@ -92,10 +90,8 @@ it("finds uploader and copyright", () => {
     ]);
     expect(otherSearchTerms).toEqual("dogs");
     expect(specialParts.length).toBe(2);
-    // Note that the order in which the parts are extracted depends on their order
-    // in the topic list, not in the input string.
-    expect(specialParts[0]).toBe("uploader:fred@example");
-    expect(specialParts[1]).toBe("copyright:sil.org");
+    expect(specialParts.includes("uploader:fred@example"));
+    expect(specialParts.includes("copyright:sil.org"));
 });
 
 it("corrects case, but not if both cases are valid", () => {
@@ -111,7 +107,110 @@ it("corrects case, but not if both cases are valid", () => {
     ]);
     expect(otherSearchTerms).toEqual("cats");
     expect(specialParts.length).toBe(3);
-    expect(specialParts[0]).toBe("topic:Health");
-    expect(specialParts[1]).toBe("topic:math");
-    expect(specialParts[2]).toBe("topic:Math");
+    expect(specialParts.includes("topic:Health"));
+    expect(specialParts.includes("topic:math"));
+    expect(specialParts.includes("topic:Math"));
+});
+
+it("handles publisher and original publisher", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString("frogs publisher: Pratham originalPublisher:Bob", [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual("frogs");
+    expect(specialParts.length).toBe(2);
+    expect(specialParts.includes("originalPublisher:Bob"));
+    expect(specialParts.includes("publisher:Pratham"));
+});
+
+it("handles publisher and original publisher with spaces", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString(
+        'frogs publisher: "African Storybook Project" originalPublisher:"Book Dash"',
+        ["something irrelevant", "topic:Math"]
+    );
+    expect(otherSearchTerms).toEqual("frogs");
+    expect(specialParts.length).toBe(2);
+    expect(specialParts.includes("originalPublisher:Book Dash"));
+    expect(specialParts.includes("publisher:African Storybook Project"));
+});
+
+it("handles corner case of facet with no content", () => {
+    const { otherSearchTerms, specialParts } = splitString("frogs publisher:", [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual("frogs");
+    expect(specialParts.length).toBe(1);
+    expect(specialParts.includes("publisher:"));
+});
+
+it("handles facet corner case with quotes, but no space", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString('frogs publisher:"Pratham"', [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual("frogs");
+    expect(specialParts.length).toBe(1);
+    expect(specialParts.includes("publisher:Pratham"));
+});
+
+it("doesn't crash on missing facet final quote", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString('frogs publisher: "Book Dash', [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual("frogs");
+    expect(specialParts.length).toBe(1);
+    expect(specialParts.includes("publisher:Book Dash"));
+});
+
+it("doesn't crash on missing facet leading quote", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString('frogs publisher: Book Dash"', [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual('frogs Dash"');
+    expect(specialParts.length).toBe(1);
+    expect(specialParts.includes("publisher:Book"));
+});
+
+it("doesn't crash on mismatched facet quotes", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString('frogs publisher: "Book Dash" "another', [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual('frogs "another');
+    expect(specialParts.length).toBe(1);
+    expect(specialParts.includes("publisher:Book Dash"));
+});
+
+it("doesn't crash with facet empry string search", () => {
+    const {
+        otherSearchTerms,
+        specialParts,
+    } = splitString('frogs publisher: ""', [
+        "something irrelevant",
+        "topic:Math",
+    ]);
+    expect(otherSearchTerms).toEqual("frogs");
+    expect(specialParts.length).toBe(1);
+    expect(specialParts.includes("publisher:"));
 });