Bug 1759452 - When querying snapshot groups, take into account hidden snapshots. r=mak

Differential Revision: https://phabricator.services.mozilla.com/D141374

browser/components/places/SnapshotGroups.jsm

@@ -284,6 +284,8 @@ const SnapshotGroups = new (class SnapshotGroups {
    *   Use -1 to specify no limit.
    * @param {boolean} [options.hidden]
    *   Pass true to also return hidden groups.
+   * @param {boolean} [options.countHidden]
+   *   Pass true to include hidden snapshots in the count.
    * @param {string} [options.builder]
    *   Limit searching snapshot groups to results from a particular builder.
    * @param {boolean} [options.skipMinimum]
@@ -297,6 +299,7 @@ const SnapshotGroups = new (class SnapshotGroups {
     limit = 50,
     builder = undefined,
     hidden = false,
+    countHidden = false,
     skipMinimum = false,
   } = {}) {
     let db = await PlacesUtils.promiseDBConnection();
@@ -304,6 +307,7 @@ const SnapshotGroups = new (class SnapshotGroups {
     let params = {};
     let sizeFragment = [];
     let limitFragment = "";
+    let joinFragment = "";
     if (!skipMinimum) {
       sizeFragment.push("HAVING snapshot_count >= :minGroupSize");
       params.minGroupSize = MIN_GROUP_SIZE;
@@ -329,6 +333,10 @@ const SnapshotGroups = new (class SnapshotGroups {
       whereTerms.push("g.hidden = 0");
     }
 
+    if (!countHidden) {
+      joinFragment = "AND s.hidden = 0";
+    }
+
     let where = whereTerms.length ? `WHERE ${whereTerms.join(" AND ")}` : "";
 
     let rows = await db.executeCached(
@@ -342,14 +350,14 @@ const SnapshotGroups = new (class SnapshotGroups {
               SELECT preview_image_url, url
               FROM moz_places_metadata_snapshots sns
               JOIN moz_places_metadata_groups_to_snapshots gs USING(place_id)
-              JOIN moz_places h ON h.id = gs.place_id
+              JOIN moz_places h ON h.id = gs.place_id AND gs.hidden = 0
               WHERE gs.group_id = g.id
               ORDER BY sns.last_interaction_at ASC
               LIMIT 2
               )
             ) AS image_urls
       FROM moz_places_metadata_snapshots_groups g
-      LEFT JOIN moz_places_metadata_groups_to_snapshots s ON s.group_id = g.id
+      LEFT JOIN moz_places_metadata_groups_to_snapshots s ON s.group_id = g.id ${joinFragment}
       LEFT JOIN moz_places_metadata_snapshots sn ON sn.place_id = s.place_id
       ${where}
       GROUP BY g.id ${sizeFragment.join(" ")}

browser/components/places/tests/unit/interactions/test_snapshot_groups.js

@@ -6,7 +6,9 @@
  */
 
 const TEST_URL1 = "https://example.com/";
+const TEST_IMAGE_URL1 = "https://example.com/preview1.png";
 const TEST_URL2 = "https://example.com/12345";
+const TEST_IMAGE_URL2 = "https://example.com/preview2.png";
 const TEST_URL3 = "https://example.com/67890";
 const TEST_URL4 = "https://example.com/135246";
 const TEST_URL5 = "https://example.com/531246";
@@ -92,7 +94,7 @@ add_task(async function test_add_and_query() {
   });
 
   info("add a featured image for second oldest snapshot");
-  let previewImageURL = "https://example.com/preview2.png";
+  let previewImageURL = TEST_IMAGE_URL2;
   await PlacesUtils.history.update({
     url: TEST_URL2,
     previewImageURL,
@@ -108,7 +110,7 @@ add_task(async function test_add_and_query() {
   });
 
   info("add a featured image for the oldest snapshot");
-  previewImageURL = "https://example.com/preview1.png";
+  previewImageURL = TEST_IMAGE_URL1;
   await PlacesUtils.history.update({
     url: TEST_URL1,
     previewImageURL,
@@ -444,9 +446,28 @@ add_task(async function test_get_snapshots_sortBy() {
 add_task(async function test_get_snapshots_hidden() {
   let groups = await SnapshotGroups.query({ skipMinimum: true });
   Assert.equal(groups.length, 1, "Should return 1 SnapshotGroup");
+  Assert.equal(
+    groups[0].snapshotCount,
+    3,
+    "Should have the correct count of snapshots"
+  );
 
   await SnapshotGroups.setUrlHidden(groups[0].id, TEST_URL2, true);
 
+  groups = await SnapshotGroups.query({ skipMinimum: true });
+  Assert.equal(
+    groups[0].snapshotCount,
+    2,
+    "Should have the correct count of snapshots when not counting hidden ones"
+  );
+
+  groups = await SnapshotGroups.query({ skipMinimum: true, countHidden: true });
+  Assert.equal(
+    groups[0].snapshotCount,
+    3,
+    "Should have the correct count of snapshots when counting hidden ones"
+  );
+
   let snapshots = await SnapshotGroups.getSnapshots({
     id: groups[0].id,
     sortBy: "last_interaction_at",
@@ -467,6 +488,13 @@ add_task(async function test_get_snapshots_hidden() {
 
   await SnapshotGroups.setUrlHidden(groups[0].id, TEST_URL2, false);
 
+  groups = await SnapshotGroups.query({ skipMinimum: true });
+  Assert.equal(
+    groups[0].snapshotCount,
+    3,
+    "Should have the correct count of snapshots when not counting hidden ones"
+  );
+
   snapshots = await SnapshotGroups.getSnapshots({
     id: groups[0].id,
     sortBy: "last_interaction_at",
@@ -479,6 +507,24 @@ add_task(async function test_get_snapshots_hidden() {
   ]);
 });
 
+add_task(async function test_snapshot_image_with_hidden() {
+  let groups = await SnapshotGroups.query({ skipMinimum: true });
+  Assert.equal(groups.length, 1, "Should return 1 SnapshotGroup");
+
+  // The images were set on TEST_URL1/TEST_URL2 in an earlier test.
+  assertSnapshotGroup(groups[0], {
+    imageUrl: TEST_IMAGE_URL1,
+  });
+
+  await SnapshotGroups.setUrlHidden(groups[0].id, TEST_URL1, true);
+
+  groups = await SnapshotGroups.query({ skipMinimum: true });
+  info("Should use the oldest non-hidden image");
+  assertSnapshotGroup(groups[0], {
+    imageUrl: TEST_IMAGE_URL2,
+  });
+});
+
 add_task(async function test_minimum_size() {
   let urls = [TEST_URL1, TEST_URL2, TEST_URL3];
   let groupId = await SnapshotGroups.add(