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

Add pin events to user timeline (backend) #1559

Merged
merged 10 commits into from
Aug 9, 2021

Conversation

jdaok
Copy link
Contributor

@jdaok jdaok commented Jul 21, 2021

(this PR doesn't depend on any currently opened PR's and can be merged)
This PR contains the backend changes to add pin events to the user timeline.

Changes:

  • adds get_pins_for_feed() to pinned_recording.py to retrieve pins for list of users
  • adds new class APIPinEvent to APIEventMetadata
  • adds get_recording_pin_events() to user_timeline_event_api.py to fetch pins and convert to APITimelineEvents

frontend changes -> #1560

This function a list of PinnedRecordings for a list of users in descending order of their created date. this will be used to converted to timeline events to render pin events on user feeds
@pep8speaks
Copy link

pep8speaks commented Jul 21, 2021

Hello @jdaok! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-08 05:00:14 UTC

@jdaok jdaok changed the title Pin timeline backend Add pin events to user timeline (backend) Jul 21, 2021
events = []
for pin in recording_pin_events_db:
try:
pin = fetch_track_metadata_for_pin(pin)
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of a problem here -- given that the fetch_track_metadata_for_pin is in a loop, it means that we're going to do 1 DB query for each pin event, which is far from ideal. We need to support doing this in one query -- one possibility is to refactor the fetch_track_metadata_for_pin to accept a list of pins, rather than just one.

Copy link
Contributor Author

@jdaok jdaok Jul 30, 2021

Choose a reason for hiding this comment

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

Thanks, I opened #1570 to refactor the functions and fix this, after that gets approved I'll merge changes here and we should be able to merge this and #1560 into master.

Copy link
Member

Choose a reason for hiding this comment

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

OK, 1570 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Fix the merge conflict on 1560 and lets get this merged!

Copy link
Member

@mayhem mayhem left a comment

Choose a reason for hiding this comment

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

Looks very good overall, we just need to refactor that one (two?) function(s).

@mayhem mayhem merged commit 7dd6a42 into metabrainz:master Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants