fix: Filter collections by remote isApproved field (#526)

* fix: Filter collections by remote isApproved field * test: fix test * chore: adds documentation to methods
decentraland · May 20, 2022 · 584f541 · 584f541
1 parent d313d83
commit 584f541
Show file tree

Hide file tree

Showing 6 changed files with 142 additions and 47 deletions.
diff --git a/src/Collection/Collection.model.ts b/src/Collection/Collection.model.ts
@@ -2,6 +2,7 @@ import { Model, raw, SQL } from 'decentraland-server'
 import { DEFAULT_LIMIT } from '../Pagination/utils'
 import { CurationStatusFilter, CurationStatusSort } from '../Curation'
 import { CollectionCuration } from '../Curation/CollectionCuration'
+import { ItemCuration } from '../Curation/ItemCuration'
 import { database } from '../database/database'
 import { Item } from '../Item/Item.model'
 import { CollectionAttributes } from './Collection.types'
@@ -24,18 +25,24 @@ export type FindCollectionParams = {
   status?: CurationStatusFilter
   sort?: CurationStatusSort
   isPublished?: boolean
+  remoteIds?: CollectionAttributes['id'][]
 }
 
 export class Collection extends Model<CollectionAttributes> {
   static tableName = 'collections'
 
-  static getOrderByStatement(sort?: CurationStatusSort) {
+  static getOrderByStatement(sort?: CurationStatusSort, remoteIds?: string[]) {
     switch (sort) {
       case CurationStatusSort.MOST_RELEVANT:
+        // Order should be
+        // 1- To review: Not assigned && isApproved false from the contract
+        // 2- Under review: Assigned && isApproved false from the contract OR has pending curation
+        // 3- Approved: Curation approved
+        // 4- Rejected: Curation rejected
         return SQL`
           ORDER BY
             CASE WHEN(
-              collection_curations.assignee is NULL)
+              collection_curations.assignee is NULL AND collections.contract_address = ANY(${remoteIds}))
             THEN 0
             WHEN(collection_curations.assignee is NOT NULL AND collection_curations.status = ${CurationStatusFilter.PENDING})
             THEN 1
@@ -44,7 +51,7 @@ export class Collection extends Model<CollectionAttributes> {
               WHEN(collection_curations.status = ${CurationStatusFilter.REJECTED})
             THEN 3
             ELSE 4
-          END
+          END, collections.created_at DESC
         `
       case CurationStatusSort.NAME_ASC:
         return SQL`ORDER BY collections.name ASC`
@@ -63,9 +70,10 @@ export class Collection extends Model<CollectionAttributes> {
     status,
     address,
     thirdPartyIds,
+    remoteIds,
   }: Pick<
     FindCollectionParams,
-    'q' | 'assignee' | 'status' | 'address' | 'thirdPartyIds'
+    'q' | 'assignee' | 'status' | 'address' | 'thirdPartyIds' | 'remoteIds'
   >) {
     if (!q && !assignee && !status && !address && !thirdPartyIds?.length) {
       return SQL``
@@ -82,12 +90,13 @@ export class Collection extends Model<CollectionAttributes> {
         ? [
             CurationStatusFilter.PENDING,
             CurationStatusFilter.APPROVED,
-            CurationStatusFilter.REJECTED,
           ].includes(status)
-          ? SQL`collection_curations.status = ${status}`
-          : status === CurationStatusFilter.TO_REVIEW
-          ? SQL`collection_curations.assignee is NULL`
-          : status === CurationStatusFilter.UNDER_REVIEW
+          ? SQL`collection_curations.status = ${status} AND collections.contract_address = ANY(${remoteIds})`
+          : status === CurationStatusFilter.REJECTED // Rejected not a single curation in approved
+          ? SQL`collection_curations.status = ${status} AND collections.contract_address = ANY(${remoteIds})`
+          : status === CurationStatusFilter.TO_REVIEW // To review: Not assigned && isApproved false from the contract
+          ? SQL`collection_curations.assignee is NULL AND collections.contract_address = ANY(${remoteIds})`
+          : status === CurationStatusFilter.UNDER_REVIEW // Under review: isApproved false from the contract OR it's assigned & has pending curation
           ? SQL`collection_curations.assignee is NOT NULL AND collection_curations.status = ${CurationStatusFilter.PENDING}`
           : SQL``
         : undefined,
@@ -110,14 +119,52 @@ export class Collection extends Model<CollectionAttributes> {
     return result
   }
 
-  static getPublishedJoinStatement(isPublished = false) {
-    return isPublished
-      ? SQL`JOIN ${raw(
-          Item.tableName
-        )} items on items.collection_id = collections.id AND items.blockchain_item_id is NOT NULL`
+  static getPublishedJoinStatement(isPublished: boolean | undefined) {
+    return isPublished !== undefined
+      ? SQL`
+            JOIN ${raw(Item.tableName)} items ON items.collection_id = c.id ${
+          isPublished
+            ? SQL`AND (
+              (items.blockchain_item_id is NOT NULL AND c.third_party_id IS NULL)
+              OR c.third_party_id is NOT NULL)`
+            : SQL`AND (items.blockchain_item_id is NULL AND c.third_party_id IS NULL)`
+        }
+            LEFT JOIN ${raw(
+              ItemCuration.tableName
+            )} item_curations ON item_curations.item_id = items.id
+            ${
+              isPublished === false
+                ? SQL`
+                  WHERE (
+                    (SELECT COUNT(*) FROM item_curations
+                      LEFT JOIN items on items.id = item_curations.item_id
+                      LEFT JOIN collections cc on cc.id = items.collection_id
+                      WHERE items.collection_id = c.id AND item_curations.item_id = items.id
+                    ) = 0)
+                `
+                : SQL``
+            }
+        `
       : SQL``
   }
 
+  /**
+   * Finds all the Collections that given parameters. It sorts and paginates the results.
+   * If the status is APPROVED, the remoteIds will be the ones with `isApproved` true.
+   * If the status is TO_REVIEW, UNDER_REVIEW or REJECTED, the remoteIds will be the ones with `isApproved` false.
+   * If status is not defined, the remoteIds will be the ones with `isApproved` false, so the ORDER BY can put them first.
+   *
+   * @param limit - limit param to paginate the query
+   * @param offset - offset param to paginate the query
+   * @param sort - the type of sorting to apply. MOST_RELEVANT will return the NOT approved first.
+   * @param isPublished - if true, will return only the published collections. If false, will return only the unpublished collections.
+   * @param q - the query to search for.
+   * @param assignee - the assignee to filter by.
+   * @param status - the status to filter by.
+   * @param address - the address to filter by.
+   * @param thirdPartyIds - the third party ids to filter by. If it's a committee member, the array will have all the ids.
+   * @param remoteIds - The remote ids to filter the query with
+   */
   static findAll({
     limit = DEFAULT_LIMIT,
     offset = 0,
@@ -128,16 +175,23 @@ export class Collection extends Model<CollectionAttributes> {
     const query = SQL`
       SELECT collections.*, COUNT(*) OVER() as collection_count, (SELECT COUNT(*) FROM ${raw(
         Item.tableName
-      )} WHERE items.collection_id = collections.id) as item_count
-        FROM ${raw(this.tableName)} collections
-        ${SQL`LEFT JOIN ${raw(
-          CollectionCuration.tableName
-        )} collection_curations ON collection_curations.collection_id = collections.id`}
-        ${SQL`${this.getPublishedJoinStatement(isPublished)}`}
+      )} 
+        WHERE items.collection_id = collections.id) as item_count
+        FROM (
+          SELECT DISTINCT on (c.id) c.* FROM ${raw(Collection.tableName)} c
+            ${SQL`${this.getPublishedJoinStatement(isPublished)}`}  
+        ) collections
+        ${SQL`
+        LEFT JOIN 
+          (SELECT DISTINCT on (cc.collection_id) cc.* FROM ${raw(
+            CollectionCuration.tableName
+          )} cc ORDER BY cc.collection_id, cc.created_at DESC) collection_curations 
+          ON collection_curations.collection_id = collections.id
+        `}
         ${SQL`${this.getFindAllWhereStatement(whereFilters)}`}
-        ${SQL`${this.getOrderByStatement(sort)}`}
+        ${SQL`${this.getOrderByStatement(sort, whereFilters.remoteIds)}`}
         LIMIT ${limit}
-        OFFSET ${offset}  
+        OFFSET ${offset}
       `
     return this.query<CollectionWithCounts>(query)
   }

diff --git a/src/Collection/Collection.router.spec.ts b/src/Collection/Collection.router.spec.ts
@@ -844,13 +844,14 @@ describe('Collection router', () => {
             })
             expect(Collection.findAll).toHaveBeenCalledWith({
               assignee: undefined,
-              isPublished: false,
+              isPublished: undefined,
               q: undefined,
               sort: undefined,
               status: undefined,
               limit,
               offset: page - 1, // it's the offset,
               thirdPartyIds: [],
+              remoteIds: [],
             })
           })
       })
@@ -911,6 +912,7 @@ describe('Collection router', () => {
               offset: page - 1, // it's the offset
               limit,
               thirdPartyIds: [],
+              remoteIds: [],
             })
           })
       })

diff --git a/src/Collection/Collection.router.ts b/src/Collection/Collection.router.ts
@@ -48,7 +48,7 @@ import {
 } from './Collection.types'
 import { upsertCollectionSchema, saveTOSSchema } from './Collection.schema'
 import { hasPublicAccess } from './access'
-import { toFullCollection } from './utils'
+import { toFullCollection, toRemoteWhereCondition } from './utils'
 import {
   AlreadyPublishedCollectionError,
   InsufficientSlotsError,
@@ -233,18 +233,26 @@ export class CollectionRouter extends Router {
       )
     }
 
-    const [allCollectionsWithCount, remoteCollections] = await Promise.all([
-      this.service.getCollections({
-        q: q as string,
-        assignee: assignee as string,
-        status: status as CurationStatusFilter,
-        sort: sort as CurationStatusSort,
-        isPublished: is_published === 'true',
-        offset: page && limit ? getOffset(page, limit) : undefined,
-        limit,
-      }),
-      collectionAPI.fetchCollections(),
-    ])
+    // If status is passed, the graph query will be filtered and those results will be included in a WHERE statement in the query later on
+    // If status is not passed, the query won't be filtered and all the collections will be retrieved
+    const remoteCollections = await collectionAPI.fetchCollections(
+      toRemoteWhereCondition({ status: status as CurationStatusFilter })
+    )
+
+    const allCollectionsWithCount = await this.service.getCollections({
+      q: q as string,
+      assignee: assignee as string,
+      status: status as CurationStatusFilter,
+      sort: sort as CurationStatusSort,
+      isPublished: is_published ? is_published === 'true' : undefined,
+      offset: page && limit ? getOffset(page, limit) : undefined,
+      limit,
+      remoteIds: status
+        ? remoteCollections.map((c) => c.id)
+        : // if the status is not passed, we still want to prioritize the not approved. It won't filter by them, it'll just use them for the sort.
+          // We filter at this level and not in the query because we need all the collections so they can be consolidated later on.
+          remoteCollections.filter((r) => !r.isApproved).map((c) => c.id),
+    })
 
     const totalCollections =
       Number(allCollectionsWithCount[0]?.collection_count) || 0

diff --git a/src/Collection/utils.ts b/src/Collection/utils.ts
@@ -9,7 +9,10 @@ import {
   ContractName,
 } from 'decentraland-transactions'
 import { Bridge } from '../ethereum/api/Bridge'
-import { collectionAPI } from '../ethereum/api/collection'
+import {
+  collectionAPI,
+  CollectionQueryFilters,
+} from '../ethereum/api/collection'
 import { getMappedChainIdForCurrentChainName } from '../ethereum/utils'
 import { ItemCuration } from '../Curation/ItemCuration'
 import {
@@ -19,6 +22,7 @@ import {
   hasTPCollectionURN,
   isTPCollection,
 } from '../utils/urn'
+import { CurationStatusFilter } from '../Curation'
 import { Cheque } from '../SlotUsageCheque'
 import { CollectionAttributes, FullCollection } from './Collection.types'
 import { UnpublishedCollectionError } from './Collection.errors'
@@ -220,3 +224,18 @@ export async function getAddressFromSignature(
     chequeSignatureData.signature
   )
 }
+
+/**
+ * Converts a '/collections' endpoint filter to an object that can be translated to a WHERE condition for the collections graph
+ *
+ * @param dbCollection - The "FullCollection" to be converted into a DB collection.
+ */
+export function toRemoteWhereCondition({
+  status,
+}: {
+  status?: CurationStatusFilter
+}): CollectionQueryFilters {
+  return {
+    isApproved: status ? status === CurationStatusFilter.APPROVED : undefined,
+  }
+}
diff --git a/src/Pagination/utils.ts b/src/Pagination/utils.ts
@@ -1,6 +1,6 @@
 import { Request } from 'express'
 
-export const DEFAULT_LIMIT = 10000 // let's use the default as the max as well
+export const DEFAULT_LIMIT = 100000 // let's use the default as the max as well
 
 export const getPaginationParams = (req: Request) => {
   const { limit, page } = req.query

diff --git a/src/ethereum/api/collection.ts b/src/ethereum/api/collection.ts
@@ -19,6 +19,10 @@ import {
   PAGINATION_ARGUMENTS,
 } from './BaseGraphAPI'
 
+export type CollectionQueryFilters = {
+  isApproved?: boolean
+}
+
 const getCollectionByIdQuery = () => gql`
   query getCollectionById($id: String) {
     collections(where: { id: $id }) {
@@ -28,14 +32,22 @@ const getCollectionByIdQuery = () => gql`
   ${collectionFragment()}
 `
 
-const getCollectionsQuery = () => gql`
-  query getCollections(${PAGINATION_VARIABLES}) {
-    collections(${PAGINATION_ARGUMENTS}) {
-      ...collectionFragment
-    }
+const getCollectionsQuery = ({ isApproved }: CollectionQueryFilters) => {
+  const where: string[] = []
+
+  if (isApproved !== undefined) {
+    where.push(`isApproved : ${isApproved}`)
   }
-  ${collectionFragment()}
-`
+
+  return gql`
+    query getCollections(${PAGINATION_VARIABLES}) {
+      collections(${PAGINATION_ARGUMENTS}, where: { ${where.join('\n')} }) {
+        ...collectionFragment
+      }
+    }
+    ${collectionFragment()}
+  `
+}
 
 const getCollectionsByAuthorizedUserQuery = () => gql`
   query getCollectionsByCreator(${PAGINATION_VARIABLES}, $user: String, $users: [String!]) {
@@ -166,9 +178,9 @@ export class CollectionAPI extends BaseGraphAPI {
     return collections.length > 0 ? collections[0] : null
   }
 
-  fetchCollections = async (): Promise<CollectionFragment[]> => {
+  fetchCollections = async (filters: CollectionQueryFilters): Promise<CollectionFragment[]> => {
     return this.paginate(['collections'], {
-      query: getCollectionsQuery(),
+      query: getCollectionsQuery(filters),
     })
   }