#metabrainz

bitmap: I don't think so. ruaok, can you give it one final look?

shivam-kapila: i see no follow button :(

k

if you lcick it, what is the url it goes to

also what is the urlfo the image

> We are also upping the required versions of both Perl and Node.js.

> if you lcick it, what is the url it goes to

its a POST request, and I think it can be executed in the browser only via some code

might be nice to actually mention which versions right in the sentence.

> also what is the urlfo the image

sorry which image CatQuest?

CatQuest: https://twitter.com/ListenBrainz/status/1370075...

This is what should be happening

sorri assumed that the "follow me" button was an image

ruaok: +1

bitmap: reosarevok: looks good. it could use some boilerplate guff at the bottom: "We'll post final details about this release just prior to the release and shortly after we complete the release, we'll post instructions on how you can update your own copy.".

that's a beefy list -- good luck!

!m MB team

You're doing good work, MB team!

thanks!

CatQuest: anything visible in browser logs?

reosarevok: made the changes ruaok mentioned

if they look ok feel free to hit publish

Mr_Monkey: there's a test missing for updateMode in FollowerFollwing Modal and a render test for SimilarUsers modal. Shall I add these?

I get 4pm sun directly in my eyes when sitting at my desk only during summer

I think I'm going to need a summer house and a winter house

rich alastairp

Yes please shivam-kapila !

bitmap: oh, yvanzo wanted zas to confirm that moving to 20.04 would be sensible

(for Perl 5.30)

reosarevok: it shouldn't be much an issue, as https://github.com/phusion/baseimage-docker is now based on 20.04

at worse, if something breaks, we'll have to fix it anyway

ok, I'll make a new branch for https://github.com/metabrainz/base-image

I deployed the similar users PR on https://test.listenbrainz.org/feed , it's looking nice !

Mr_Monkey: added the tests

there seem to be many tests build in the queue. can we stop some unneccessary ones?

alastairp, i have updated https://github.com/metabrainz/listenbrainz-serv... with the spotify reader fix.

I'm just looking at it now

_lucifer: I was just reading code. spotify_read_listens line 149:

time_to_sleep = e.headers.get('Retry-After', delay)

is that header value going to be a string or an int?

alastairp, should be an int. https://sentry.metabrainz.org/metabrainz/listen...

Hey, yvanzo and reosarevok I hope you folks are doing alright. My name is Saad. I am a Computer Science Junior at college. I am really interested in one of your GSoC 2021 projects of Pushing the URL relationship editor to the next level. I am an experienced JavaScript developer, specifically in React.js. I was looking for the repo to get better

understanding of the codebase but unfortunately, I couldn't find it. Can you please share the link so that I can get started? Thank you!

and welcome to the community!

Welcome msaaddev !

Mr_Monkey: Is 1320 ready from your side?

I was reviewing once more

But yeah, it all seems to be there and working, and the code looks clean (to me). I haven't looked at the python parts much.

I am also reviewing and the python bits looks sane

With that PR we're adding endpoints to the API. Is there anything we need to do regarding that? Are the docs all there?

The api endpoints lack docstrings

Could you tackle that too please?

ok will do

_lucifer: that still looks suspicious to me. https://github.com/plamere/spotipy/blob/master/... shows that it directly returns the `requests` Response.headers dict. I don't think that requests does anything magic with numbers

in fact, I just did a quick test and got this response:

{'Date': 'Wed, 17 Mar 2021 16:18:22 GMT', 'Content-Type': 'application/json; charset=utf-8', 'Content-Length': '16', 'Connection': 'keep-alive', 'X-Powered-By': 'Express', 'Access-Control-Allow-Origin': '*', 'x-pd-status': 'sent to primary'}

content-length is a string, even though the underlying data is an int

but you're right, I see no TypeErrors in sentry that might be caused by this

makes me think that we're never getting that field :/

alastairp, that sounds right. i went through the sentry stacktrace i posted, there is no 429 there only 500s

I'll open a ticket fo rit

the 429 is from spotipy not actually spotify

so this exception doesn't set headers

but the one that I linked does

yes right and further its not a rate limit actually but too many 500s

but I think that spotify does return 429 as well, this is just a special-case that the bindings do after it's run out of retries

yes, its not well documented but there are some posts on spotify community about rate limiting

LB-842

LB-842: Parse Retry-After header as int before using in sleep https://tickets.metabrainz.org/browse/LB-842

should i just include the fix in the current PR?

bitmap: looks good, published

_lucifer: yeah, we're working on it anyway so may as well

👍

Mr_Monkey: hai!

just asking a question about https://github.com/metabrainz/listenbrainz-serv...

Hai !

why did this need to move out of the if?

If the owner is the only item in the collaborators list, it will be removed by that bit of code, and as a consequence we pass an empty list to `get_many_users_by_mb_id`, and it doesn't like that (SQL exception about `IN ()`)

ahhh, that's https://sentry.metabrainz.org/metabrainz/listen... which I had open to look at!

great fix 🎉

correct

and thanks :)

and I see that you did mention this in the PR body too

I haven't used sentry much; am I supposed to resolve those if they are fixed?

yes, that'd be good

because then it'll alert us again if it happens again

Yeah, it's obvious now that I phrase it :p

I don't use sentry very well either

assuming spotify follows the Oauth spec for invalid grant, `invalid_grant` can also occur if our credentials or redirect uri is incorrect.

>The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.

alastairp, should we add a check for this?

in which place? while a user is doing the auth workflow?

in `refresh_user_token`

in case `refresh_user_token` gets an `invalid_grant` it will unlink the user.

"does not match redirect uri" seems a bit weird

does that mean that if we changed the redirect url in our app, and then tried to refresh everyone, it'd fail?

that's from the OAuth Spec

I just had a thought here too - we delete the row if the refresh fails

but doesn't the row include the error message?

yes, i think so and unlink all the users along the way

i din't understand which row? the row for that user in spotify auth table?

sorry, yeah. spotify_auth

spotify_auth.error_message

yeah, right. that would be an issue.

how about instead of deleting we update the error message or add a column to the table and check for a specific message or that field before processing a user?

yeah, I'd be happy to have an active field

[troi-recommendation-playground] mayhem merged pull request #34 (main…improved-mbid-mapping): Improved mbid mapping, playlist submit improvements and year lookup improvements. https://github.com/metabrainz/troi-recommendati...

makes sense.

but if this PR allows us to update the spotify reader container then let's merge it and open a ticket for this and add it next

it will begin to error when we try to update the error message i think because the user is not present in the db

ah, right

yeah, let's use exceptions to indicate that case then, and do all database work in process_one_user

ah actually it won't since we already a different exception

but yeah moving the logic to process_one_user is probably better to keep it one place and avoid confusion

this one points to my branch

ah, now your question makes more senses

sense

so you're asking "should we catch expction; log; raise in process_one_user" or "just call refresh_user_token and catch/log in process_all_users"

reosarevok: yvanzo: I'm wondering how we should deal with keeping these new materialized tables up-to-date on slave servers. they aren't replicated and slaves don't have triggers, we we'd either have to add a file for slave-only triggers, or regenerate the tables hourly after packets are applied

alastairp, yes

the only thing I'd consider here is where we want to modify the database row. it doesn't make sense to modify a single user in process_all_users when there's an auth issue, and in process_one_user in all other cases

keeping it together in a single method would be easier I think

having said that - I'm thinking through a use-case here: token hasn't expired, but when we try and get currently playing we get an exception because the user has revoked permission

in that case it sounds like we should wrap all of process_one_user in a try/except?

yeah, it kinda already is in process_all_users and that's the only place we call it

oh, you're right. line 330-339?