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.

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

Fix album notification entity type #7580

Merged
merged 7 commits into from
Feb 22, 2024
Merged

Conversation

isaacsolo
Copy link
Contributor

@isaacsolo isaacsolo commented Feb 13, 2024

Description

Fixes C-3785

Discovery isn't propagating is_album so client is unable to discern album from playlist notifications. Only album milestone and of_repost notifs are affected.

How Has This Been Tested?

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

Tested on sandbox.

/v1/full/notifications example

{
    "type": "milestone",
    "group_id": "milestone:PLAYLIST_SAVE_COUNT:id:1481913973:threshold:10",
    "is_seen": true,
    "seen_at": 1707785740,
    "actions": [
        {
            "specifier": "0MMZ6",
            "timestamp": 1707785647,
            "data": {
                "type": "playlist_save_count",
                "threshold": 10,
                "playlist_id": "qy9qx3E",
                "is_album": false
            }
        }
    ]
},

Screenshot 2024-02-13 at 11 30 00 AM
Screenshot 2024-02-20 at 12 08 49 PM

@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 13, 2024
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/is-fix-album-notification

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 13, 2024
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/is-fix-album-notification

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/is-fix-album-notification

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

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

nice improvement :) we could possibly do the same for track vs podcast/audiobook sometime.

on conflict do nothing;


if entity_type = 'track' then
Copy link
Contributor

Choose a reason for hiding this comment

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

did we have to break this out because we need extra param for playlist/albums json_build_object, so we couldnt combine track + playlist like we did before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup exactly

Copy link
Contributor

@amendelsohn amendelsohn left a comment

Choose a reason for hiding this comment

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

Looks great! Just need the one suggested change before merge. Thanks again for taking on the fix for this

@@ -2101,7 +2101,7 @@ export const audiusBackend = ({
}
return {
type: NotificationType.Milestone,
entityType: Entity.Playlist,
entityType: data.is_album ? Entity.Album : Entity.Playlist,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also need to make this change in this helper function in the same file:

  function getDiscoveryEntityType(type: string) {
    if (type === 'track') {
      return Entity.Track
    }
    return Entity.Playlist
  }

This should cover the other notification types listed on the ticket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea it's kind of a confusing state because not all notif types have is_album but i'll update for it to be optional. we actually getting the album entity type downstream here:

const entityTypeText =
entity && 'is_album' in entity && entity.is_album
? Entity.Album
: entityType

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 22, 2024
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/is-fix-album-notification

Copy link

gitguardian bot commented Feb 22, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9349019 Triggered Generic High Entropy Secret b828e54 packages/ddex/.env.stage View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 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!

@isaacsolo isaacsolo enabled auto-merge (squash) February 22, 2024 23:53
@isaacsolo isaacsolo merged commit cf8ad1b into main Feb 22, 2024
9 of 13 checks passed
@isaacsolo isaacsolo deleted the is-fix-album-notification branch February 22, 2024 23:57
audius-infra pushed a commit that referenced this pull request Feb 24, 2024
[3ff422f] [PROTO-1513] Setup e2e test stub with LocalStack, wire it up to CI, and automate Mongo init (#7705) Theo Ilie
[e705bcf] Allow empty adc auto-deploy commits (#7709) Michelle Brier
[ebf162f] [PAY-2523] CollectionTrackRelations migration for sellable albums (#7677) Reed
[7fcc5f9] Fix harmony system-appearance (#7706) Dylan Jeffers
[f8df062] Update welcome drawer close functionality to always open the notifications drawer (#7704) Kyle Shanks
[1845dfc] Remove flipper from android and update readme (#7703) Sebastian Klingler
[ced4135] Add documents path to asset url check (#7702) Randy Schott
[c3e46e6] Bump version to 0.6.45 audius-infra
[81383fd] [C-3879] Remove web Text component (#7686) Dylan Jeffers
[138f7c8] Fix half-load of spring animation on all <Page> components (#7692) Raymond Jacobson
[cf8ad1b] Fix album notification entity type (#7580) Isaac Solo
[4f2a415] [Web] Fix red caret on Transactions History page (#7693) nicoback2
[50156e5] [PAY-2435] Remove pointer cursor when dropzone at max capacity (#7695) Saliou Diallo
[ea3365a] [C-3838] Analytics events for mobile sign up (#7655) JD Francis
[40edef7] [C-3877] Remove web icon component (#7674) Dylan Jeffers
[58c7e47] [C-3880] Add bottom padding to the safe area on the sign in screen (#7689) Kyle Shanks
[3e7f71e] Fix app site association (#7694) Sebastian Klingler
[4f92b88] [C-3855] Fix android white screen issue on sign up (#7683) JD Francis
[596f9cc] Add remote redux debugging to mobile app (#7687) Sebastian Klingler
[df30494] Fix amplitude key injection on mobile (#7690) Dylan Jeffers
[f8c4f43] [PAY-2513] Fix file size text wrap on web DownloadRow (#7688) Reed
[7084823] [PAY-2517] Change name of temp download dir on mobile (#7671) Reed
[051d95f] [PROTO-1668] placement for mediorum uploads (#7668) Steve Perkins
[17f9536] [C-3857] Remove the shadow on the verified handle section in new sign up flow (#7681) Kyle Shanks
[0d6556a] [INF-659] Remove flipper and enable experimental debugger (#7685) Sebastian Klingler
[336dc7a] [PROTO-1669] Find Mongo changes when offline and clean up DDEX (#7680) Theo Ilie
[043c3a5] [C-3863] Increase touch target of close button of artist preview banner (#7666) Kyle Shanks
[b2a0609] [C-3881] Add apiKey to the jwt payload (#7684) Raymond Jacobson
[dc41e93] [Discovery] Add year time range for metrics endpoints [C-3753] (#7676) nicoback2
[c104b7e] [C-3854] Update size of social buttons in new sign up flow (#7682) Kyle Shanks
[f772ae8] [C-3859] Fix mobile handle validation quirks (#7664) JD Francis
[21f5d8e] [DVRL-10] Ensure history uses track activity model (#7678) Raymond Jacobson
[3a8b27a] Bump version to 0.6.44 audius-infra
[eb858ee] Upgrade dependencies used in alpine base image (#7675) Reed
[9824ddc] [PROTO-1670] Find userId in DDEX parser and remove artist allowlist (#7662) Theo Ilie
[fc4ead6] ⚠️ [PAY-2421] Upload stems in parallel (#7644) Marcus Pasell
[831ca36] [C-3870] Update AccountHeader handle text to be one line (#7672) Kyle Shanks
[5c87774] [C-3876] Remove stems icons (#7670) Dylan Jeffers
[dded48a] Pin dnspython to version 2.5.0 (#7673) Isaac Solo
[ec07ff1] [C-3861] Remove default padding on text inputs for andoird (#7669) Kyle Shanks
[3c217da] [C-3873] Migrate to harmony IconButton (#7659) Dylan Jeffers
[a8ff559] [PAY-2229] Add lossless UI metrics (#7665) Saliou Diallo
[0c37ede] Revert "PROTO-1574: sepolia config and register all nodes" (#7667) alecsavvy
[51cdfd6] [DVRL-10] Add auth middleware sdk changes for history (#7663) Raymond Jacobson
[4ec621f] Bump version to 0.6.43 audius-infra
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants