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

Listening now: allow users to vote on and add tags #2475

Merged
merged 49 commits into from
Jul 31, 2023

Conversation

MonkeyDo
Copy link
Contributor

@MonkeyDo MonkeyDo commented May 5, 2023

This PR takes over from #2072 which had become very much out of date.

This new branch is rebased on fresh master, and implements the missing pieces of this new feature (auth token, refresh and retry mechanism).

User can now vote on and add tags from the front-end !
For now, until we finesse a few details* this is only available on the Listening Now page as a test bed.
Allows users to vote on existing tags
image
Also allows user to create their own freetext tags:
image
image

Select/search for a tag from the MusicBrainz genres :
image

* < sarcasm > small details like how the tags will look, where we show them, which entities do we want to tag and other trivial points < /sarcasm >

Also left to implement is a way to show the current user's tags (and votes?) when we load or refresh the page.
This will require a lot more API calls I suppose.

MonkeyDo added 13 commits May 4, 2023 14:36
always pass an entity MBID, which we need for the add tag component
Actually enable users to write a new tag, and stub implementation of submission mechanism
Yada yada XML yada yada validation yada yada auth token.
Auth token is currently missing, will need to wait for  LB-1120 to be implemented.
We can't generate the callback frunction from a TagActionType array.map to refactor, the callback functions have to be defined separately, duplicating some code.

See https://react.dev/reference/react/useCallback#i-need-to-call-usememo-for-each-list-item-in-a-loop-but-its-not-allowed
1. Use the right auth header (Bearer)
2. refresh token + retry if we get an unauthorized status
Merging is hard when you change the directory structure drastically.
In this case we had a types file left over that didn't merge.
Applying the necessary changes, and deleting the extraneous file.
MonkeyDo added 12 commits May 5, 2023 18:10
I was expecting a format for RG tags (entity MBID at tag. release_group_mbid) like for artist tags, which didn't materialize, so there was no entity MBID available
In the Listening Now page for now, perhaps this should be handled and served from the back-end instead.
Use the pre-defined list of MB genres to select from (searchable select).
Updates the react-select component while I'm in there, which isn't used anywhere else as far as i can tell.
I think we can only set items on the global context when we first instantiate it
instead of the object returned by react-select
Using only react-select and some heavy customization
+ improvements and adaptations to the TagComponent
+ separate into its own file
Preparing to make them reusable
Don't allow to vote + disable input if entity MBID is missing (no matched MBID)
Comment on lines +135 to +144
try {
const response = await fetch(
"https://musicbrainz.org/ws/2/genre/all?fmt=txt"
);
const genresList = await response.text();
globalAppContext.musicbrainzGenres = Array.from(genresList.split("\n"));
} catch (error) {
console.error(error);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a follow-up PR, we should probably load those as props from the back-end, and cache them aggressively?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me, Maybe a ticket or ping me tomorrow and I'll add it.

@MonkeyDo MonkeyDo marked this pull request as ready for review July 3, 2023 16:11
remove useEffect used in TagsComponent
woopsie!
Previous implementation?
In any case, method is unused and the /profile/mbtoken doesn't seem to exist anymore.
Only show genre tags, in order of vote count, as well as the user's own tags.
We filter out other users' non-genre tags, because it can quickly become too much.
Couldn't see any error messages otherwise, duh.
Don't lose tag rounded corners, + tweak colors a bit.
Mistakes were made !
We need the artist_mbid for example from the original tag
@MonkeyDo MonkeyDo merged commit ffb470c into master Jul 31, 2023
4 checks passed
@MonkeyDo MonkeyDo deleted the listening-now-tags-2 branch July 31, 2023 16:26
@UltimateRiff
Copy link

I wonder if we could add support for tagging works here as well? made a Jira ticket as well LB-1326 as a gentle reminder more broadly

@mayhem
Copy link
Member

mayhem commented Aug 14, 2023

Yes, that has been in the works for a while, but the PR is not quite ready for merging yet, IIRC.

@mayhem
Copy link
Member

mayhem commented Aug 14, 2023

I stand corrected -- the PR was merged, but due to an unrelated bug, this feature didn't appear in the Metadata Viewer. If you're using the metadata viewer and you can't add tags, log out from MB on your desktop browser and log in again. Then restart the metadata viewer.

@MonkeyDo
Copy link
Contributor Author

@mayhem i think perhaps @UltimateRiff 's suggestion is that we could also tag the MB Work entity as well as the current entities (Recording, Release/RG, Artist).

To answer that, I think it's a good idea, but I'll need to avaluate how to do that clearly. I already want to change the UI of this component to remove the accordion stuff so we can see all the information at once. The only thing the accordions bring is that it is clear(er) which entity you are tagging, so adding a 4th type of entity to tag could become confusing.
But thank you for opening the ticket!
At the very least, when we have a page to show more info, stats and links for recordings (a recording page in short) that'll be a great place to tag associated works 👍

@UltimateRiff
Copy link

@mayhem I have actually been having that issue with the live version of Listening Now, but not the test version. I'll try that fix~ ...but indeed, I'm talking here about MusicBrainz works entities~

I could definitely see the accordion becoming complicated with works, especially since (unlike releases and recordings), there can be several works attached to the same recording

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.

4 participants