#metabrainz

/

      • thomasross has quit
      • BrainzGit
        [musicbrainz-server] mwiencek merged pull request #2005 (schema-change-2021-q2…MBS-11451): MBS-11451: Support ratings for places https://github.com/metabrainz/musicbrainz-serve...
      • Major_Lurker joined the channel
      • MajorLurker has quit
      • BrainzGit has quit
      • bitmap
        yvanzo: can you create a schema change branch at https://github.com/yvanzo/mbdata for removing ordering_attribute?
      • I see that's why https://github.com/metabrainz/musicbrainz-serve... is currently failing
      • sir points to your repo atm
      • yvanzo
        bitmap: yes, I plan also to make PRs to the main mbdata repo :)
      • bitmap
        great, thx
      • _lucifer
        Mr_Monkey: i fixed the font size issue by removing `font-size: 0.75em` from `.details`.
      • updated beta again.
      • alastairp: https://github.com/metabrainz/listenbrainz-serv... , i think this block has become a bit convoluted. do you have any suggestions to simplify this?
      • one way i thought of was to set a default email if the mb database is not available.
      • but not sure if that's the right way to approach this.
      • alastairp
        we could break out "get" and "create" into 2 different methods
      • what do you mean if the mb database isn't available? During local development, or if it disappears during prod?
      • _lucifer
        during local development
      • alastairp
        what are our options? set it to some dummy value, or have an LB-level config option which allows this to be set to null without an error
      • _lucifer
        we can set email to None but that would lead to warnings and in the future issues with submitting listens.
      • yeah, disabling that option during development would work.
      • alastairp
        for example, can we disable the listen submit flag during development?
      • right
      • I wonder if there's some place here that we could move some of that code out
      • especially see how we already have a call to `update_last_login`
      • _lucifer
      • alastairp
        ah!
      • _lucifer
        yeah :(
      • that's not a less of mess either.
      • but on the upside, i'd expect a lot of this complexity to go away after account migration is done.
      • alastairp
        I would concentrate on making get_user() just get the user information
      • 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]
      • _lucifer
        makes sense. thanks!
      • alastairp
        _lucifer: we're about ready for a release, no?
      • _lucifer
        alastairp, yes.
      • how should we go about it? we have node ready to release. youtube changes are ready as well.
      • 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.
      • alastairp
        yeah, we have stuff merged which is not released, so let's just release it
      • do we need to re-populate the spotify tables?
      • _lucifer
        when we deploy the youtube changes, yes.
      • alastairp
        ah, only for youtube? great
      • let's get the node upgrade out of the way then
      • _lucifer
        👍
      • the youtube changes and spotify changes are in the same PR, so they'll go out together anyways.
      • alastairp
        right, that's what I wasn't sure about
      • tagging
      • _lucifer
        no build triggered :(
      • alastairp
        yeah
      • I'm just looking into it again
      • maybe we should use the `release` event: https://docs.github.com/en/actions/reference/ev...
      • _lucifer
        sure, let's try that.
      • alastairp
        is it because I pushed _only_ a tag? not a new commit with a tag associated with it?
      • _lucifer
        i'll enable https://docs.github.com/en/actions/managing-wor... also. maybe it helps/
      • alastairp
        because it's a lightweight tag, so it's attached to a commit that was already pushed yesterday
      • _lucifer
        i don't think that should matter. i have pushed many tags without commits before and it *usually* works.
      • ruaok
        moooin!
      • alastairp & _lucifer : something vaguely interesting about people's listening habits:
      • `96725 listens: exact 23480 high 254 med 1105 low 1444 no 11962 prev 58480 err 0 87.6%`
      • of the 96k listens processed overnight, 58k were previously identified by the mapper.
      • more than half.
      • and it hasn't been running all that long. it seems as though the "on current rotation" space of music isn't all that big. at least not yet.
      • _lucifer
        yeah. personally, most of my listens are from something i have listened previously. looks like that the case for many of LB users.
      • ruaok
        yes, this should't be surprising to us since one of our goals is to expose people to more music, right?
      • _lucifer
        plus, that means many users are listening to the same pool of recordings.
      • right.
      • ruaok
        I'm just pleased that this process seems to be working ok.
      • I suspect it won't take long to process 2 years worth of listens -- which is about as far back as we ever go for recommendation stuff. stats further.
      • alastairp
        _lucifer: building manually
      • _lucifer
        👍
      • brainzgit seems to be down.
      • shivam-kapila
        ruaok: this means the newly imported listens should have mbids(if they have matches)?
      • ruaok
        hopefully seconds after being submitted, yes.
      • alastairp
        doing new LB deploy now
      • back up. js compiled with node 16 seems to be working fine
      • _lucifer: spotify reader, spark reader, and websockets currently running an image from 2021-04-22, ok to upgrade?
      • _lucifer
        alastairp, yes. but we are going to do another release, so may as well update those just once.
      • ruaok
        Mr_Monkey: _lucifer: do we use babel? https://babeljs.io/blog/2021/05/10/funding-upda...
      • _lucifer
        yes, we do.
      • ruaok
        right. will send $$$
      • _lucifer
        that would be nice :D. thanks!
      • ruaok
      • Mr_Monkey
        Thank you ! 🎉
      • ruaok
        LOL, open source collective has the same address as us. lol. open source people think alike.
      • sumedh joined the channel
      • alastairp
        _lucifer: I had some thoughts about spotify: LB-891 LB-892
      • BrainzBot
        LB-891: Check that spotify importer is keeping up with user listens https://tickets.metabrainz.org/browse/LB-891
      • LB-892: Move spotify now-playing checks out of listen importer https://tickets.metabrainz.org/browse/LB-892
      • alastairp
        Mr_Monkey: I managed to keep my house warm with heat from my laptop last night LB-890
      • BrainzBot
        LB-890: Frontend should have back-off time when retrying after failed queries https://tickets.metabrainz.org/browse/LB-890
      • _lucifer
        alastairp, thanks for the ticket, i'll take a look.
      • the current release seems to be working fine, should I release the youtube changes now?
      • alastairp
        _lucifer: just some thoughts for now, based on the stats that I found it's not urgent to fix them, but something to consider
      • _lucifer
        we also want to add more importers like for deezer and apple music, so we should keep these in mind at that time.
      • yes, 1361.
      • alastairp
        they should definitely be separate importers, we can talk about it at the time
      • what's the process for 1361? We need to update the db tables? make sure that config is in consul,
      • did you sort out the auth process for youtube? Is there still a request that we need to make there?
      • _lucifer
        the config is in consul, beta is currently running 1361.
      • alastairp
        let me have 1 more look through it
      • zas
        ruaok: I'll look into the ftp.eu cert issue
      • _lucifer
        we still need to get the youtube process verified but that needs to happen after this is in produciton.
      • ruaok
        k, thanks.
      • alastairp
        and so what happens while it's not verified? the login button is there but people can't use it?
      • do we know if this verification process is automatic or manual? any idea on the time required?
      • _lucifer
        the people can use it but it'll show a warning to users that the appkication is not verified.
      • alastairp
        ah, great.
      • _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.
      • alastairp
        ok, but as long as it still works while that verification process happens (with the warning), that's fine
      • do we want to add a little "(?) We're in the process of verification our application with Youtube" message above the button?
      • 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?)
      • _lucifer
        the google oauth screen will show this info so i think its fine either way.
      • alastairp
        Mr_Monkey: I think it was refresh token?
      • _lucifer: right, but the question was more to tell users why they'll see it on the google screen
      • Mr_Monkey
        For which service? Or for LB?
      • alastairp
        it was hitting the LB API
      • Mr_Monkey
        OK, thanks
      • alastairp
        i.e. localhost/1/something
      • 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
      • _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." ?
      • alastairp
        yeah, that's what I mean
      • 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.
      • The spotify line is already there. Might wanna add google to the list
      • _lucifer: ^
      • _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 ?
      • alastairp, Mr_Monkey: thoughts?
      • shivam-kapila
        There's nothing like revoking access for YT accounts?
      • alastairp
        yeah, I noticed that last night. Can we move it to below the spotify chooser?
      • _lucifer
        yup, will do that.
      • 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.
      • alastairp
        yes, that's right
      • > You can revoke these permissions whenever you want by unlinking your Youtube account.
      • Mr_Monkey
        "You can revoke these permissions whenever you want by unlinking your account" should be for all services, maybe at the top?
      • alastairp
        we use "disable" on the button. should be consistent here.
      • and also what Mr_Monkey said
      • oh, I see that the spotify one is in the lower paragraph
      • 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
      • alastairp
        agreed
      • shivam-kapila
        > "You can revoke these permissions whenever you want by unlinking your account" should be for all services, maybe at the top?
      • Exactly what I tried to point out actually 😅
      • alastairp
        oh. although, here's something that I just realised
      • > 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.
      • there's an email address here... we could have used this to notify users when integration fails ;-)
      • _lucifer
        we can do that but then do we want to use that email for other purposes as well?
      • alastairp
        no, we shouldn't
      • 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
      • _lucifer
        yeah, in that case. it probably makes sense to just use the email we get from MB everywhere
      • 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