#metabrainz

Mr_Monkey: I had a look at PR 350. LGTM

Also https://test.bookbrainz.org/revisions revision #10489-#10492

Is this a bug?

or a feature? :p

This is a single work I revised multiple times.

https://github.com/bookbrainz/bookbrainz-site/p... This broke some stuff @Mr_Monkey

i think you should revert these changes, this PR didn't really solve a bug :P

or maybe move`router.get('work/create')` to the top and all the routes with `work/:bbid` to the bottom?

But that would mean having `create` and `edit` route at top and bottom and ideally they should be together like right now ( because they're similar)

what do you think?

and I think this is perfect time to add more tests for routes in https://github.com/bookbrainz/bookbrainz-site/b...

:P

prabal: Ah, indeed that's an issue. I'll have another look, there should be a right way to do this without breaking the other routes

As for revisions #10489-#10492, I don't see an issue at first glance. I see a single work modified multiple times then deleted…

Or do you mean they should all have the same name?

Thanks for reviewing !

lol "invalid BBID" whne no BBid

Yep…

I was under impressiong that that `makeEntityLoader

`makeEntityLoader` function wasn't called for create/edit routes (without the `:bbid` parameter). Investigating now

Hrumpf, in the route `work/create`, it considers `create` to be the bbid parameter…

Project bookbrainz-site build #2789: passed in 2 min 14 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...

Yeah

we can move those routes at the top?

work/create , work/create/handler

@ruaok hey, Apple has updated their license agreement. can you log into your Apple developer account and accept the new license agreement for MB? Picard notarization will not work otherwise :(

on it.

thanks

ruaok: hope you are all well. how is the situation in Barcelona?

improving. the city is quiet for the first time ever, from what I can tell.

I'm doing well, you?

yes, but that's currently a strange feeling with empty cities everywhere.

all good here so far. people around really take this serious and act accordingly and keep distance.

hmmm. two factor auth is not working. I think bitmap may need to do this step. bitmap ^^

but having the whole family at home all day keeps me busy :D

2FA on Apple is a PITA :( But worked for me this time. But only the account holder can accept the license agreement (which makes sense)

ah, finally got it.

any idea where to find the license agreement? nothing shown from the dashboard.

found it.

accepted. it would take hours to read that thing. fuckers.

ruaok: thanks, let me check if the notarization runs again

those license agreements are horrible. I guess they mostly count on people accepting without reading. I guess they also did not offer a summary of the changes

nor would we have a choice if we didn't want to accept.

right

picard notarization is working again :)

prabal: I modified `makeEntityLoader` to accept the route '/create' . I'm writing basic tests for the routes now.

As for '/create/handler' POST route, we shouldn't need to modify it as the `makeEntityLoader` middleware is only called for GET requests.

Yeah alright :)

Thanks for catching the issue, I totally missed it yesterday…

Yeah I missed it too. :( I think test-site should be completed. It has only 4-5 routes rn.

Writing other test for other routes won't be difficult i think...

prabal: Nice suggestion on test-site. I am working on the same

iliekcomputers: Hi, I have updated the docs, added linter to the test suite and made a pre-commit hook

Please have a look

The pre-commit hook is not installed by default

Right?

No

I added instructions to the docs

Right, thanks, that seems reasonable. I'll test today. :)

This disc ID is still in the system but doesn't show any releases: https://musicbrainz.org/cdtoc/3hVImbd8bdIZaqq5W... It was originally added to https://musicbrainz.org/release/0a2e0cfa-39b5-4... which later got turned into vinyl. Disc ID tab and WS don't show it either. I wonder what happens when https://musicbrainz.org/edit/68686180 goes through.

That release and disc ID also exists on test.mb.o although it had no history. After changing the format the disc ID is still orphaned

[listenbrainz-server] mayhem merged pull request #775 (timescale…timescale-tests): [wip] Timescale Listenstore tests https://github.com/metabrainz/listenbrainz-serv...

shivam-kapila: I've pushed fixes to the timescale branch -- all listenstore tests now pass.

how do we enable all the other tests to run again with test.sh ?

Do any of our Indians speak Bengali? :)

ruaok: It will be enabled by removing the specific test path in pytest.ini

Were the changes I made fine?

yes, thanks for debugging the whole from_timescale function. :)

reosarevok: Not me.

ruaok: My pleasure. I wanted to discuss about next test

Do you have around 5 minutes?

let me finish one task. gimme 5-10 minutes, please

Sure sure.

ok, full tests running now. let's see how that goes.

shall we chat about next steps?

there are plenty of other tests failing. I suppose those should be our first step.

ruaok: Some tests are left in listenstore too which I guess will be finished in some time

The follwing test fails

Reason being we insert the listened_at from 1 to eleven and for full dump we test against listened_at

well, I just removed that module. screw influx.

Lol 😂

ok, why dont you work on the remaining tests for the timescale listenstore and I'll see about fixing the dumpmanager tests?

ruaok: Sure but I need some help regarding the test I mentioned

not sure I understand what you are asking/need

In influx we tested against created rather than listened_at

for full dumps if I am not wrong

not sure that is right -- the full dumps don't all have created/inserted_at timestamps, but the incremental ones always will

so, full dumps should test against listened_at and incremental should test against created

I ran this test and saw that it fails bkz we have listened_at for test data as 1-11 but for check we pass timestamp value

So 1-11 is always less than the timestamp

btw while I have you LB folks here. i see LB added MBID importing from last.fm. Is that applied to existing imports? Will a fresh import enrich existing imported listens, or should I reset the "last import" date first?

But it should split at between. So I just wanted to ask that can I modify the way I generate test data for this test?

ruaok: ^^

ah, given that postgres can handle the proper seconds since epoch timestamps, rather than the full long painful influx timestamps, we should move all the tests to not use timestamps, but integer epochs.

yes, standardize on tests using integer timestamps, not string timestamps.

does that make sense shivam-kapila?

ruaok: Didnt quite get it sorry. :(

ok, influx has one format of timestamps, a longer one that is based on ascii, strings, right?

Yes

and the listened_at column in the listens table is an integer, right?

Yes

so, now we have 2 formats for timestamps, which is odd. we could adapt the tests to use integer or string, depending on the test.

OR we could change the created column to also be an integer column.

that means we we have more consistency in our tables.

iliekcomputers: any thoughts on this?

Currently what I modified yestereday is that I assumed we always get datetime type values for start and end times for dumps

For full dumps we use listened_at so I coverted it to int in listenstore and not in tests

For incremental we use created_at which is datetime so I pass the values as it is

I think it might be best for us to keep the schema as is, and then do the necessary conversion between the two formats as needed.

The current problem I faced is that I input 1-11 in listened_at. I need to form dump values for 1-5 so I need to split it at 5 but for check value I send timestamp value. As its full dump so the timestamp gets converted to integer in the listenstore and as its in millions so 1-11 are all less than thst this millions int. So all get into dump. Did you get it now?

what do 1-11 and 1-5 refer to?