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

[Suvid-Singhal]

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]

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

[MonkeyDo]

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]

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]

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]

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

[MonkeyDo]

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.

[Suvid-Singhal]

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. 😓

[MonkeyDo]

OK, let's take a step back for a second because it seems you are trying to add stuff to make it work, without a clear plan or knowing what info you are missing where.

Let's think about our issue by analyzing the process step by step.
The scenario is simple: user1 want to thank user2 for a recommendation (event1).
Here's the data we have:

• user1 called the API with their own username: /user/user1/timeline-event/create/thanks so we have the thanker's username
• an event is created in the DB (event2) with the id of user1 (user_id column of the user_timeline_event table), so we have the thanker's user id as well
• the thanks event (event2) metadata contains the original_event_id
• the original event (event1) was created by user2, and in the event1 DB row has the id of user2 in the user_id column
• so far, we have the name and id of user1 from the thanks event, and we have the id of user2 via the original event

The only piece of data we are missing at this point is the name of user2.
There are two options: 1. get user2's name from the original event, using their user_id or 2. store the thankee_username in the thanks event metadata when you create it.

Option 1: To get user2's name, you can retrieve event1 using the original_event_id, using get_user_timeline_event_by_id then get the user name using the user_id of event1, with the get_users_by_id function.

Option 2: when you create the thanks event, either send the thankee_name from the front-end, or on the back-end use the original_event_id and as described above fetch user2's name using the user_id of the original event, then store that in the thanks event metadata in the database.

I think option 2 will be easier and faster, at least at retrieval time (we don't need to fetch the original event or the username of the original event creator if we already store that in the thanks event metadata)

I think with this limited amount of information, we only want to show a simple UI for now, just the line "user1 thanked user2 for recommendation_type"
This can probably be improved but let's cross that bridge when we come to it.

---

Regarding your second attempt, the front-end does not send any thanker_username as part of the metadata JSON (here), and when it is received by the back-end endpoint, the event metadata uses that JSON as-is (here), meaning it is created in the DB without the thanker_username.

When you try to retrieve it from the DB, the APIThanksEvent class expects a thanker_username in the metadata and does not find it, and throws a validation error.

[MonkeyDo]

One last thing :)

For the UI, it's always easiest to first make a mockup of what you want things to look like, before coding it.
That allows you to iterate quickly over various options, get an idea of whether it satisfies what users expect, and also tells you what data you are going to need in order to achieve it.

I usually make my mockups directly in the browser page, modifying the html and css of the page directly from the browser devtools, then take a screenshot of that.

[Suvid-Singhal]

Hi @MonkeyDo,
Thanks a lot for the detailed review. It helped a lot understanding how frontend is communicating with the backend in the codebase. I think its much more clearer to me now.
The backend now works as intended and now it's sending all the 4 pieces of information to the frontend :)

I think now only frontend part is left cuz it looks really bad as of now 😅
Implemented the very basic UI you suggested for now.

Also, should this event be listenable?
Please suggest some UI changes :)

[MonkeyDo]

Hi @MonkeyDo, Thanks a lot for the detailed review. It helped a lot understanding how frontend is communicating with the backend in the codebase. I think its much more clearer to me now. The backend now works as intended and now it's sending all the 4 pieces of information to the frontend :)
I think now only frontend part is left cuz it looks really bad as of now 😅 Implemented the very basic UI you suggested for now. !) Also, should this event be listenable? Please suggest some UI changes :)

I would like you to make a UI mockup as per my last comment.
It is good to learn how to code, but one must also learn to understand how to evaluate the entire feature.

In your case, that means testing the website some more to get a feel for it, and making UI mockups of how you think this feature would best integrate into the feed.

Some things to think about:

• not all thanks will contain a message
• thanks events don't need to be playable (listenable)
• do you want to show the thank event under the original event?
• if not, do you wan to duplicated the original event to show some context?
• if you duplicate the original event, it might make sense to modify its opacity (for example) to give it less importance compared to the thank event

[Suvid-Singhal]

Hi @MonkeyDo,
Thanks a lot for the design inspirations first of all. It really made me think about the product and how can I implement the feature in terms of the UI.

do you want to show the thank event under the original event?

I like this idea as duplicating the event again and again would clutter the user feed. So, this seems to be a better solution.
I tried making this mockup in dev tools:

I can be displayed like a sub-event timeline of the original event.
Its a bit rough as of now. Can add a box for the message but I think that will take too much space if there are multiple thankers.

Or we can do it like this as well:

Clicking the link will reveal the contents in like in the above screenshot and clicking it again would hide them again.

What do you think?

[MonkeyDo]

Good work!
I think I prefer option 1, displayed as a sub-event.
I don't think multiple thank yous are going to be the norm, so it should be fine even in the rare cases where we have multiple thank yous with or without text.

So I like that quite a lot, but one issue is that since I can potentially thank you for a recommendation you made a long time ago, the original event might not be in "page 1" of the feed, but I still want you to see the recent thank you in the top of the timeline.

I think there are two ways to deal with that:
Option A: show the thank event twice, 1. at the expected spot in the timeline, but a short version like in your second mockup (we'll call this one the preview) and 2. under the original event as you did for mockup 1, with the full text
Then we could try to get the preview to link to the original event.

Mockup: note the "recommended a track" is now a link, should take you to the original event, which will look like the second image

And then lower in the page:

Option B: Show the original event multiple times.
This version does the inverse, and shows the thank you events at their normal place in the timeline, with an option to expand it to show the original event it refers to. That way would be simpler, we just need to load the original events separately when clicking to expand the thank you.

Mockup:

And after clicking the button:

I think for both of the options, you will need to send the created timestamp of the original event in the thankyou event metadata.
This is because from the front-end, the only identifying piece of info we can use to fetch an event is the timestamp.

[Suvid-Singhal]

I think we can go ahead with Option B as it seems easier to implement 😛 and also looks good enough. I think it kinda makes more sense as we won't be putting the thanks event twice in the timeline. And yea, I didn't consider the case when someone thanks a really old recommendation/pin.
Btw your mockups look so good 🔥
Will try to make better mockups next time :)

Recommendations and thank you's do look like a really helpful feature ngl, would love to have it on Spotify as well xD
Ok, so I think I'll start working on it by tomorrow.