feat: Add URN to collection and item #286

LautaroPetaccio · 2021-09-29T22:29:16Z

This PR does the following:

Adds the urn_suffix column to the collection and the items.
Adds the logic to generate the URNs at the time of retrieving the collections and the items.
Adds the auth middleware for the (GET) /collections/:id path that was missing.
Adds tests for both the items and the collections

Closes #286

…feat/add-urn-to-collection-and-item

nicosantangelo

Great work!

nicosantangelo · 2021-10-07T15:49:34Z

spec/utils.ts

@@ -37,6 +39,24 @@ export const wallet: Wallet = {
  },
 }

+export const collectionAttributesMock: CollectionAttributes = {


You think it'd be a good idea to move this to a seed.ts file? I know it might be too soon but asking just in case we also want to move this

I've moved most of the mocks and functions used to build the results to a mocks directory so we can re-use them afterwards. It is definitely tidier now.

nicosantangelo · 2021-10-07T15:54:32Z

src/Collection/Collection.router.ts

@@ -36,6 +41,7 @@ export class CollectionRouter extends Router {
    const withCollectionExists = withModelExists(Collection, 'id')
    const withCollectionAuthorization = withModelAuthorization(Collection)
    const withLowercasedAddress = withLowercasedParams(['address'])
+    console.log('Auth middleware created', withAuthentication)


Whops! (was it on purpose?)

LOL I completely forgot to remove this.

nicosantangelo · 2021-10-07T16:05:27Z

src/Collection/Collection.types.ts

+  lock: Date | null
+  reviewed_at: Date


This is correct, it was a mistake on my part because node-pg used to return undefined for NULL values. But while we're at it, you mind fixing the type for reviewed_at and forum_link? both are NULLABLE and are not set at first

nicosantangelo · 2021-10-07T16:07:39Z

src/Collection/utils.ts

+    ...dbCollection,
+    urn: getDecentralandCollectionURN(dbCollection.contract_address),
+  }
+  delete (fullCollection as FullCollection & { urn_suffix: unknown }).urn_suffix


we could take advantage of decentraland-common's omit here if you like. Avoiding this delete for something like

const fullCollection: FullCollection = { ...utils.omit(dbCollection, ['urn_suffix']), urn: getDecentralandCollectionURN(dbCollection.contract_address), }

or just

const { urn_suffix, ...sufixlessDbCollection } = dbCollection const fullCollection: FullCollection = { ...sufixlessDbCollection, urn: getDecentralandCollectionURN(dbCollection.contract_address), }

but that might upset the linter

Same goes for toDBCollection with the urn!

nicosantangelo · 2021-10-07T16:11:52Z

src/Collection/utils.ts

+import { getCurrentNetworkURNProtocol } from '../ethereum/utils'
+import { CollectionAttributes, FullCollection } from './Collection.types'
+
+export function getDecentralandCollectionURN(


Question: You think any of these methods might make sense on the CollectionService class? I'm not entirely sure as of right now, but if no, maybe we can think about clear boundaries of these file and the service.

My first intuition is that utils is fine in these cases and if we wanted to extract this, it'd make more sense to do a CollectionBridge of sorts with these methods (overkill)

I'm not entirely sure where this should go, as I think that we would really need more methods like this to have a clear view on how to structure them. The main reason why I'm not sure about moving it into a service is because this method only builds a string that will be used for a collection, but it's not directly involved with collections at all.

Agree, let's keep an eye out tho! maybe even for moving the service methods here!

nicosantangelo · 2021-10-07T16:15:10Z

src/Collection/Collection.router.ts

+    // Build the full collection
+    return consolidatedCollections.map((collection) =>
+      toFullCollection(collection)
+    )
  }

  async getAddressCollections(req: AuthRequest) {


Maybe we can go the Item.router.ts route (heh) here too and add return types to these methods, no?

I'm 100% on board of typing return types. Done!

nicosantangelo · 2021-10-07T16:16:49Z

src/Item/Item.router.ts

+    const catalystItems =
+      remoteItems && remoteItems.length > 0
+        ? await peerAPI.fetchWearables(remoteItems.map((item) => item.urn))
+        : []


Why's this necessary? can remoteItems be undefined? the type says Promise<ItemFragment[]>
peer.ts already has this check:

const wearables: PeerWearable[] = urns.length > 0 ? await this.lambdasClient.fetchWearables({ wearableIds: urns }) : []

same thing here

You're right, there was no need to check for undefined. Removed it in both places.

nicosantangelo · 2021-10-07T16:32:30Z

src/ethereum/api/Bridge.ts

      in_catalyst: false,
      is_approved: false,
      is_published: false,
      total_supply: 0,
    }
+
+    delete (fullItem as FullItem & { urn_suffix: string | undefined })


Maybe we can use utils.omit or destructuring here and here too?

Changed to utils.omit here. For the second one, I chose to use this toFullItem method, as there might be more changes in the future to the item.

…feat/add-urn-to-collection-and-item

coveralls · 2021-10-07T20:29:06Z

Pull Request Test Coverage Report for Build 1317804282

59 of 74 (79.73%) changed or added relevant lines in 11 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+7.1%) to 49.496%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Collection/utils.ts	7	8	87.5%
src/Item/Item.router.ts	4	6	66.67%
src/ethereum/api/Bridge.ts	6	8	75.0%
src/ethereum/utils.ts	8	10	80.0%
src/Item/utils.ts	2	5	40.0%
src/Collection/Collection.router.ts	16	21	76.19%

Files with Coverage Reduction	New Missed Lines	%
src/Collection/Collection.router.ts	1	41.34%

Totals
Change from base Build 1317667898:	7.1%
Covered Lines:	1163
Relevant Lines:	2087

💛 - Coveralls

LautaroPetaccio added 20 commits September 26, 2021 17:07

Initial commit

824b8a8

feat: Add DCL URN for collections

2a9c00b

feat: Add URN for some items

c652d45

feat: Add urn to collection and items

36dcb2a

Merge branch 'master' of github.com:decentraland/builder-server into …

e9e9383

…feat/add-urn-to-collection-and-item

chore: Add URN comment in item

146a1dd

Merge branch 'master' of github.com:decentraland/builder-server into …

66b2eac

…feat/add-urn-to-collection-and-item

chore: Add single collection URN test

a7092cd

chore: Add tests

a0a97cd

chore: Change column to urn_suffix

72ac479

Merge branch 'master' of github.com:decentraland/builder-server into …

877d901

…feat/add-urn-to-collection-and-item

chore: Add item tests

3f37a75

chore: Fix URN column

5fc2a08

fix: Package-lock

17a89c7

Merge branch 'master' of github.com:decentraland/builder-server into …

78e01a9

…feat/add-urn-to-collection-and-item

chore: Add toDBCollection, fix collection tests and fix some item tests

c86c3bc

Merge branch 'master' of github.com:decentraland/builder-server into …

2ee45a8

…feat/add-urn-to-collection-and-item

Merge branch 'master' of github.com:decentraland/builder-server into …

88f749b

…feat/add-urn-to-collection-and-item

fix: Tests

b2d5bb1

fix: Collection controller not having auth

3635ab2

nicosantangelo reviewed Oct 7, 2021

View reviewed changes

LautaroPetaccio added 4 commits October 7, 2021 15:01

fix: PR comments

e74fc58

chore: Removed empty file

64d6ff0

chore: Remove unncessary length check

97f847e

Merge branch 'master' of github.com:decentraland/builder-server into …

075517d

…feat/add-urn-to-collection-and-item

nicosantangelo approved these changes Oct 7, 2021

View reviewed changes

nicosantangelo merged commit e83edaf into master Oct 7, 2021

nicosantangelo deleted the feat/add-urn-to-collection-and-item branch October 7, 2021 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add URN to collection and item #286

feat: Add URN to collection and item #286

LautaroPetaccio commented Sep 29, 2021 •

edited

Loading

nicosantangelo left a comment

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

nicosantangelo Oct 7, 2021

LautaroPetaccio Oct 7, 2021

coveralls commented Oct 7, 2021

feat: Add URN to collection and item #286

feat: Add URN to collection and item #286

Conversation

LautaroPetaccio commented Sep 29, 2021 • edited Loading

nicosantangelo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 7, 2021

Pull Request Test Coverage Report for Build 1317804282

💛 - Coveralls

LautaroPetaccio commented Sep 29, 2021 •

edited

Loading