but on the upside, i'd expect a lot of this complexity to go away after account migration is done.
2021-05-11 13137, 2021
alastairp
I would concentrate on making get_user() just get the user information
2021-05-11 13143, 2021
alastairp
and have a post-get-user method which does all of the updates - set last_login, set counts if necessary, update or set email address [not sure if we have the right information here to set error messages as necessary or if it has to be done in get_user]
2021-05-11 13130, 2021
_lucifer
makes sense. thanks!
2021-05-11 13125, 2021
alastairp
_lucifer: we're about ready for a release, no?
2021-05-11 13115, 2021
_lucifer
alastairp, yes.
2021-05-11 13122, 2021
_lucifer
how should we go about it? we have node ready to release. youtube changes are ready as well.
2021-05-11 13114, 2021
_lucifer
i was thinking if we there are some other small PRs ready then we should merge those and release with node. after that do another release with youtube changes.
2021-05-11 13131, 2021
alastairp
yeah, we have stuff merged which is not released, so let's just release it
2021-05-11 13151, 2021
alastairp
do we need to re-populate the spotify tables?
2021-05-11 13115, 2021
_lucifer
when we deploy the youtube changes, yes.
2021-05-11 13151, 2021
alastairp
ah, only for youtube? great
2021-05-11 13100, 2021
alastairp
let's get the node upgrade out of the way then
2021-05-11 13104, 2021
_lucifer
👍
2021-05-11 13149, 2021
_lucifer
the youtube changes and spotify changes are in the same PR, so they'll go out together anyways.
we also want to add more importers like for deezer and apple music, so we should keep these in mind at that time.
2021-05-11 13126, 2021
_lucifer
yes, 1361.
2021-05-11 13142, 2021
alastairp
they should definitely be separate importers, we can talk about it at the time
2021-05-11 13106, 2021
alastairp
what's the process for 1361? We need to update the db tables? make sure that config is in consul,
2021-05-11 13125, 2021
alastairp
did you sort out the auth process for youtube? Is there still a request that we need to make there?
2021-05-11 13128, 2021
_lucifer
the config is in consul, beta is currently running 1361.
2021-05-11 13140, 2021
alastairp
let me have 1 more look through it
2021-05-11 13153, 2021
zas
ruaok: I'll look into the ftp.eu cert issue
2021-05-11 13106, 2021
_lucifer
we still need to get the youtube process verified but that needs to happen after this is in produciton.
2021-05-11 13115, 2021
ruaok
k, thanks.
2021-05-11 13137, 2021
alastairp
and so what happens while it's not verified? the login button is there but people can't use it?
2021-05-11 13151, 2021
alastairp
do we know if this verification process is automatic or manual? any idea on the time required?
2021-05-11 13156, 2021
_lucifer
the people can use it but it'll show a warning to users that the appkication is not verified.
2021-05-11 13105, 2021
alastairp
ah, great.
2021-05-11 13135, 2021
_lucifer
its manual. we'll need to submit a video recording of the workflow as well. i don't know how long it could take.
2021-05-11 13102, 2021
alastairp
ok, but as long as it still works while that verification process happens (with the warning), that's fine
2021-05-11 13129, 2021
alastairp
do we want to add a little "(?) We're in the process of verification our application with Youtube" message above the button?
2021-05-11 13139, 2021
Mr_Monkey
Glad to hear you can replace your central heating with open source, alastairp ! (Do you know which query was retrying to infinity and beyond?)
2021-05-11 13155, 2021
_lucifer
the google oauth screen will show this info so i think its fine either way.
2021-05-11 13111, 2021
alastairp
Mr_Monkey: I think it was refresh token?
2021-05-11 13128, 2021
alastairp
_lucifer: right, but the question was more to tell users why they'll see it on the google screen
2021-05-11 13131, 2021
Mr_Monkey
For which service? Or for LB?
2021-05-11 13146, 2021
alastairp
it was hitting the LB API
2021-05-11 13151, 2021
Mr_Monkey
OK, thanks
2021-05-11 13157, 2021
alastairp
i.e. localhost/1/something
2021-05-11 13130, 2021
alastairp
you can reproduce it by bringing up LB with JS API pointing to localhost, loading a page, shutting down the server, then trying to do something on the frontend
2021-05-11 13125, 2021
_lucifer
sure, we can add a message there. something like "Our Youtube OAuth integration is currently under verification. Till then, you may see a warning screen while trying to enable it." ?
2021-05-11 13133, 2021
alastairp
yeah, that's what I mean
2021-05-11 13102, 2021
shivam-kapila
While you are adding the message you might as well want to add "......by revoking your Spotify or *Google account* access" in the last para on the same page.
2021-05-11 13123, 2021
shivam-kapila
The spotify line is already there. Might wanna add google to the list
2021-05-11 13131, 2021
shivam-kapila
_lucifer: ^
2021-05-11 13150, 2021
_lucifer
that paragraph only concerns spotify. should we just move that to inside the spotify panel or change the heading to A note about Spotify permissions ?
2021-05-11 13159, 2021
_lucifer
alastairp, Mr_Monkey: thoughts?
2021-05-11 13136, 2021
shivam-kapila
There's nothing like revoking access for YT accounts?
2021-05-11 13145, 2021
alastairp
yeah, I noticed that last night. Can we move it to below the spotify chooser?
2021-05-11 13100, 2021
_lucifer
yup, will do that.
2021-05-11 13149, 2021
_lucifer
there is but i think the aim is of that paragraph is to tell about some extra permissions that we don't use but spotify needs us to ask.
2021-05-11 13142, 2021
alastairp
yes, that's right
2021-05-11 13102, 2021
alastairp
> You can revoke these permissions whenever you want by unlinking your Youtube account.
2021-05-11 13115, 2021
Mr_Monkey
"You can revoke these permissions whenever you want by unlinking your account" should be for all services, maybe at the top?
2021-05-11 13121, 2021
alastairp
we use "disable" on the button. should be consistent here.
2021-05-11 13126, 2021
alastairp
and also what Mr_Monkey said
2021-05-11 13145, 2021
alastairp
oh, I see that the spotify one is in the lower paragraph
2021-05-11 13147, 2021
Mr_Monkey
And maybe I can try rephrasing the spotify-specific paragraph to whittle it down a bit, and then it goes in the spotify bloc
2021-05-11 13153, 2021
alastairp
agreed
2021-05-11 13149, 2021
shivam-kapila
> "You can revoke these permissions whenever you want by unlinking your account" should be for all services, maybe at the top?
2021-05-11 13149, 2021
shivam-kapila
Exactly what I tried to point out actually 😅
2021-05-11 13110, 2021
alastairp
oh. although, here's something that I just realised
2021-05-11 13138, 2021
alastairp
> Oddly enough, Spotify also requires the permission to read your email address [..] These permissions are required to determine if you are a premium user [..] ListenBrainz will never read these pieces of data.
2021-05-11 13158, 2021
alastairp
there's an email address here... we could have used this to notify users when integration fails ;-)
2021-05-11 13155, 2021
_lucifer
we can do that but then do we want to use that email for other purposes as well?
2021-05-11 13137, 2021
alastairp
no, we shouldn't
2021-05-11 13103, 2021
alastairp
yeah - it's not a huge issue - the process that we're doing now to require that users have an email in MB is a much better way of doing it
2021-05-11 13117, 2021
_lucifer
yeah, in that case. it probably makes sense to just use the email we get from MB everywhere
2021-05-11 13137, 2021
alastairp
but I found it funny to see that we had access to a verified email for spotify notifications from the beginning but we just decided to not use it
