#metabrainz

Morning!

ruaok: lots of 500's on artist msid-mbid lookup...

any reason why?

ishaanshah: the api is not working for any user

also morning

ishaanshah:

```psycopg2.errors.UndefinedTable: relation "mapping.msid_mbid_mapping" does not exist

LINE 12: ... FROM mapping.ms...

^

```

[musicbrainz-server] reosarevok opened pull request #1710 (master…MBS-11121): MBS-11121: Normalize Apple Music beta links https://github.com/metabrainz/musicbrainz-serve...

MBS-11121: Update iTunes/Apple Music URL cleanup https://tickets.metabrainz.org/browse/MBS-11121

[musicbrainz-server] reosarevok opened pull request #1711 (master…MBS-8650): MBS-8650: Lower how long an IPI must be to 5 https://github.com/metabrainz/musicbrainz-serve...

MBS-8650: Short IPIs are considered invalid https://tickets.metabrainz.org/browse/MBS-8650

Heh, just realized our reused-emails issue is also a problem for the otherwise trivial MBS-122

MBS-122: Combine forgot username and forgot password pages/functions https://tickets.metabrainz.org/browse/MBS-122

bitmap: wanna port your script? :p https://tickets.metabrainz.org/browse/MBS-9563

MBS-9563: Implement expand-collapse-release-groups.user.js as server code

_lucifer: you weren't on the approved list of committers for jenkins, which I think is why the jobs weren't running. I've added you

alastairp: ah! that makes sense. thanks!

bitmap, yvanzo: MBS-1575 - wontfix to not break the URLs, or change because these are not destination URLs people link to anyway?

MBS-1575: Terminology: delete vs. remove release https://tickets.metabrainz.org/browse/MBS-1575

(everything else has been changed, except the url is still /delete)

ishaanshah: ooops, sorry for that. we were trying to debug a filespace issue on paco yesterday and the mapping didn't regenerate correctly. I'll have it fixed in an hour.

outsidecontext: are you maintaining libdiscid now? Wondering if the bug mentioned in MBS-1588 still exists at all

MBS-1588: More user friendly page when trying to add a CD with a bad TOC https://tickets.metabrainz.org/browse/MBS-1588

(not that nicer errors are a bad thing)

reosarevok: I don't know if I do maintain it :D I did not intent to actually, but applied some changes recently so probably that means I volunteered

haha

I just asked because it's all you in recent history

about the issue not sure, my gut feeling would be that the underlying issue is probably solved. that bug report is rather old, and there had been quite a few changes afterwards in around 2013. but hard to tell without knowing the actual disc and how the TOC was bad

Ok

Well, improving the error page won't hurt either way

But good to know the whole thing has been improved in the meantime anyway

alastairp: hey, wanna necro your 6 year old code? https://bitbucket.org/chirlu/musicbrainz-server...

wow, that's a blast from the past

reosarevok: can be changed if needed, it will probably break URLs for that ticket only.

reosarevok: I really can't tell for sure, the info is very sparse there. The issue on that TOC is really that the total sectors are smaller then the last offset. but to see how this came to be one would need to check an actual disc producing something like this. can also be OS dependent, a glitch in reading the disc or maybe even have been generated with something other than libdiscid.

anyway, I guess improving the error page for wrongly submitted TOCs wouldn't hurt and is basically independent of this.

explaining that the submitted TOC has some invalid format and maybe link to discid documentation would help

tested libdiscid, at least it does not accept the offsets if you feed it the TOC directly

alastairp: two queries regarding the PRs

> My preference is for these error messages to not have a . at the end of them

so only "You have to select a license" right?

second, i am not sure how to add a test case for the cache issue ?

reosarevok: off the top of your head do you know if flash messages in MB end with a . ?

hah, and here is me saying ! at the end of a message is a bit naff and we shouldn't use them

_lucifer: does CB have any tests where we mock the cache? I think I have a few of that kind in AB

we could have a test where we test the specific get_x_by_id, mocking the cache and the BU get method, and just put in each of the types of necessary data

e.g., mock cache.get returns None, verify that the db get method is called. mock cache.get returns something, verify that db get isn't called

also, verify that the argument to mock cache.get is expected (e.g. 'artist_[artistid]')

alastairp: yes, i was able to find one in `test_statistics` where cache is mocked to return `None`

perfect

so, 2 tests, in one mock cache and in other mock db, right?

alastairp: I don't know if all do, but imo they should

! is nice in moderation

tfw you spend 2 weeks on a design doc for a 4 week project

sure, but by spending 2 weeks on it you make sure that it doesn't blow out to 10 weeks

I bet that is going to be worth it.

true

_lucifer: I think it might be OK. Do those methods have any tests?

getting people to agree on what needs doing is harder than just doing the thing :D

reosarevok: that'd be a nice thing to put into a style-guide. I'm trying to decide what the best one for us is

iliekcomputers: the story of my last 20 years!

we also use these messages to say things like "field x is required"

and I'm not sure I like . at the end of those, but I can be convinced

yes, ! sometimes is good. That example that I pasted probably iskn't too bad

deff

alastairp: i didn't understand you completely. which methods are you referring to?

critiquebrainz/frontend/external/musicbrainz_db/artist.py get_artist_by_id

alastairp: no that method has no test

OK, so let's add tests for all of these get_x_by_id methods, with the cache mocked as we discussed

is there a sample mb database available in the tests?

yeah, there is a test_db argument used in init_db so must be

`postgresql://critiquebrainz:critiquebrainz@db_test:5432/critiquebrainz`

it points to this for the tests

but that's the test critiquebrainz database, right?

I mean this one: https://github.com/metabrainz/critiquebrainz/bl...

(that's a yes..., I was just checking so that we can do a test that actually loads this data from the musicbrainz database)

yeah, this db will be empty or it has some sample data we can use right away?

that's what I'm asking you

I don't know

oh ok! i'll check that

it's empty alastairp

_lucifer: right. I don't know if the tests do anything to load dummy data into the db or not. Perhaps all we should do is mock the call to the brainzutils method too?

I don't like this much, because we're basically mocking everything, but I think the actual method is tested in BU anyway, so perhaps it's not strictly necessary

yeah, i agree. the get_x_by_id method is just choosing between the cache and db

so we can just check which one is it calling

and the argument is right or not ?

yes

[listenbrainz-server] mayhem opened pull request #1107 (master…msb-mapping-improvements): Cleanup from getting the mapping into production https://github.com/metabrainz/listenbrainz-serv...

ruaok: "&" is a join phrase in MB?

it can be

Hmm, so should not be removed while removing punctuation.