Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update playlist on EntityManager #3417

Merged
merged 6 commits into from
Jul 13, 2022
Merged

Update playlist on EntityManager #3417

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8b8fc31
wip
isaacsolo Jul 6, 2022
7224a95
Add delete playlist to audius data libs + indexing
isaacsolo Jul 7, 2022
d0da025
update playlist
isaacsolo Jul 10, 2022
ef1f215
clean
isaacsolo Jul 11, 2022
6ef5776
add types
Jul 11, 2022
60281dd
Merge branch 'hn_audius_data_write_path' into is-delete-playlist
hareeshnagaraj Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions discovery-provider/src/tasks/audius_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ def audius_data_state_update(
txhash,
)

if action == "Create":
playlist_id = entity_id
if action == "Create" or action == "Update":
isaacsolo marked this conversation as resolved.
Show resolved Hide resolved
playlist_record = parse_playlist_create_data_event(
update_task,
entry,
Expand All @@ -104,9 +103,7 @@ def audius_data_state_update(
"playlist"
] = playlist_record
playlist_events_lookup[playlist_id]["events"].append(event_type)

playlist_ids.add(playlist_id)

processed_entries += 1

num_total_changes += processed_entries
Expand Down
78 changes: 78 additions & 0 deletions libs/src/api/entityManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,84 @@ export class EntityManager extends Base {
}
return respValues
}
/**
* Update a playlist using updated data contracts flow
**/
async updatePlaylist({
playlistId,
playlistName,
trackIds,
description,
isAlbum,
isPrivate,
coverArt,
logger = console
}: {
playlistId: number
playlistName: string
trackIds: number[]
description: string
isAlbum: boolean
isPrivate: boolean
coverArt: any
logger: any
isaacsolo marked this conversation as resolved.
Show resolved Hide resolved
}): Promise<{ blockHash: any; blockNumber: any; playlistId: number }> {
isaacsolo marked this conversation as resolved.
Show resolved Hide resolved
let respValues = {
blockHash: null,
blockNumber: null,
playlistId: 0
}
try {
const userId: number = parseInt(this.userStateManager.getCurrentUserId())
const action = 'Update'
const entityType = 'Playlist'
isaacsolo marked this conversation as resolved.
Show resolved Hide resolved
this.REQUIRES(Services.CREATOR_NODE)
// if (playlistName){

// }
isaacsolo marked this conversation as resolved.
Show resolved Hide resolved
let dirCID;
if (coverArt) {
const updatedPlaylistImage = await this.creatorNode.uploadImage(
coverArt,
true // square
)
dirCID = updatedPlaylistImage.dirCID
}

const playlist = (await this.discoveryProvider.getPlaylists(1, 0, [playlistId]))[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also check that the playlist owner id here matches the current user id, as an added precaution, but not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i was wondering if we should include any validation in libs but i figured we'd ignore it for now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great callout @jowlee - definitely worth adding, but can be done separately


const metadata = {
action, // why include action here?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also confused why this is here, would be in favor of removing

entityType,
playlist_id: playlistId,
playlist_contents: trackIds || playlist.playlist_contents,
playlist_name: playlistName || playlist.playlist_name,
playlist_image_sizes_multihash: dirCID || playlist.playlist_image_sizes_multihash,
description: description || playlist.description,
is_album: isAlbum || playlist.is_album,
is_private: isPrivate || playlist.is_private
}
const { metadataMultihash } = await this.creatorNode.uploadPlaylistMetadata(metadata)

const resp = await this.manageEntity({
userId,
entityType,
entityId: playlistId,
action,
metadataMultihash
})
logger.info(`UpdatePlaylistData - ${JSON.stringify(resp)}`)
isaacsolo marked this conversation as resolved.
Show resolved Hide resolved
const txReceipt = resp.txReceipt
respValues = {
blockHash: txReceipt.blockHash,
blockNumber: txReceipt.blockNumber,
playlistId: entityId
}
} catch (e) {
logger.error(`Data update playlist: err ${e}`)
}
return respValues
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    }
    ```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const phases = {
GETTING_USER: 'GETTING_USER',
UPLOADING_TRACK_CONTENT: 'UPLOADING_TRACK_CONTENT',
ADDING_TRACK: 'ADDING_TRACK',
ASSOCIATING_TRACK: 'ASSOCIATING_TRACK'
}

so we know which step it errored on.
I would also imaging returning the error to be a better interface

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it should just error

Copy link
Contributor Author

@isaacsolo isaacsolo Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah exactly, we can clean it up prior to master merge here

}

/**
* Manage an entity with the updated data contract flow
Expand Down