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

LB-1688 Feature to thank a user for a pin/recommendation #3143

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

Suvid-Singhal
Copy link
Contributor

Problem

This feature aims to add a functionality to thank users for their pins/recommendations
This feature is a work in progress as of now
Backend is mostly complete but frontend needs some work.

The feature request ticket is: LB-1688

Solution

Implemented backend as of now. Didn't have to change the DB schema for it.

Action

I need some help in some places for frontend in order to complete this.

@pep8speaks
Copy link

pep8speaks commented Jan 23, 2025

Hello @Suvid-Singhal! 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 2025-01-27 13:01:05 UTC

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thank you for opening the PR!

There are a couple of things missing before I can test the feature:

Currently, I am recieveing an error on the back-end and two on the front-end.
The back-end one is crashing the web container preventing my testing.

Back-end error:

File "/code/listenbrainz/listenbrainz/webserver/views/user_timeline_event_api.py", line 731
web-1 | raise APIBadRequest(f"{metadata['event_type']} event with id {
web-1 | SyntaxError: unterminated string literal (detected at line 731)

Front-end errors:

First one is easy and just needs renaming the thankFeedEvent method to avoid name collision:

/code/frontend/js/src/user-feed/UserFeed.tsx
static_builder-1 | 270:15 error 'thankFeedEvent' is already declared in the upper scope on line 268 column 9

Second issue is a typescript type error:

ERROR in ./frontend/js/src/user-feed/UserFeed.tsx:277:27
static_builder-1 | Found 1 error in 13396 ms.
static_builder-1 | TS2339: Property 'blurb_content' does not exist on type 'EventMetadata'.
static_builder-1 | Property 'blurb_content' does not exist on type 'Listen'.

I think at the core of this issue is a misunderstanding of where the thanking logic goes.
The way it is currently set up, clicking on "Thank Event" tries to send a non-existing blurb content via the API (the event you are thanking might not have a blurb content, and if it has one it's not the thank you message).
So instead, clicking on the button should open a modal where the user can write an optional thank you message. The logic for sending the thank-you with the optional blurb content should be implemented in the modal in question instead of the UserFeed page.
A good example of how to do all this is to see how the PersonalRecommendationModal is used in the ListenCard component, and basically use that as an example for the action button and the new modal.

@Suvid-Singhal
Copy link
Contributor Author

Hi @MonkeyDo
The backend error was caused by linter 😅
I have fixed it now
Thanks for your detailed inputs for the frontend. Yes, indeed I had misunderstood about where I had to put the logic for thanking logic should have been put. I have now fixed that as well. Made a new file for the ThanksModal.
I think the frontend is fine to test now. Just need some minor cosmetic changes I believe. The backend also seems to be fine now.

The issue I am facing now is with database. I am getting Database exception while testing. I will have to write SQL scripts and run them on my system to test it but I can't figure it out for now. Will try to fix that as well.

Could you please test backend and frontend part and let me know where changes are needed for now?

@MonkeyDo
Copy link
Member

I think I know where the DB errors are coming from actually: the 'thanks' event type is missing in the user_timeline_event_type_enum.
You'll need to modify the create_types.sql file (for new DB creation) as well as creating an sql update file that adds the type in the existing enum (for modifying existing DBs)

@Suvid-Singhal
Copy link
Contributor Author

Hi @MonkeyDo,
So I updated the DB now but still I'm facing 500 internal server error. I'm not able to figure out what might be causing them :(
Could you please help me out?
The error message suggests that it is being caused due to DatabaseException only I think.
Thanks

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

This set of changes should resolve the blocking issues.

For reference, I just put current_app.logger.info(... calls at regular intervals to follow where the issue was coming from and isolate the bits of code that were failing, one by one.

While I am here, without doing a full code review of the front-end parts, the UI is looking great. All that is missing is to show the thank events on both users' timelines.

listenbrainz/webserver/views/user_timeline_event_api.py Outdated Show resolved Hide resolved
listenbrainz/webserver/views/user_timeline_event_api.py Outdated Show resolved Hide resolved
listenbrainz/webserver/views/user_timeline_event_api.py Outdated Show resolved Hide resolved
@Suvid-Singhal
Copy link
Contributor Author

Hi @MonkeyDo,
It's mostly working now. The event shows up on the feed now but it does not show the user who is thanking and the one being thanked properly. It just shows user thanking only for both. Tried using user_name, currentUser.name and event.name as well. So I tried to include a field in metadata called thanker_username. But after adding it, it does not even seem to fetch any thanks events from the DB.

I'm kinda confused on how to proceed.
Should I event include it in the first place in metadata or there seems to be some other way?
Really sorry for so many problems as it's my first time working on backend and frontend together in such a large codebase. 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants