#metabrainz

Either that, or hide it, which I do'nt think is a great idea

or "Created for", or we could decide that "Created for you" is a noun, and it's implied that this means that it's the lists created for that specific user

I don't know if there's a lot of room for misinterpreting the tab. It does say the name of the user in that blurb

yeah, and the sub heading below does directly say what it is, anyway

if the user sees the page, we can say "created for you". But if another user sees it "Created for [monkey]".

mayhem: sure, easy to do. I wonder what our longest username is

We do that in a couple of places where there is more room for misinterpretation

So it would be consistent.

ok, great

[listenbrainz-server] 14alastair merged pull request #2158 (03master…follow-button-class): Rename follow-button class to prevent adblocking https://github.com/metabrainz/listenbrainz-serv...

But as suggested I don't think htat's a good idea in a tab

The tabs are already quite broken on smaller screens, I would hate to imagine what happens when they're longer

lucifer: some good things in that dumps review that we could probably roll back into regular dumps too

yes makes sense

aerozol: I was thinking of two things we could do together when we are in person:

1. I can show you how I use github desktop and other technical bits of our workflows that you would like to have an intro to

2. we can do a serious session of 4-hands designing/doodling for everything we can think of, from landing pages to LB user dashboard, etc.

1 is for your convenience, and 2 is for maybe preparing projects for you to work on for the next 6 months or so?

4 hands... any more and I'll start having Cthulhu nightmares

[listenbrainz-server] 14amCap1712 merged pull request #2154 (03recommendations-support-mbid…recommendations-support-mbid-fe): Enable recommend track button if only recording mbid is present https://github.com/metabrainz/listenbrainz-serv...

lucifer: hi, about sqlalchemy 2 warnings - specifically the one about _mapping or .mappings()

is there still an open PR for this?

hmm, that being said, I see calls to .mappings() in the playlist db method, weird. let me dig into this a bit more

anope. some of the warnings were introduced later or missed. i haven't opened another PR to fix these because warnings until those are turned to errors in tests, it'll happen again. we cannot currently turn warnings to errors because mbdata needs to be updated.

i do have the changes locally though probably. let me see.

👍 ok, I'll fix warnings as they appear in the files that I'm touching

sure, if you have any for lb/playlist.py send me a diff and I'll apply them

I've found the place that I need to fix, anyway

ah cool, thanks!

https://github.com/flask-debugtoolbar/flask-deb... this is another warning, we should keep an eye on a new release od this

[listenbrainz-server] 14MonkeyDo merged pull request #2147 (03master…add-data-foobar200): Add foobar2000 player and its plugin to compatible players list https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] 14MonkeyDo merged pull request #2156 (03master…monkey-listen-duration-seconds): LB-1126: show track duration properly on ListenCard https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] 14amCap1712 opened pull request #2159 (03master…improve-dumps): Remove redundant excepts from dump.py https://github.com/metabrainz/listenbrainz-serv...

finally!

one column done, other still to do

!m lucifer

You're doing good work, lucifer!

alastairp: we didn't reach a conclusion earlier. do we want to go the replication packets way?

lucifer: from what I understand then, using the last_updated will work if any metadata has changed, or if metadata in related items has changed (as we follow these relations)

but it won't work if other relations are added or removed?

it can work for added, will need to confirm but definitely not for removed.

and how long is a dump taking for you now?

to confirm - what is your artist_mbids test doing here? This is for when a new artist is added to a recording?

ran from 13:01:20 to 14:41:24

this is comparing the that all artist mbids in the old and new cache are same.

this is for checking we are not missing capturing any change.

so for artist mbids, for changes that happened in MB between the time you generated the 2 tables, we are capturing all changes?

regarding you actual question: if there are changes that we can't determine based on the last updated field, then we definitely need another way of getting it. I'm not sure if it's best to do that with a replication packet, or by looking at the edit table

are changes to the edit table included in replication packets? reosarevok? I guess they will only apply if you made a mirror that includes edits in the initial import?

alastairp: Looking at tickets now, turns out the rating is not optional for mini-reviews in LB ?!

monkey: hmmm

that sounds familar to me. not sure if I remember seeing the same ticket?

I'm wondering if we're sending the wrong thing in the API call to CB

We're not sending a rating at all if user didn't click one

alastairp: yes, i think we are capturing all changes but i'll do a few more tests to confirm.

from a very quick scan of the code, it appears that rating is optional in the API

there's a check to see that you must have at least one of text and rating, but nothing to say that rating is required

at least one of text *or* rating

I saw the same in docs, which is why I'm here asking you :p

monkey: sending rating as 0 ?

Nope, not sending any rating: https://usercontent.irccloud-cdn.com/file/LGunU...

Ahhaaa, but that is sent to https://api.listenbrainz.org/1/user/mr_monkey/t...

btw, another thing I noticed is that the text on this popup says "are licensed under a Creative Commons Attribution-ShareAlike 3.0 Unported or Creative Commons Attribution-ShareAlike-NonCommercial license as per your choice above"

but doesn't ask for a license choice

ahhh,

And indeed the LB backedn does set to 0: `rating=metadata.get("rating", 0)`

that error "Parameter too small" makes sense now, because I see that LB uses pydantic

and that's a pydantic validation erroro

`class CBReviewMetadata` doesn't indicate that review and rating are optional

I suspect it might be this. I can have a look at the validation now if you want

Yes please :) No need to do it right now, just wanted to understand where the issue was

Thanks for the brainstorming

monkey: do we require some text in the frontend?

Better text would be good to show an error

But I can do validation front-end side

I see from the modal that it always requires some text

so that's less of an issue

Yep

For API validation error I think the above should be acceptable

monkey: btw, I see the yellow stars offset from the grey ones

Ooh, floating stars

do you see that too?

Affirmative

due to `#CBReviewModal .rating-container .rating-stars>span` `top`

although that should apply to both child divs, not sure why it isn't

I'll investigate that one

There is an override of the `top` property adde to the element's `style` (so overruling any css definition)

but only for the gray one, and then the yellow ones aren't overridden also?

humm

monkey: does an LB user page make a lookup to api /1/metadata/lookup ?

yes, `APIService.lookupRecordingMetadata` in now playing popup

in development, if your api root is set to your local environment, then this endpoint requires a metadata lookup, which requires the mapping tables, and so you get quite a significant error message

Yes, for now playing

ah, beat me to ut

Eek

Beautiful.

Oh, full html error page it looks like

that API endpoint should have a bit more error handling

server-side or client-side?

ah - in this case yes, because I have debug mode on, and that shows a custom flask error interface

so I think it makes sense to have a way of indicating that in development this endpoint doesn't work, in any case we should probably capture and handle this exception anyway

right, I guess what's happening here is that the frontend is correctly handling a non-200 status code, but printing the response in a popup

perhaps some ... might be in order in general?

Yep, agreed

[listenbrainz-server] 14MonkeyDo opened pull request #2160 (03master…stars-align): When the stars align… https://github.com/metabrainz/listenbrainz-serv...

monkey: any chance you could make another sneaky change in that PR to remove the 2 license options from the disclaimer text?

Sure thing

and replace it with just the one license that we use in LB (cc by-sa)

alastairp: this one? https://creativecommons.org/licenses/by-sa/2.0/

yes, but version 3

Or https://creativecommons.org/licenses/by-sa/3.0/

Yep, OK

[musicbrainz-server] 14reosarevok opened pull request #2642 (03master…MBS-12593): MBS-12593: Convert cdtoc/attach_filter_release to React https://github.com/metabrainz/musicbrainz-serve...

bitmap: thanks, finally got it to work (hopefully properly) :)

lucifer: hi, quick question. I'm trying to run the mapping writer, and I have a kombu related error:

alastairp: plz check review text

alastairp: calling run instead of start probably.

lucifer: looks like somehow the `init_rabbitmq_connection` isn't getting called?

how does the existing mapper work then? 🤔

monkey: will do

alastairp: i fixed it in some branch. prod is not running master affair

lucifer: oh duh. .start does init and then calls self.run

right

Monkey: sounds great. I think we'll come up with heaps to do actually if we set aside some time! Let's do those though

Oh and thanks for the help with those tickets monkey, I'm visiting family for a few days and giving the arm a rest, I'll go through your instructions when I'm back. Genuinely looking forward to figuring out what all that stuff means!