Update playlist on EntityManager #3417
Conversation
isaacsolo
force-pushed
the
is-delete-playlist
branch
from
a0b7239 to
d0da025
Compare
| dirCID = updatedPlaylistImage.dirCID | ||
| } | ||
|
|
||
| const playlist = (await this.discoveryProvider.getPlaylists(1, 0, [playlistId]))[0] |
There was a problem hiding this comment.
could also check that the playlist owner id here matches the current user id, as an added precaution, but not necessary
There was a problem hiding this comment.
yeah i was wondering if we should include any validation in libs but i figured we'd ignore it for now
There was a problem hiding this comment.
great callout @jowlee - definitely worth adding, but can be done separately
libs/src/api/entityManager.ts
Outdated
| const playlist = (await this.discoveryProvider.getPlaylists(1, 0, [playlistId]))[0] | ||
|
|
||
| const metadata = { | ||
| action, // why include action here? |
There was a problem hiding this comment.
I'm also confused why this is here, would be in favor of removing
libs/src/api/entityManager.ts
Outdated
| } catch (e) { | ||
| logger.error(`Data update playlist: err ${e}`) | ||
| } | ||
| return respValues |
There was a problem hiding this comment.
not sure if on error we want to return - might be confusing from client libs consumption pov, would double check with client team / libs standard on what to do if error case
let respValues = {
blockHash: null,
blockNumber: null,
playlistId: 0
}
```
There was a problem hiding this comment.
One thing we do in other places is add smt called phases example:
audius-protocol/libs/src/api/track.js
Lines 299 to 304 in af7447b
so we know which step it errored on.
I would also imaging returning the error to be a better interface
There was a problem hiding this comment.
yeah it should just error
There was a problem hiding this comment.
AudiusBackend will just catch anything and log it so we could simply throw with a message for now and consolidate error types later. @hareeshnagaraj probably just did this for testing purposes. this PR is just to merge into a feature branch.
There was a problem hiding this comment.
yeah exactly, we can clean it up prior to master merge here
isaacsolo
force-pushed
the
is-delete-playlist
branch
from
21b0ca4 to
6ef5776
Compare
| const playlist = (await this.discoveryProvider.getPlaylists(1, 0, [playlistId]))[0] | ||
|
|
||
| const metadata = { | ||
| action: updateAction, // why include action here? |
There was a problem hiding this comment.
Reason for adding the action was so that we could easily tell what the intent of this metadata was in the case we had to look it up independently for some reason - open to changing this however
Description
Update playlist.
Client change sample: AudiusProject/audius-client@08c8ce7
Complete PLAT-115.
Tests
Monitoring - How will this change be monitored? Are there sufficient logs / alerts?