#metabrainz

[listenbrainz-server] 14amCap1712 opened pull request #2141 (03master…fix-api-test): Fix test_get_listens_ts_unavailable https://github.com/metabrainz/listenbrainz-serv...

alastairp: ^, this is the issue for the test failures i mentioned yesterday.

(on a side note, sleeping on a problem after spending hours to try fix it works really well. was able to fix in 5mins today lol)

[listenbrainz-server] 14amCap1712 opened pull request #2142 (03fix-api-test…fix-duration-chek): Improve handling of duration/duration_ms fields in listen submission json https://github.com/metabrainz/listenbrainz-serv...

lucifer: oh yeah, sleeping is amazing for that!

indeed!

[listenbrainz-server] 14amCap1712 merged pull request #2141 (03master…fix-api-test): Fix test_get_listens_ts_unavailable https://github.com/metabrainz/listenbrainz-serv...

lucifer: "while this is behaviour is discouraged and undocumented, we need this to work around LFM API quirks."

ever actually fixed a bug in your sleep?

this is a bit difficult to understand to me

do you mean that "passing a number as a string" is the lfm behaviour, and our documented format is an actual number?

i meant that the docs say we accept only integer, but the current validation also allows strings if those are convertible to integers.

that's a lot easier to understand than the existing comment!

ah! i'll update

unrelated but since we have merged DELETE FROM instead of DROP TABLE, no ResourceClosed errors. :D

yeah, that doesn't surprise me either

alastairp: fine to merge LB#2142?

Improve handling of duration/duration_ms fields in listen submission json: https://github.com/metabrainz/listenbrainz-serv...

lucifer: yeah, looks fine

lucifer: mm, any thought about grabbing the listen before validating it in the lfm api?

I mean - incoming compat listen -> convert duration immediately to number -> validate as normal

and then validator requires durations to be a number

alastairp: yes, thats the alternative but i chose the current impl because that's what we do for listened_at/timestamp field as well. but i guess we could also change both.

lucifer: yeah, fine to leave as-is for now, maybe a ticket if you think that we could improve it in the future?

sure sound good

[listenbrainz-server] 14amCap1712 merged pull request #2142 (03master…fix-duration-chek): Improve handling of duration/duration_ms fields in listen submission json https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] 14amCap1712 closed pull request #2094 (03master…tests-speed-up): [Do not merge] Speed up LB tests https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] 14amCap1712 merged pull request #2103 (03master…msb-move): Simplify MsB and migrate data to TS db https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] 14amCap1712 opened pull request #2143 (03master…improve-api-compat-tests): Use wait_for_query_to_have_items in api_compat tests https://github.com/metabrainz/listenbrainz-serv...

alastairp: monkey: mayhem: any PRs to merge? will do a release

other than the hundred that you just merged, no :)

lucifer: maybe that wordpress one?

you said you wanted to test it on beta

lucifer: nothing for me

ah right, let me do that now

Nothing ready for me either

alastairp: one issue discovered when testing on beta. added comment

thx

i'll wait for this PR before doing a release.

good catch lucifer, I guess because I have my api url on local dev to same host as the webserver

yup right in local dev, api and website are served from same host/url.

monkey: on the subject of blog urls on LB, one title appears as `GSoC’22: CritiqueBrainz reviews for BookBrainz entities`

something probably needs to be decoded in the api response.

lucifer: force pushed this change

lucifer: oops, do we need to fix that too, then?

alastairp: separate PR is fine for that i think. it already exists in the master

it's unusual, theoreitcally we didn't do anything different compared to getting this data from wordpress

yeah, that

ok, perfect. thanks

looking at this, we probably need to render the field as html to fix the issue.

yep

it also returns the content in the response which seems wasteful but now that we are caching on the backend, not much of an issue.

the content of the blog?

yes

yeah, we could make this smaller

in fact, we could also render the cache data directly on the flask template too... 🤔

indeed.

but for later improvements anyway

yep

and it may also interact with the SPA/SSR work so probably best to change at that time

yep, true. so keeping it in react helps with that in the future anyway

updated beta. works fine now. thanks!

[listenbrainz-server] 14amCap1712 merged pull request #2132 (03master…blog-local-cache): Cache wordpress API locally in LB and serve to clients from the cache https://github.com/metabrainz/listenbrainz-serv...

[listenbrainz-server] release 03v-2022-09-13.0 has been published by 14github-actions[bot]: https://github.com/metabrainz/listenbrainz-serv...

What is SPA/SSR stand for?

single page application/server side rendering

i have stopped ts writer for now.

checking msb db was correctly transferred, will bring up on new image after checks

single page application? where can I find out more about this plan?

monkey probably has more details on aims and the plan but iirc one of the aims was that BP continues playing when switching pages on LB.

here are many UX dragons down that road, but I understand the motivation.

Right, so currently navigating from one part of the website to another = browser navigation = BP playback stops, which we want to avoid. We also have other issues with having to create special modals for some features (think CB reviews, the new personalized recommend feature, etc.) which we have to create on each of our entry point pages, which makes the code duplicated and hard to maintain

And also means that we don't have all the features available on all the parts of the website. Ideally we want to have the same menu options and features in any listencard anywhere on the website

And you guessed it, the way to deal with all this at once is to have a single page app with routing managed in react

Which is pretty much standard for React front-end websites. We just have an odd combination currently with the python templates serving a single separate react page.

The main UX dragon is preserving normal browser navigation behavior, which these days is just handled perfectly by the routing package.

The other somewhat related project is server-side rendering, where the server does the initial rendering of the react page into HTML that is sent to the client, rather than sending a mostly empty page and only then rendering the react bits. Pages load a lot faster for users visually, you can use the website even if you have javascript disabled, and it's also good for SEO (which is probably moot in our case)

MB already does the SSR part

So we're likely to copy how they do it

LB-768

LB-768: Merge unit and integration tests back into a single docker-compose configuration https://tickets.metabrainz.org/browse/LB-768

ok, BrainzBot is back it seems

alastairp: mayhem: the script seems to have worked fine to me except it duplicates the last transferred row which caused unique index building on msid to fail. i removed the 3-4 offending rows manually. do you want to check/verify before i bring up ts writer again?

did the index build?

yes the unique index built fine now

that sounds like good validation for me

agreed

fwiw, i checked the last row in old db and new db. compared counts to verify other than this. and everything seems sane

hmm while building the other index, i get

let me have a look at the PR again

the offending row....

[listenbrainz-server] 14MonkeyDo opened pull request #2144 (03master…monkey-lint-dev-no-error): Don't fail webpack build for style issues in development https://github.com/metabrainz/listenbrainz-serv...

I wasn't aware of this limitation in postgres

alastairp: yes, at least 1 row.

what index did we have in msb previously? was it over less columns?

yeah me neither.

or was it because we did an index over the hash instead of the data?

yes it looks we only indexed hash not the actual text

mm, right

actaully we do have https://github.com/metabrainz/listenbrainz-serv...

but maybe its first column so it worked?

right, but is the issue because the combination of these 5 columns are too long?

(lower(recording), lower(artist_credit), lower(release), lower(track_number), duration)

yes right

there's no reason we couldn't have separate indices here fwiw.

yeah, I was just going to ask what this index was used for