The ctrl+ part I didn't have a fix for, but it does look pretty easy to fix
2021-08-30 24228, 2021
monkey
There's also those right/left arrow key navigation events (to go to prev/next page) that shouldn't fire if an input is currently focused.
2021-08-30 24232, 2021
alastairp
yeah, if we can do a cross-browser/OS test to check that it works as expected in all combinations then I think it'd be an easy fix and will properly restore expected functionality :)
2021-08-30 24206, 2021
monkey
You end up navigation right and left when writing text in the pin listen or CB review modals
2021-08-30 24224, 2021
alastairp
I didn't even know about those events. I wonder if there's a way that we can expose this info better. perhaps a `?` help overlay like on other sites (github)
2021-08-30 24241, 2021
alastairp
oh, that doesn't sound ideal. I guess it just captures the event globally on the page?
2021-08-30 24241, 2021
yvanzo
reosarevok: Spotted a bug in beta thanks to Sentry: MBS-11923
I expect chaban would have reported this to us otherwise
2021-08-30 24200, 2021
reosarevok
I'll tag them on the ticket and ask
2021-08-30 24234, 2021
lucifer
monkey: huh! weird. i have another hunch try disconnecting and reconnecting CB from test.lb
2021-08-30 24245, 2021
monkey
lucifer: Good hunch! Working for me now
2021-08-30 24256, 2021
monkey
Thanks for your help
2021-08-30 24205, 2021
lucifer
cool, i know the issue now.
2021-08-30 24216, 2021
lucifer
CORS headers are not applied to 40X requests.
2021-08-30 24235, 2021
lucifer
s/40X/50X
2021-08-30 24244, 2021
lucifer
somewhere in the request, an ISE occurs and a 500 is returned without cors headers so it looks like it is a cors issue but its actually an ISE.
2021-08-30 24229, 2021
lucifer
there's an issue with test.lb/beta.lb and lb setup. unlike spotify, CB does not support multiple redirect URIs. so each site has its own app.
2021-08-30 24207, 2021
lucifer
so if you register using beta and try to write a review from prod, the access token will not be considered valid. CB currently does not handle this error correctly, ending up in a 500.
The PoolWorker-1 2021-08-30 16:08:31,345 ERROR: can't locate strategy for <class 'sqlalchemy.orm.relationships.RelationshipProperty'> (('deferred', False), ('instrument', True)) bit is the actual issue, anyway
reosarevok: no, I don't think so. though incidentally I was working on something that could help here
2021-08-30 24254, 2021
bitmap
I'd like us to remove the need for these triggers + rabbitmq, and I think the best way to do that is have sir consume the dbmirror_pendingdata table
2021-08-30 24214, 2021
bitmap
but we'll need to change dbmirror so that it stores the old row versions
2021-08-30 24221, 2021
aerozol has quit
2021-08-30 24231, 2021
aerozol joined the channel
2021-08-30 24204, 2021
lucifer
reosarevok: can you log and share the list of required columns passed to the defer_everything_but function? i'd do it myself but my mb setup isn't up and running yet.
PoolWorker-1 2021-08-30 16:48:33,635 DEBUG: Loading only ['gid', 'name', 'id'] on <class 'mbdata.models.InstrumentType'>
2021-08-30 24220, 2021
reosarevok
But I'm not sure if that's where the error is, or if it just does all of them, then breaks
2021-08-30 24233, 2021
lucifer
oh! i missed that :p. from the issue, it seems that it doesn't work on relationships. many of these are probably relationships but why did it work in the earlier way then...
2021-08-30 24257, 2021
reosarevok
In theory, we split relationships
2021-08-30 24202, 2021
reosarevok
But PoolWorker-1 2021-08-30 16:48:33,634 DEBUG: Loading only ['count', 'tag', 'instrument'] on <class 'mbdata.models.InstrumentTag'>
2021-08-30 24208, 2021
reosarevok
instrument there does seem to be a relationship
2021-08-30 24244, 2021
reosarevok
See if isinstance(prop, RelationshipProperty): on searchentities.py build_entity_query
2021-08-30 24254, 2021
reosarevok
My understanding is that this should detect and split relationships
2021-08-30 24232, 2021
reosarevok
Or is that only about composites, I guess
2021-08-30 24234, 2021
reosarevok
Anyway, it does seem to do the loads differently for relationships...
2021-08-30 24235, 2021
lucifer
not sure. but i would like to see understand why we need the eager loading anyways
2021-08-30 24223, 2021
reosarevok
I would like to understand why we need sqlalchemy :p
2021-08-30 24235, 2021
reosarevok
(ok, ok, apparently because otherwise we can't use mbdata so it's a full rewrite)
2021-08-30 24222, 2021
reosarevok
Oh, next meeting is Sep 9, anyway, lol. I should read the topic
2021-08-30 24255, 2021
lucifer
Sep 6 :)
2021-08-30 24200, 2021
reosarevok
oh. Yeah. That.
2021-08-30 24236, 2021
reosarevok
Given the weird way we use sqlalchemy, according to 2020 bitmap at least, wonder if we could change it to at least use it less weirdly
2021-08-30 24254, 2021
lucifer
i am generally in favor of using an ORM like sqlalchemy but if we have to resort weird stuff then its questionable
2021-08-30 24225, 2021
lucifer
indeed. i suggest maybe revert this commit for now and just fix the breaking changes?
2021-08-30 24252, 2021
lucifer
once we are on the latest version, we can reduce the weirdness. or at least document why it is necessary
2021-08-30 24248, 2021
reosarevok
I mean, I was seeing if this would avoid the breaking changes :p
