sorri assumed that the "follow me" button was an image
2021-03-17 07615, 2021
bitmap
ruaok: +1
2021-03-17 07630, 2021
ruaok
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.".
2021-03-17 07641, 2021
ruaok
that's a beefy list -- good luck!
2021-03-17 07643, 2021
ruaok
!m MB team
2021-03-17 07643, 2021
BrainzBot
You're doing good work, MB team!
2021-03-17 07608, 2021
bitmap
thanks!
2021-03-17 07659, 2021
shivam-kapila
CatQuest: anything visible in browser logs?
2021-03-17 07657, 2021
bitmap
reosarevok: made the changes ruaok mentioned
2021-03-17 07632, 2021
bitmap
if they look ok feel free to hit publish
2021-03-17 07622, 2021
shivam-kapila
Mr_Monkey: there's a test missing for updateMode in FollowerFollwing Modal and a render test for SimilarUsers modal. Shall I add these?
2021-03-17 07649, 2021
alastairp
I get 4pm sun directly in my eyes when sitting at my desk only during summer
2021-03-17 07659, 2021
alastairp
I think I'm going to need a summer house and a winter house
2021-03-17 07645, 2021
shivam-kapila
rich alastairp
2021-03-17 07640, 2021
Mr_Monkey
Yes please shivam-kapila !
2021-03-17 07638, 2021
HenryG has quit
2021-03-17 07611, 2021
akashgp09 joined the channel
2021-03-17 07618, 2021
HenryG joined the channel
2021-03-17 07650, 2021
reosarevok
bitmap: oh, yvanzo wanted zas to confirm that moving to 20.04 would be sensible
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
2021-03-17 07631, 2021
msaaddev
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!
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.
2021-03-17 07609, 2021
shivam-kapila
I am also reviewing and the python bits looks sane
2021-03-17 07600, 2021
Mr_Monkey
With that PR we're adding endpoints to the API. Is there anything we need to do regarding that? Are the docs all there?
2021-03-17 07648, 2021
shivam-kapila
The api endpoints lack docstrings
2021-03-17 07617, 2021
Mr_Monkey
Could you tackle that too please?
2021-03-17 07625, 2021
shivam-kapila
ok will do
2021-03-17 07642, 2021
alastairp
_lucifer: that still looks suspicious to me. https://github.com/plamere/spotipy/blob/master/sp… shows that it directly returns the `requests` Response.headers dict. I don't think that requests does anything magic with numbers
2021-03-17 07650, 2021
alastairp
in fact, I just did a quick test and got this response:
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 ()`)
and I see that you did mention this in the PR body too
2021-03-17 07643, 2021
Mr_Monkey
I haven't used sentry much; am I supposed to resolve those if they are fixed?
2021-03-17 07654, 2021
alastairp
yes, that'd be good
2021-03-17 07601, 2021
alastairp
because then it'll alert us again if it happens again
2021-03-17 07607, 2021
Mr_Monkey
Yeah, it's obvious now that I phrase it :p
2021-03-17 07623, 2021
alastairp
I don't use sentry very well either
2021-03-17 07601, 2021
RikkoM has quit
2021-03-17 07605, 2021
msaaddev has quit
2021-03-17 07642, 2021
_lucifer
assuming spotify follows the Oauth spec for invalid grant, `invalid_grant` can also occur if our credentials or redirect uri is incorrect.
2021-03-17 07612, 2021
_lucifer
>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.
2021-03-17 07605, 2021
_lucifer
alastairp, should we add a check for this?
2021-03-17 07628, 2021
alastairp
in which place? while a user is doing the auth workflow?
2021-03-17 07646, 2021
_lucifer
in `refresh_user_token`
2021-03-17 07616, 2021
_lucifer
in case `refresh_user_token` gets an `invalid_grant` it will unlink the user.
2021-03-17 07648, 2021
alastairp
"does not match redirect uri" seems a bit weird
2021-03-17 07610, 2021
alastairp
does that mean that if we changed the redirect url in our app, and then tried to refresh everyone, it'd fail?
2021-03-17 07622, 2021
_lucifer
that's from the OAuth Spec
2021-03-17 07634, 2021
alastairp
I just had a thought here too - we delete the row if the refresh fails
2021-03-17 07642, 2021
alastairp
but doesn't the row include the error message?
2021-03-17 07646, 2021
_lucifer
yes, i think so and unlink all the users along the way
2021-03-17 07619, 2021
SothoTalKer has quit
2021-03-17 07625, 2021
_lucifer
i din't understand which row? the row for that user in spotify auth table?
2021-03-17 07600, 2021
SothoTalKer joined the channel
2021-03-17 07654, 2021
alastairp
sorry, yeah. spotify_auth
2021-03-17 07613, 2021
alastairp
spotify_auth.error_message
2021-03-17 07649, 2021
_lucifer
yeah, right. that would be an issue.
2021-03-17 07607, 2021
dpmittal_ joined the channel
2021-03-17 07604, 2021
_lucifer
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?
2021-03-17 07628, 2021
alastairp
yeah, I'd be happy to have an active field
2021-03-17 07604, 2021
BrainzGit
[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-recommendation…
2021-03-17 07613, 2021
_lucifer
makes sense.
2021-03-17 07618, 2021
alastairp
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
2021-03-17 07605, 2021
_lucifer
it will begin to error when we try to update the error message i think because the user is not present in the db
2021-03-17 07622, 2021
akashgp09 has quit
2021-03-17 07634, 2021
alastairp
ah, right
2021-03-17 07605, 2021
alastairp
yeah, let's use exceptions to indicate that case then, and do all database work in process_one_user
2021-03-17 07604, 2021
_lucifer
ah actually it won't since we already a different exception
2021-03-17 07639, 2021
_lucifer
but yeah moving the logic to process_one_user is probably better to keep it one place and avoid confusion
2021-03-17 07659, 2021
RikkoM joined the channel
2021-03-17 07621, 2021
-- BotBot disconnected, possible missing messages --
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"
2021-03-17 07633, 2021
bitmap
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
2021-03-17 07602, 2021
_lucifer
alastairp, yes
2021-03-17 07648, 2021
alastairp
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
2021-03-17 07611, 2021
alastairp
keeping it together in a single method would be easier I think
2021-03-17 07600, 2021
goapunk_ is now known as goapunk
2021-03-17 07600, 2021
alastairp
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
2021-03-17 07632, 2021
alastairp
in that case it sounds like we should wrap all of process_one_user in a try/except?
2021-03-17 07619, 2021
_lucifer
yeah, it kinda already is in process_all_users and that's the only place we call it
