feat: Return full updated items after upsert (#764)

* feat: Return full updated items after upsert

* fix: Add comments

spec/mocks/items.ts

@@ -83,9 +83,9 @@ export function toResultTPItem(
     ...itemAttributes,
     created_at: itemAttributes.created_at.toISOString(),
     updated_at: itemAttributes.updated_at.toISOString(),
-    is_approved: true,
-    in_catalyst: true,
-    is_published: true,
+    is_approved: !!catalystItem,
+    in_catalyst: !!catalystItem,
+    is_published: !!catalystItem,
     urn: hasURN
       ? buildTPItemURN(
           dbCollection!.third_party_id!,
@@ -169,7 +169,7 @@ export const dbTPItemMock: ThirdPartyItemAttributes = {
 export const itemFragmentMock = {
   id: dbCollectionMock.contract_address + '-' + dbItemMock.blockchain_item_id,
   blockchainId: '0',
-  urn: `urn:decentraland:mumbai:collections-v2:${dbCollectionMock.contract_address}:${dbItemMock.blockchain_item_id}`,
+  urn: `urn:decentraland:amoy:collections-v2:${dbCollectionMock.contract_address}:${dbItemMock.blockchain_item_id}`,
   totalSupply: '1',
   price: dbItemMock.price!.toString(),
   beneficiary: 'aBeneficiary',

spec/utils.ts

@@ -1,10 +1,12 @@
 import { Authenticator, AuthIdentity } from '@dcl/crypto'
+import { Wearable } from '@dcl/schemas'
 import { Model, QueryPart } from 'decentraland-server'
 import { env } from 'decentraland-commons'
 import { collectionAPI } from '../src/ethereum/api/collection'
 import { peerAPI } from '../src/ethereum/api/peer'
 import { isPublished } from '../src/utils/eth'
 import { AUTH_CHAIN_HEADER_PREFIX } from '../src/middleware/authentication'
+import { CollectionFragment, ItemFragment } from '../src/ethereum/api/fragments'
 import { Collection } from '../src/Collection'
 import { ItemCuration } from '../src/Curation/ItemCuration'
 import { Ownable } from '../src/Ownable/Ownable'
@@ -405,4 +407,43 @@ export function mockThirdPartyCollectionURNExists(
   })
 }
 
+/**
+ * Mocks the result of the fetchCollectionWithItem method from the collections client.
+ * This mock requires collectionAPI to be mocked first.
+ *
+ * @param collectionFragment - The blockchain collection to return.
+ * @param itemFragment - The blockchain item to return.
+ */
+export function mockFetchCollectionWithItem(
+  collectionFragment: CollectionFragment | null,
+  itemFragment: ItemFragment | null
+) {
+  if (!(collectionAPI.fetchCollectionWithItem as jest.Mock).mock) {
+    throw new Error(
+      "collectionAPI.fetchCollectionWithItem should be mocked to mock the fetchCollectionWithItem method but it isn't"
+    )
+  }
+
+  ;(collectionAPI.fetchCollectionWithItem as jest.Mock).mockResolvedValueOnce({
+    collection: collectionFragment,
+    item: itemFragment,
+  })
+}
+
+/**
+ * Mocks the result of the fetch items method from the catalyst client.
+ * This mock requires peerAPI to be mocked first.
+ *
+ * @param items - The Catalyst items to return.
+ */
+export function mockFetchCatalystItems(items: Wearable[]) {
+  if (!(peerAPI.fetchItems as jest.Mock).mock) {
+    throw new Error(
+      "peerAPI.fetchItems should be mocked to mock the fetchItems method but it isn't"
+    )
+  }
+
+  ;(peerAPI.fetchItems as jest.Mock).mockResolvedValueOnce(items)
+}
+
 export const isoDateStringMatcher = /\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d\.\d+([+-][0-2]\d:[0-5]\d|Z)/

src/Item/Item.router.spec.ts

@@ -1,7 +1,7 @@
 import { MappingType, Rarity, Wearable } from '@dcl/schemas'
 import supertest from 'supertest'
 import { v4 as uuidv4 } from 'uuid'
-import { Wallet } from 'ethers'
+import { ethers, Wallet } from 'ethers'
 import { utils } from 'decentraland-commons'
 import { omit } from 'decentraland-commons/dist/utils'
 import {
@@ -15,6 +15,8 @@ import {
   mockThirdPartyItemCurationExists,
   mockThirdPartyURNExists,
   isoDateStringMatcher,
+  mockFetchCollectionWithItem,
+  mockFetchCatalystItems,
 } from '../../spec/utils'
 import {
   dbCollectionMock,
@@ -129,8 +131,6 @@ describe('Item router', () => {
     resultTPItemNotPublished = asResultItem(dbTPItemNotPublished) // no itemCuration & no catalyst, should be regular Item
     resultTPItemPublished = {
       ...toResultTPItem(dbTPItemPublished, dbTPCollectionMock),
-      is_approved: false,
-      in_catalyst: false,
     }
   })
 
@@ -262,7 +262,6 @@ describe('Item router', () => {
       ;(peerAPI.fetchWearables as jest.Mock).mockResolvedValueOnce([tpWearable])
       url = '/items'
     })
-
     it('should return all the items that are published with URN and the ones that are not without it', () => {
       return server
         .get(buildURL(url))
@@ -1003,8 +1002,14 @@ describe('Item router', () => {
                     undefined,
                     tpCollectionMock
                   ),
+                  beneficiary: ethers.constants.AddressZero,
+                  blockchain_item_id: updatedItem.urn_suffix,
+                  price: '0',
                   updated_at: expect.stringMatching(isoDateStringMatcher),
                 }
+                // Mock get TP item
+                mockThirdPartyItemCurationExists(dbTPItem.id, false)
+                mockThirdPartyURNExists(itemToUpsert.urn!, false)
               })
 
               it('should respond with a 200, update the item and return the updated item', () => {
@@ -1073,8 +1078,14 @@ describe('Item router', () => {
                   undefined,
                   tpCollectionMock
                 ),
+                beneficiary: ethers.constants.AddressZero,
+                blockchain_item_id: updatedItem.urn_suffix,
+                price: '0',
                 updated_at: expect.stringMatching(isoDateStringMatcher),
               }
+              // Mock get TP item
+              mockThirdPartyItemCurationExists(dbTPItem.id, false)
+              mockThirdPartyURNExists(itemToUpsert.urn!, false)
             })
 
             it('it should respond with a 400 when the urn in the url does not match the one in the body', () => {
@@ -1247,7 +1258,13 @@ describe('Item router', () => {
                   ),
                   updated_at: expect.stringMatching(isoDateStringMatcher),
                   created_at: expect.stringMatching(isoDateStringMatcher),
+                  beneficiary: ethers.constants.AddressZero,
+                  blockchain_item_id: updatedItem.urn_suffix,
+                  price: '0',
                 }
+                // Mock get TP item
+                mockThirdPartyItemCurationExists(dbTPItem.id, false)
+                mockThirdPartyURNExists(itemToUpsert.urn!, false)
               })
 
               it('should respond with a 200, update the item and return the updated item', () => {
@@ -1273,6 +1290,9 @@ describe('Item router', () => {
                 collection_id: tpCollectionMock.id,
                 urn: null,
               }
+              // Mock get TP item
+              mockThirdPartyItemCurationExists(dbTPItem.id, false)
+              mockThirdPartyURNExists(itemToUpsert.urn!, false)
             })
 
             it('should respond with a 400, signaling that the URN is not valid', () => {
@@ -1978,6 +1998,12 @@ describe('Item router', () => {
                   managers: [wallet.address],
                 })
             )
+            // Mock get item
+            mockFetchCollectionWithItem(itemFragment.collection, {
+              ...itemFragment,
+              totalSupply: '0',
+            })
+            mockFetchCatalystItems([wearable])
           })
 
           afterEach(() => {
@@ -1996,6 +2022,10 @@ describe('Item router', () => {
                 ...Bridge.toFullItem(dbItem),
                 local_content_hash: expect.any(String),
                 eth_address: ethAddress,
+                beneficiary: 'aBeneficiary',
+                in_catalyst: true,
+                is_published: true,
+                urn: wearable.id,
                 created_at: dbItem.created_at.toISOString(),
                 updated_at: currentDate.toISOString(),
               },
@@ -2105,6 +2135,12 @@ describe('Item router', () => {
                   creator: wallet.address,
                 })
             )
+            // Mock get item
+            mockFetchCollectionWithItem(itemFragment.collection, {
+              ...itemFragment,
+              totalSupply: '0',
+            })
+            mockFetchCatalystItems([wearable])
           })
 
           afterEach(() => {
@@ -2121,6 +2157,10 @@ describe('Item router', () => {
             expect(response.body).toEqual({
               data: {
                 ...Bridge.toFullItem(dbItem),
+                beneficiary: itemFragment.beneficiary,
+                in_catalyst: true,
+                is_published: true,
+                urn: wearable.id,
                 local_content_hash: expect.any(String),
                 eth_address: wallet.address,
                 created_at: dbItem.created_at.toISOString(),

src/Item/Item.service.spec.ts

@@ -8,7 +8,11 @@ import {
   itemFragmentMock,
 } from '../../spec/mocks/items'
 import { wearableMock } from '../../spec/mocks/peer'
-import { mockOwnableCanUpsert } from '../../spec/utils'
+import {
+  mockFetchCatalystItems,
+  mockFetchCollectionWithItem,
+  mockOwnableCanUpsert,
+} from '../../spec/utils'
 import { CollectionAttributes } from '../Collection'
 import { Collection } from '../Collection/Collection.model'
 import { CollectionService } from '../Collection/Collection.service'
@@ -139,6 +143,7 @@ describe('Item Service', () => {
         beforeEach(() => {
           ;(Item.findByURNSuffix as jest.Mock).mockResolvedValueOnce(undefined)
         })
+
         describe('and it is inserting less than the maximun amount of tags', () => {
           beforeEach(() => {
             dbItem = {
@@ -164,7 +169,11 @@ describe('Item Service', () => {
             jest
               .spyOn(CollectionService.prototype, 'isDCLPublished')
               .mockResolvedValueOnce(false)
+            // Mock get item
+            mockFetchCollectionWithItem(null, null)
+            mockFetchCatalystItems([])
           })
+
           it('should not throw any errors and return the inserted item', () => {
             const result = service.upsertItem(
               Bridge.toFullItem(dbItem, dbTPCollectionMock),
@@ -201,11 +210,11 @@ describe('Item Service', () => {
         beforeEach(() => {
           ;(Item.findByURNSuffix as jest.Mock).mockResolvedValueOnce(dbItem)
         })
-        describe('and the item already has the maximun amount of tags', () => {
+        describe('and the item already has the maximum amount of tags', () => {
           it('should throw the MaximunAmountOfTagsReachedError error', () => {
             return expect(
               service.upsertItem(
-                Bridge.toFullItem(dbItem, dbTPCollectionMock),
+                Bridge.toFullItem(dbItem, dbCollectionMock),
                 dbItem.eth_address
               )
             ).rejects.toThrowError(MaximunAmountOfTagsReachedError)
@@ -237,10 +246,14 @@ describe('Item Service', () => {
               .spyOn(CollectionService.prototype, 'isDCLPublished')
               .mockResolvedValueOnce(false)
             ;(Item.upsert as jest.Mock).mockResolvedValueOnce(dbItem)
+            // Mock get item
+            mockFetchCollectionWithItem(null, null)
+            mockFetchCatalystItems([])
           })
+
           it('should not throw any error and return the inserted item', () => {
             const result = service.upsertItem(
-              Bridge.toFullItem(dbItem, dbTPCollectionMock),
+              Bridge.toFullItem(dbItem, dbCollectionMock),
               dbItem.eth_address
             )
             return expect(result).resolves.toEqual(

src/Item/Item.service.ts

@@ -355,10 +355,12 @@ export class ItemService {
   }
 
   private async getTPItem(
-    dbItem: ThirdPartyItemAttributes
+    dbItem: ThirdPartyItemAttributes,
+    dbCollection?: CollectionAttributes
   ): Promise<{ item: FullItem; collection?: CollectionAttributes }> {
     let item: FullItem = Bridge.toFullItem(dbItem)
-    let collection = await Collection.findOne(dbItem.collection_id)
+    let collection =
+      dbCollection ?? (await Collection.findOne(dbItem.collection_id))
 
     if (collection && isTPCollection(collection)) {
       const urn = buildTPItemURN(
@@ -375,24 +377,27 @@ export class ItemService {
         : collection
 
       const catalystItems = await peerAPI.fetchWearables<Wearable>([urn])
-      if (catalystItems.length > 0) {
-        item = Bridge.mergeTPItem(dbItem, collection, catalystItems[0])
-      }
+      item = Bridge.mergeTPItem(
+        dbItem,
+        collection as ThirdPartyCollectionAttributes,
+        catalystItems[0]
+      )
     }
 
     return { item, collection }
   }
 
   private async getDCLItem(
-    dbItem: ItemAttributes
+    dbItem: ItemAttributes,
+    dbItemCollection?: CollectionAttributes
   ): Promise<{ item: FullItem; collection?: CollectionAttributes }> {
     let item: FullItem = Bridge.toFullItem(dbItem)
     let collection: CollectionAttributes | undefined
 
     if (dbItem.collection_id && dbItem.blockchain_item_id) {
-      const dbCollection = await Collection.findOne<CollectionAttributes>(
-        dbItem.collection_id
-      )
+      const dbCollection =
+        dbItemCollection ??
+        (await Collection.findOne<CollectionAttributes>(dbItem.collection_id))
 
       if (!dbCollection) {
         throw new InconsistentItemError(
@@ -426,7 +431,6 @@ export class ItemService {
           )
         }
       }
-
       // Set the item's URN
       item.urn =
         item.urn ??
@@ -664,12 +668,20 @@ export class ItemService {
 
     // Compute the content hash of the item to later store it in the DB
     attributes.local_content_hash =
-      dbCollection && isStandardItemPublished(attributes, dbCollection)
-        ? await calculateItemContentHash(attributes, dbCollection)
+      dbCollection &&
+      isStandardItemPublished(attributes, itemCollection ?? dbCollection)
+        ? await calculateItemContentHash(
+            attributes,
+            itemCollection ?? dbCollection
+          )
         : null
 
     const upsertedItem: ItemAttributes = await Item.upsert(attributes)
-    return Bridge.toFullItem(upsertedItem, dbCollection)
+    const { item: fullItem } = await this.getDCLItem(
+      upsertedItem,
+      itemCollection ?? dbCollection
+    )
+    return fullItem
   }
 
   /**
@@ -786,10 +798,10 @@ export class ItemService {
       ? await calculateItemContentHash(attributes, dbCollection)
       : null
 
-    const upsertedItem: ItemAttributes = await Item.upsert({
+    const upsertedItem = (await Item.upsert({
       ...attributes,
       ...(attributes.mappings ? { mappings: attributes.mappings } : {}),
-    })
+    })) as ThirdPartyItemAttributes
     if (dbItem && attributes.local_content_hash) {
       // Update the Item Curation content_hash
       await ItemCuration.update(
@@ -798,7 +810,8 @@ export class ItemService {
       )
     }
 
-    return Bridge.toFullItem(upsertedItem, dbCollection)
+    const { item: fullItem } = await this.getTPItem(upsertedItem, dbCollection)
+    return fullItem
   }
 
   private async checkIfThirdPartyItemURNExists(