fix: Simplify curation access #442
Conversation
Pull Request Test Coverage Report for Build 2000808759
💛 - Coveralls |
src/Collection/Collection.model.ts
Outdated
| @@ -25,6 +26,18 @@ export class Collection extends Model<CollectionAttributes> { | |||
| WHERE contract_address = ANY(${contractAddresses})`) | |||
| } | |||
|
|
|||
| static async findCollectionOwningItem( | |||
There was a problem hiding this comment.
I like this name, but I think we were using ByXXX if you want to be consistent ( findByItemId )
There was a problem hiding this comment.
Changed it to findByItemId!
src/Collection/access.ts
Outdated
| @@ -25,7 +25,7 @@ export async function hasAccess( | |||
| isCommitteeMember(eth_address), | |||
| isTPCollection(collection) | |||
| ? thirdPartyAPI.isManager(collection.third_party_id, eth_address) | |||
| : isManager(eth_address, collection), | |||
| : !!collection.is_published && isManager(eth_address, collection), | |||
There was a problem hiding this comment.
I don't know if I understand this condition:
- Why are we casting a (supposedly) boolean value? might be badly typed
There was a problem hiding this comment.
The thing is that isManager should only be called with a merged collection. The code should be something like isMergedCollection(collection) && isManager(eth_address, collection).
I could add this function elsewhere, but I would love to do it with some type changes to collection first.
Check related comment down below.
src/Curation/Curation.router.ts
Outdated
| } else if (error instanceof HTTPError) { | ||
| throw error | ||
| } else { | ||
| throw new HTTPError( |
There was a problem hiding this comment.
Question! why is this the only place where we're doing this? meaning checking for HTTPErrors and throwing a new one otherwise.
It seems like the code in insertCuration should do the same ( if (error instanceof HTTPError) )? and the other catches might need to throw a new HTTPError instead of throw error ?
There was a problem hiding this comment.
You're right! moved it to validateAccessToCuration so the errors can get catched every time.
| @@ -1,5 +1,5 @@ | |||
| import { Model, SQL, raw } from 'decentraland-server' | |||
| import { Collection } from '../Collection' | |||
| import { Collection } from '../Collection/Collection.model' | |||
There was a problem hiding this comment.
Sadly there are a lot of circular dependencies between item and collection. I can't remember if this one was impacting our tests, but I tried to reduce them as much as possible.
| @@ -37,7 +50,8 @@ export async function hasAccess( | |||
| eth_address | |||
| ) | |||
| } else { | |||
| isManager = isCollectionManager(eth_address, collection) | |||
| isManager = | |||
| collection.is_published && isCollectionManager(eth_address, collection) | |||
There was a problem hiding this comment.
same question as here
Also, reading here, it seems odd that checking for is_published seems to be required for calling isManager, if it's a MUST maybe it should be in the function?
There was a problem hiding this comment.
The problem is that there's an issue with the types here. The method is taking CollectionAttributes as the parameter for its type, but CollectionAttributes is not correctly typed, it has properties like managers, is_published and is_approved that don't live in the database, making is_published possibly null.
We need to fix the collection types to be able to represented a MergedCollection containing all of these values and a real CollectionAttributes object, but this PR has gotten pretty long already.
This PR does the following:
getMergedItemrequest as getting the item was not required.getMergedCollectionget a collection instead of an id so it can be used without re-asking for the collection in the database.hasAccessmethods for the items and the collections usable if the collection given to them is not published.