LB-1230: Added ability to edit comment after pinning a Listen

[lzzy12]

Problem

Fixes this Ticket:
https://tickets.metabrainz.org/browse/LB-1230

Solution

Added an extra action for pinned listen:

[pep8speaks]

Hello @lzzy12! 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 2024-03-05 10:23:40 UTC

[MonkeyDo]

Thanks for opening a PR! It's looking quite good !

I did a first round of review without running the code, but it's looking functional already.
Some questions about return value and statuses below:

[lzzy12]

Hi, thanks for your feedbacks. I will work on them

[lzzy12]

@MonkeyDo I have pushed an update. Please have a look when you feel better :)

[MonkeyDo]

Thanks for the modifications, I have a couple more comments but it's looking nice !

[MonkeyDo]

Thanks for those changes, the code looks great now !

I deployed the PR to our test server, and it's working nicely! I was able to update a comment without any issue, nice work !

Through testing I do have two more pieces of feedback from a user perspective. Sorry about not realizing sooner !

• It would be useful to have the current pin comment in the textarea when I open the modal to edit the pin. Currently to update a typo I would have to copy the comment first. Left some comments about that below
• I would want the UI to update so that the new text content is shown on the pin card on the page. Also left some comments to that effect, but basically we can consider the call modal.show( as a promise and make the modal return a value when the API call succeeds (in this case the updated text).

[MonkeyDo]

Here we also want to pass pinnedRecording.blurb_content as a new initialBlurbContent

[MonkeyDo]

new initialBlurbContent?: string; prop required here

[MonkeyDo]

Set initial blurb content here, fallback to the empty string as before:

Suggested change

	const [blurbContent, setBlurbContent] = React.useState("");
	const [blurbContent, setBlurbContent] = React.useState(initialBlurbContent ?? "");

With the other related changes that should mean you have the existing content in the textarea when you open the modal

[MonkeyDo]

Here we can have the modal return the updated text so we can update the UI accordingly:

Suggested change

	);
	);
	modal.resolve(blurbContent);

I'll leave a comment in PinnedRecordingCard for how to use the returned value.

I don't know if we need to return anything when there is an error, in the catch block below.

[MonkeyDo]

There is a good example in the codebase of how to use that return value in the PinnedRecordingCard component:

listenbrainz-server/frontend/js/src/user/playlists/Playlists.tsx

Lines 171 to 175 in c8b5abf

	NiceModal.show(CreateOrEditPlaylistModal)
	// @ts-ignore
	.then((playlist: JSPFPlaylist) => {
	this.onPlaylistCreated(playlist);
	});

That also means you will have to store pinnedRecording in the component state of PinnedRecordingCard rather than using it from the props.

You'll need a new class method updatePinnedRecording or something like that where you update the state accordingly, similar to what we do here:

listenbrainz-server/frontend/js/src/user/playlists/Playlists.tsx

Lines 96 to 101 in c8b5abf

	onPlaylistCreated = async (playlist: JSPFPlaylist): Promise<void> => {
	const { playlists } = this.state;
	this.setState({
	playlists: [playlist, ...playlists],
	});
	};

Except since pinnedRecording is an object we want to merge properties, something like:

    this.setState( currentState => ({
      pinnedRecording: {
        ...currentState.pinnedRecording,
        blurb_content: updatedBlurbContent},
    }));

[lzzy12]

Actually the current behavior was inlined to what happens when we pin a track. It only shows up in pin when the user refreshes. I have observed this behavior at multiple places in the webapp. That was the reason I tried to keep it inlined to that behavior.
I will work on it to implement the suggested changes

[MonkeyDo]

Actually the current behavior was inlined to what happens when we pin a track. It only shows up in pin when the user refreshes. I have observed this behavior at multiple places in the webapp. That was the reason I tried to keep it inlined to that behavior.
I will work on it to implement the suggested changes

Yes, I didn't want to muddy the waters by mentioning it, but I did realize the pin is not updated on the page when pinning a track, which is definitely not great UX :)

I think since you are working on that feature, and since you noticed it yourself, you will be in a great position to fix that too in your next PR, if you are interested in doing that.

[MonkeyDo]

Hi @lzzy12 !
Just checking in to see if you have some time to finish this PR, or if you would rather pass the baton to someone else.
Either way is fine by me, please let me know :)

[lzzy12]

Sorry got caught up with something. Will be finishing it this weekend