feat: tp is published and is approved #378

nicosantangelo · 2022-01-03T19:35:57Z

Closes #277
Closes #368

…er-server into feat/tpw-is-published

coveralls · 2022-01-10T23:53:16Z

Pull Request Test Coverage Report for Build 1740886696

218 of 264 (82.58%) changed or added relevant lines in 16 files are covered.
6 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.3%) to 65.793%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Collection/Collection.service.ts	24	26	92.31%
src/Item/Item.model.ts	0	2	0.0%
src/ethereum/api/Bridge.ts	48	50	96.0%
src/Collection/Collection.router.ts	14	18	77.78%
src/Item/Item.router.ts	22	31	70.97%
src/ethereum/api/thirdParty.ts	20	32	62.5%
src/Item/Item.service.ts	42	57	73.68%

Files with Coverage Reduction	New Missed Lines	%
src/ethereum/api/Bridge.ts	2	84.8%
src/RequestParameters/RequestParameters.ts	4	7.32%

Totals
Change from base Build 1708311000:	-0.3%
Covered Lines:	1998
Relevant Lines:	2888

💛 - Coveralls

LautaroPetaccio · 2022-01-13T17:38:42Z

src/ethereum/api/thirdParty.ts

@@ -76,77 +89,204 @@ const getThirdPartyItemQuery = () => gql`
  ${thirdPartyItemFragment()}
 `

+const getLastItemQuery = () => gql`


Suggested change

const getLastItemQuery = () => gql`

const getLastReviewedItemQuery = () => gql`

LautaroPetaccio · 2022-01-13T20:11:54Z

migrations/1639023421988_rename-curations.ts

+import { MigrationBuilder } from 'node-pg-migrate'
+import { CollectionCuration } from '../src/Curation/CollectionCuration'
+
+export const up = (pgm: MigrationBuilder) => {


This should be already complete in #378

Isn't #378 this PR?

Oh! #379 right? I think this and the migration slipped thru the rebase

LautaroPetaccio · 2022-01-13T21:54:06Z

migrations/1639023686468_create-item-curations.ts

+      item_id: { type: 'UUID', notNull: true },
+      status: { type: 'CURATION_STATUS', notNull: true },
+      created_at: { type: 'TIMESTAMP', notNull: true },
+      updated_at: { type: 'TIMESTAMP', notNull: true },


If I'm not wrong, this has already been created in #378.

spec/utils.ts

LautaroPetaccio · 2022-01-14T15:24:37Z

src/ethereum/api/Bridge.ts

+    const collections: CollectionAttributes[] = []
+
+    for (const dbCollection of dbCollections) {
+      let fullCollection: CollectionAttributes = { ...dbCollection }
+
+      if (isTPCollection(dbCollection)) {
+        const lastItem = await thirdPartyAPI.fetchLastItem(
+          dbCollection.third_party_id,
+          dbCollection.urn_suffix
+        )
+        if (lastItem) {
+          fullCollection = Bridge.mergeTPCollection(dbCollection, lastItem)
+        }
+      }
+
+      collections.push(fullCollection)
+    }
+    return collections


Having this async operation inside the for loop will make this "synchronous", possibly taking a long time to process all of the collections. There's also a problem if we do it all asynchronous, and that is that we need to rate limit the requests, but we can leave this to another PR.

Suggested change

const collections: CollectionAttributes[] = []

for (const dbCollection of dbCollections) {

let fullCollection: CollectionAttributes = { ...dbCollection }

if (isTPCollection(dbCollection)) {

const lastItem = await thirdPartyAPI.fetchLastItem(

dbCollection.third_party_id,

dbCollection.urn_suffix

)

if (lastItem) {

fullCollection = Bridge.mergeTPCollection(dbCollection, lastItem)

}

}

collections.push(fullCollection)

}

return collections

const promiseOfCollections: Promise<CollectionAttributes[]> = []

promiseOfCollections = dbCollections.map(() => {

let fullCollection: CollectionAttributes = { ...dbCollection }

if (isTPCollection(dbCollection)) {

const lastItem = await thirdPartyAPI.fetchLastItem(

dbCollection.third_party_id,

dbCollection.urn_suffix

)

if (lastItem) {

fullCollection = Bridge.mergeTPCollection(dbCollection, lastItem)

}

}

})

return Promise.all(promiseOfCollections)

I purposefully awaited inside the for loop for the rate limit problem. My thought process was:

I need to do this in parallel

I should batch

There's no native batch solution

Implementing or installing one would make this PR even bigger

It sounds better to have a slow endpoint than a broken one

What do you think? do you prefer to go with the Promise.all directly?

We can leave it as it for the moment, but we should tackle this issue in the near future.

src/Item/Item.router.ts

src/Item/Item.service.ts

src/Collection/Collection.service.ts

LautaroPetaccio · 2022-01-14T18:32:47Z

src/Item/Item.router.spec.ts

+        url = `/collections/${dbCollectionMock.id}/items`
+      })
+
+      it('should return all the items of a collection that are published with URN and the ones that are not without it', () => {


As the TP items must contain the URN:

Suggested change

it('should return all the items of a collection that are published with URN and the ones that are not without it', () => {

it('should return all the items of a collection that are published with their URN, () => {

LautaroPetaccio · 2022-01-14T18:57:57Z

src/Curation/Curation.router.ts

+      await this.insertCuration(
+        item.collection_id,
+        ethAddress,
+        CurationType.COLLECTION
+      )


Maybe we could create a trigger in the DB to do this, but it's just something to think about.

Agree it could be much better. Triggers sometime make me uncomfortable as they're "hidden" logic that's super hard to debug in production tho. But we could check it out

LautaroPetaccio · 2022-01-21T12:37:24Z

src/ethereum/api/Bridge.ts

+
+  /**
+   * Merges the remote data, which comes from the blockchain, and the catalyst data (if it exists) *into* the db data
+   * The db item is first converted to a full item and then get's the appropiate data merged into it


Suggested change

* The db item is first converted to a full item and then get's the appropiate data merged into it

* The db item is first converted to a full item and then get the appropriate data merged into it

LautaroPetaccio · 2022-01-21T12:46:03Z

src/ethereum/api/Bridge.ts

+  }
+
+  /**
+   * Merges the remote data, which comes from the blockchain, and the catalyst data (if it exists) *into* the db data


What do you think about something like this:

Suggested change

* Merges the remote data, which comes from the blockchain, and the catalyst data (if it exists) *into* the db data

* Combines the remote data (item from the blockchain), the catalyst data (if it exists) and the item's DB data *into* a FullItem.

LautaroPetaccio · 2022-01-21T12:49:42Z

src/Item/Item.service.ts

+    return { dbTPItems, remoteTPItems }
+  }
+
+  public splitItems(


It seems that this JSDoc is missing.

LautaroPetaccio · 2022-01-21T12:57:25Z

src/ethereum/api/Bridge.ts

+      return { item, remoteItem }
+    })
+
+    const tpItemURNs = Object.keys(itemsByURN)


Itms by URN is an array here, so Object.keys won't work. The map above this line should do a reduce over the dbItems to build the itemsByURN map (or record).

nicosantangelo added 18 commits December 16, 2021 17:37

feat: split curation in items/curations

d0f7e5d

chore: param names

9e25a74

feat: create virtual TPW collection curations

bdbce66

feat: add is published to TPW collections (and a bunch of bugs)

8fd4a2d

fix: correctly check tpw collection publishes

f63a0bd

feat: groundwork for item isPublished/isApproved

68e24f2

feat: consolidate items

2b5fd3d

Merge branch 'feat/tpw-is-published' of github.com:decentraland/build…

925db31

…er-server into feat/tpw-is-published

feat: getItem

f2df8e3

feat: finish item and collection endpoints (missing tests)

74ffe17

fix: collection router specs

a4f6b73

WIP

578b68a

WIP

23afbfc

feat: tests

ea7c62d

chore: lint

d38c5a6

Merge branch 'master' into feat/tpw-is-published

b5195e9

feat: tests

059935c

chore: lint

cd46135

nicosantangelo marked this pull request as ready for review January 11, 2022 00:04

nicosantangelo added 3 commits January 11, 2022 01:44

feat: extract getCollectionItems

c5c0700

feat: extract getCollection

67e70c6

feat: remove eth_address and managers from TP elements

f862202

nicosantangelo force-pushed the feat/tpw-is-published branch from 16b7689 to f862202 Compare January 11, 2022 16:54

nicosantangelo added 3 commits January 13, 2022 12:54

chore: fix rebase mixup

97307de

feat: simpler tp consolidation

aafc5a7

fix: script

f8a5a72

LautaroPetaccio requested changes Jan 14, 2022

View reviewed changes

nicosantangelo added 2 commits January 14, 2022 16:43

fix: collection_id instead of id

568dff5

feat: remove repeated migrations. Renames

fd55b10

nicosantangelo added 5 commits January 18, 2022 21:37

feat: remove complexity for service call

9623160

chore: test name

bdaf724

feat: bridge feedback

20d81dd

Merge branch 'master' into feat/tpw-is-published

d7e9d1d

fix: rebase

3417c6a

LautaroPetaccio requested changes Jan 21, 2022

View reviewed changes

fix: feedback

a537d57

LautaroPetaccio approved these changes Jan 24, 2022

View reviewed changes

nicosantangelo merged commit 9a0e31d into master Jan 24, 2022

nicosantangelo deleted the feat/tpw-is-published branch January 24, 2022 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tp is published and is approved #378

feat: tp is published and is approved #378

nicosantangelo commented Jan 3, 2022 •

edited

Loading

coveralls commented Jan 10, 2022 •

edited

Loading

LautaroPetaccio Jan 13, 2022

LautaroPetaccio Jan 13, 2022

nicosantangelo Jan 18, 2022

nicosantangelo Jan 18, 2022

LautaroPetaccio Jan 13, 2022

LautaroPetaccio Jan 14, 2022

nicosantangelo Jan 18, 2022 •

edited

Loading

LautaroPetaccio Jan 19, 2022

LautaroPetaccio Jan 14, 2022

LautaroPetaccio Jan 14, 2022

nicosantangelo Jan 19, 2022

LautaroPetaccio Jan 21, 2022

LautaroPetaccio Jan 21, 2022

LautaroPetaccio Jan 21, 2022

LautaroPetaccio Jan 21, 2022

	const getLastItemQuery = () => gql`
	const getLastReviewedItemQuery = () => gql`

	it('should return all the items of a collection that are published with URN and the ones that are not without it', () => {
	it('should return all the items of a collection that are published with their URN, () => {

	* The db item is first converted to a full item and then get's the appropiate data merged into it
	* The db item is first converted to a full item and then get the appropriate data merged into it

	* Merges the remote data, which comes from the blockchain, and the catalyst data (if it exists) into the db data
	* Combines the remote data (item from the blockchain), the catalyst data (if it exists) and the item's DB data into a FullItem.

feat: tp is published and is approved #378

feat: tp is published and is approved #378

Conversation

nicosantangelo commented Jan 3, 2022 • edited Loading

coveralls commented Jan 10, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1740886696

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicosantangelo Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicosantangelo commented Jan 3, 2022 •

edited

Loading

coveralls commented Jan 10, 2022 •

edited

Loading

nicosantangelo Jan 18, 2022 •

edited

Loading