#metabrainz

/

      • supersandro2000 has quit
      • supersandro2000 joined the channel
      • supersandro2000 has quit
      • supersandro2000 joined the channel
      • Chinmay3199 has quit
      • d4rkie joined the channel
      • D4RK-PH0_ joined the channel
      • d4rkie has quit
      • D4RK-PH0ENiX has quit
      • D4RK-PH0_ has quit
      • D4RK-PH0ENiX joined the channel
      • rohitdandamudi_ joined the channel
      • rohitdandamudi_ has quit
      • prabal joined the channel
      • prabal
        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.
      • Darkloke joined the channel
      • Mineo has quit
      • Mineo joined the channel
      • 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
      • Chinmay3199 joined the channel
      • v6lur joined the channel
      • Zastai joined the channel
      • madmouser1 joined the channel
      • reosarevok_ joined the channel
      • reosarevok has quit
      • reosarevok_ is now known as reosarevok
      • davic has quit
      • Mineo has quit
      • Mineo joined the channel
      • shivam-kapila joined the channel
      • davic joined the channel
      • Mr_Monkey
        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 !
      • ZaphodBeeblebrox
        lol "invalid BBID" whne no BBid
      • Mr_Monkey
        Yep…
      • I was under impressiong that that `makeEntityLoader
      • `makeEntityLoader` function wasn't called for create/edit routes (without the `:bbid` parameter). Investigating now
      • Gazooo has quit
      • Gazooo joined the channel
      • Hrumpf, in the route `work/create`, it considers `create` to be the bbid parameter…
      • travis-ci joined the channel
      • travis-ci
        Project bookbrainz-site build #2789: passed in 2 min 14 sec: https://travis-ci.org/bookbrainz/bookbrainz-sit...
      • travis-ci has left the channel
      • prabal
        Yeah
      • we can move those routes at the top?
      • work/create , work/create/handler
      • outsidecontext
        @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 :(
      • ruaok
        on it.
      • outsidecontext
        thanks
      • ruaok: hope you are all well. how is the situation in Barcelona?
      • ruaok
        improving. the city is quiet for the first time ever, from what I can tell.
      • I'm doing well, you?
      • outsidecontext
        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.
      • ruaok
        hmmm. two factor auth is not working. I think bitmap may need to do this step. bitmap ^^
      • outsidecontext
        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)
      • ruaok
        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.
      • outsidecontext
        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
      • ruaok
        nor would we have a choice if we didn't want to accept.
      • outsidecontext
        right
      • picard notarization is working again :)
      • Mr_Monkey
        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.
      • prabal
        Yeah alright :)
      • Mr_Monkey
        Thanks for catching the issue, I totally missed it yesterday…
      • prabal
        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...
      • kieto joined the channel
      • Chinmay3199
        prabal: Nice suggestion on test-site. I am working on the same
      • ishaanshah[m]
        iliekcomputers: Hi, I have updated the docs, added linter to the test suite and made a pre-commit hook
      • Please have a look
      • iliekcomputers
        The pre-commit hook is not installed by default
      • Right?
      • ishaanshah[m]
        No
      • I added instructions to the docs
      • iliekcomputers
        Right, thanks, that seems reasonable. I'll test today. :)
      • chaban
        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
      • BrainzGit
        [listenbrainz-server] mayhem merged pull request #775 (timescale…timescale-tests): [wip] Timescale Listenstore tests https://github.com/metabrainz/listenbrainz-serv...
      • ruaok
        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 ?
      • reosarevok
        Do any of our Indians speak Bengali? :)
      • D4RK-PH0ENiX has quit
      • shivam-kapila
        ruaok: It will be enabled by removing the specific test path in pytest.ini
      • Were the changes I made fine?
      • ruaok
        yes, thanks for debugging the whole from_timescale function. :)
      • shivam-kapila
        reosarevok: Not me.
      • ruaok: My pleasure. I wanted to discuss about next test
      • Do you have around 5 minutes?
      • ruaok
        let me finish one task. gimme 5-10 minutes, please
      • shivam-kapila
        Sure sure.
      • ruaok
        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.
      • shivam-kapila
        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
      • ruaok
        well, I just removed that module. screw influx.
      • shivam-kapila
        Lol 😂
      • ruaok
        ok, why dont you work on the remaining tests for the timescale listenstore and I'll see about fixing the dumpmanager tests?
      • shivam-kapila
        ruaok: Sure but I need some help regarding the test I mentioned
      • ruaok
        not sure I understand what you are asking/need
      • shivam-kapila
        In influx we tested against created rather than listened_at
      • for full dumps if I am not wrong
      • ruaok
        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
      • shivam-kapila
        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
      • D4RK-PH0ENiX joined the channel
      • So 1-11 is always less than the timestamp
      • Zastai
        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?
      • shivam-kapila
        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: ^^
      • 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?
      • shivam-kapila
        ruaok: Didnt quite get it sorry. :(
      • ruaok
        ok, influx has one format of timestamps, a longer one that is based on ascii, strings, right?
      • shivam-kapila
        Yes
      • ruaok
        and the listened_at column in the listens table is an integer, right?
      • shivam-kapila
        Yes
      • ruaok
        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?
      • shivam-kapila
        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
      • ruaok
        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.
      • shivam-kapila
        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?
      • ruaok
        what do 1-11 and 1-5 refer to?
      • ruaok is still lost