Skip to content
This repository was archived by the owner on Oct 4, 2023. It is now read-only.

Update playlist actions to use entity manager#1674

Merged
isaacsolo merged 26 commits into
mainfrom
is-entity-manager
Aug 20, 2022
Merged

Update playlist actions to use entity manager#1674
isaacsolo merged 26 commits into
mainfrom
is-entity-manager

Conversation

@isaacsolo

@isaacsolo isaacsolo commented Aug 4, 2022

Copy link
Copy Markdown

Description

Use entity manager for playlist actions. This migration will be behind a feature flag. Playlists will also now have metadata uploaded to CNs.

https://www.notion.so/audiusproject/Data-Layer-Migration-EVM-27236549dccb4ed59ec78a1960b00239

Protocol changes:
AudiusProject/apps#3400
AudiusProject/apps#3719

Completes PLAT-272.

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

How Has This Been Tested?

Tested locally.
Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

How will this change be monitored?

Will merge after soaking in staging.

For features that are critical or could fail silently please describe the monitoring/alerting being added.

Feature Flags

Are all new features properly feature flagged? Describe added feature flags.

PLAYLIST_ENTITY_MANAGER_ENABLED for enabling playlist updates via entity manager.

@isaacsolo isaacsolo requested a review from jowlee August 4, 2022 19:06
@isaacsolo isaacsolo marked this pull request as ready for review August 4, 2022 19:06
@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/is-entity-manager

@isaacsolo isaacsolo requested a review from dylanjeffers August 13, 2022 00:08
@gitguardian

gitguardian Bot commented Aug 17, 2022

Copy link
Copy Markdown

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

Comment thread packages/web/.env/.env.dev Outdated
Comment thread packages/web/src/common/store/cache/collections/sagas.js
if (!playlistEntityManagerIsEnabled) {
yield put(
cacheActions.update(Kind.COLLECTIONS, [
{ id: uid, metadata: { _moved: subscribedUid } }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need any of this _moved logic?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

i think it's still necessary for the entity manager disabled case. just trying to keep it the same as it was before.

Comment thread packages/web/src/utils/tempPlaylistId.ts

@nicoback2 nicoback2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm pending a few comments!

Comment on lines -321 to +348
function* (confirmedPlaylistId) {
function* () {
const { blockHash, blockNumber, error } = yield call(
audiusBackendInstance.updatePlaylist,
confirmedPlaylistId,
playlistId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we gate this change behind the feature flag or does it work fine with the old update pattern?

{
operationId: PlaylistOperations.ADD_TRACK,
parallelizable: true,
useOnlyLastSuccessCall: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should add this back for the non entity manager case:
useOnlyLastSuccessCall: !playlistEntityManagerEnabled

parallelizable: true,
useOnlyLastSuccessCall: true
parallelizable: !playlistEntityManagerIsEnabled,
squashable: playlistEntityManagerIsEnabled

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ditto re: comment above about adding back useOnlyLastSuccessCall

Comment on lines 783 to 785
playlist.playlist_contents.track_ids =
updatedPlaylist.playlist_contents.track_ids

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you just remove this and pass updatedPlaylist below? (Personally prefer not mutating objects directly, tends to be bug prone)

Comment on lines +702 to +705
const tempPlaylistId = getTempPlaylistId()
const { blockHash, blockNumber, playlistId, error } = yield call(
audiusBackendInstance.createPlaylist,
tempPlaylistId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be totally misunderstanding this but doesn't this playlist Id become the real id? So maybe we should get rid of the temp prefix everywhere?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yea that's true for the new flow but it's still actually a temp ID for the legacy playlist flow so i wanted to avoid confusion there. i'll add a comment!

@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/is-entity-manager

@isaacsolo isaacsolo merged commit ca202fe into main Aug 20, 2022
@isaacsolo isaacsolo deleted the is-entity-manager branch August 20, 2022 00:38
audius-infra pushed a commit that referenced this pull request Aug 27, 2022
[ccf1ca9] [C-906] Migrate queue sagas and hls-utils to common (#1805) Dylan Jeffers
[fe441af] Consume new Text Button in collapsible content (#1790) Marcus Pasell
[2ee3b2d] Add error UI to InProgress page of BuyAudio modal (#1802) Marcus Pasell
[139e50e] Add TextButton story (#1803) Marcus Pasell
[a4d26cf] Add all the pages to the BuyAudioModal (#1800) Marcus Pasell
[15dfb72] Update package.json (#1801) Raymond Jacobson
[284431d] [C-817] Deep link /settings on mobile (#1796) Raymond Jacobson
[2b7ca95] [PAY-44] Rename getAssociatedTokenAccountInfo and getUserBank (#1794) Reed
[ff07bb4] [PAY-580] Add Success Page to BuyAudio Modal (#1791) Marcus Pasell
[51269bb] [C-901] Move audioPlayer to saga context (#1797) Dylan Jeffers
[dfc6f08] [C-900] Move queue, lineup, feed-page sagas to common (#1795) Dylan Jeffers
[4f25517] [C-897] Add explore service to saga context (#1793) Dylan Jeffers
[8cc3a29] Move sagaHelpers to common (#1792) Sebastian Klingler
[8ee3aa8] Add Text Button type to Stems (#1789) Marcus Pasell
[ff4f913] [PAY-552] Change playllenge challenge button text (#1787) Reed
[60ed26a] Fix bug with renamed component (#1788) Marcus Pasell
[718f35a] Dynamically resize ModalContentPages (#1785) Marcus Pasell
[4903c9f] [PAY-577] Add "in progress" page to Buy Audio Modal (#1783) Marcus Pasell
[72fc4fd] Migrate queue selectors to common (#1786) Dylan Jeffers
[5f3e46e] [C-871] Migrate player state to common (#1784) Dylan Jeffers
[8d77abf] [PAY-476] Display error when send to wallet fails (#1780) Reed
[6adbdda] [PAY-575] [PAY-574] [PAY-573] [PAY-572] [PAY-581] Coinbase Pay Button and Rewards Page Fixes (#1778) Marcus Pasell
[56c3f39] [PAY-458] Buy Audio Modal Input Page (#1777) Marcus Pasell
[172d6c4] Cleans up code for Challenge Twitter icon fill color (#1781) Reed
[bd1a329] Fix order playlist to use updated playlist (#1774) Isaac Solo
[1f178bc] [C-887] Host sourcemaps on s3, fixes cloudflare deploy (#1776) Sebastian Klingler
[0551dec] [PAY-571] Move Jupiter out of bundle, attach BuyAudio sagas (#1772) Marcus Pasell
[db9cd4a] [C-890] Polyfill TextEncoder, fixes builds (#1775) Dylan Jeffers
[de8c73a] Update @coinbase/cbpay-js to v1.2.0 and @solana/web3.js to 1.53.0 (#1773) Marcus Pasell
[bdec4b7] Add entity manager address to configInternalWeb3 (#1763) nicoback2
[d3fae31] Wire up initial BuyAudioModal (#1770) Marcus Pasell
[fc5e484] Add back modal hook functionality (#1769) Marcus Pasell
[ae616cc] Fix @audius/common external warnings, model imports, lint (#1767) Dylan Jeffers
[2a121e4] Add entity manager address for prod env (#1766) Isaac Solo
[b0c00f1] [C-875] Remove blocklist (#1764) Dylan Jeffers
[859df1f] [RN Reloaded] Migrate web common/store and common/hooks to @audius/common (#1675) nicoback2
[bdd9e03] [PAY-546] Add memo instruction to final onramp transfer, fix rent min problem (#1731) Marcus Pasell
[635a6ed] [PAY-431] Slide in supporters after loading (#1720) Reed
[2af212a] [PAY-438] Fix Twitter icon fill color in matrix mode (#1760) Reed
[bd925e1] Add pill radio button (#1745) Marcus Pasell
[0392854] Remove eager discprov logs (#1762) Dylan Jeffers
[b6733ef] Update @audius/sdk to 0.0.38 (#1757) Isaac Solo
[a5e941e] [C-862] Add env to saga context, fix eagerDiscprov in native (#1753) Dylan Jeffers
[873ed99] Remove /macro usage for typed-redux-saga (#1751) (#1755) Dylan Jeffers
[ca202fe] Update playlist actions to use entity manager (#1674) Isaac Solo
[48ae001] Fix margin issue (#1752) Raymond Jacobson
@AudiusProject AudiusProject deleted a comment from linear Bot Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants