elomatreb[m]: Would you mind peeping at the figma and letting me know if I’ve addressed your points? If any others come to mind post them on the ticket any time. https://www.figma.com/file/ln8XiFLit634KC3YkUW2RS…
2022-08-16 22824, 2022
tandy1000
<aerozol> "tandy: so much later sorry..." <- no worries, i meant the artist page redesign!
2022-08-16 22802, 2022
CatQuest
lol I like hitbug is an anagram of github
2022-08-16 22812, 2022
lucifer
yuzie: it was an issue with imports. i changed the import to match how React is imported in existing components and the test works now.
this changes a key "average_rating" from a number to a dict
2022-08-16 22850, 2022
alastairp
in the case that we have no data, we used to return None. now that it's a dict, do you think we should still return None, or an empty dict, or a dict with the relevant fields, but None for values?
2022-08-16 22802, 2022
lucifer
if there's some precedent in CB api, probably better to stick with that. otherwise i think None makes sense.
2022-08-16 22836, 2022
alastairp
I don't think we have any specific pattern for this
2022-08-16 22837, 2022
lucifer
instead of empty dict, count = 0, rating = None may be more sensible.
2022-08-16 22849, 2022
alastairp
yeah, that's what I was thinking. it seems likely that as soon as the "average_rating" key is there, you're going to want to read into it. it seems that we're going to have to have another check at some time to see if there is no rating, but the question is where to do that
2022-08-16 22805, 2022
alastairp
mmm, rating=None vs rating=0 is interesting
2022-08-16 22818, 2022
alastairp
I don't think it's possible to rate something 0
2022-08-16 22837, 2022
lucifer
yeah, iirc then we can only rate 1-5 then returning rate = 0 is probably fine but may be confusing sometimes.
2022-08-16 22818, 2022
alastairp
yeah, I think None is correct because it explicitly indicates the absence
2022-08-16 22846, 2022
ansh
Hi!
2022-08-16 22851, 2022
alastairp
hi ansh :)
2022-08-16 22805, 2022
alastairp
just looking through your stuff
2022-08-16 22841, 2022
lucifer
alastairp: let me know if you have some time afterwards, wanted to discuss some points about misc stuff.
we didn't notice it earlier because websockets already was doing this before we moved it to kombu.
2022-08-16 22807, 2022
alastairp
ah, right. seems reasonable to put it in, then
2022-08-16 22812, 2022
lucifer
issue is likely in librabbitmq, because i didn't encounter the issue yet when using it in spark request consumer where librabbitmq is not installed so its falling back to pyampq.
2022-08-16 22821, 2022
alastairp
ah
2022-08-16 22843, 2022
alastairp
is putting it in timescale_writer enough? will this cause problems in other places too? Do we need to put it somewhere more global?
2022-08-16 22808, 2022
lucifer
we'll need to put it in each consumer. ts writer, spark reader, mbid mapper etc.
2022-08-16 22850, 2022
alastairp
we don't need it in the webserver?
2022-08-16 22857, 2022
lucifer
it should probably be fine. but will need to check if it interacts with uwsgi before putting it in the api.
2022-08-16 22804, 2022
alastairp
yeah, good point
2022-08-16 22837, 2022
lucifer
my main concern was that it isn't documented that this should happen or that monkey_patching is needed for use.
i checked librabbitmq github, the main repo has received a few more commits recently but no release so will try installing from source to see if that helps.
2022-08-16 22848, 2022
lucifer
we are currently using a forked version because main repo was unmaintained for many months.
2022-08-16 22805, 2022
alastairp
right, it's not clear reading these pages how everything fits together and why this is specifically needed
and it falls back to py-amqp. so we've never been using it in reality.
2022-08-16 22846, 2022
ansh
alastairp: I am aware that the tests are failing in CB#453 and CB#455. It is because I added the part to show the relationships in their respective PR's only, because opening a separate PR for it was making it a bit complex.
It's some kind of yoga place that encourages disconnecting and meditating for a few days, so I guess it made sense
2022-08-16 22818, 2022
Pratha-Fish
It was kinda peaceful without internet NGL
2022-08-16 22834, 2022
alastairp
ansh: those PRs look fine. Do you want to go ahead and merge 453 and 455 into 452? then we can release to beta
2022-08-16 22852, 2022
alastairp
I see a few new files that you created have tab indentation instead of spaces, can you check those and fix the necessary files?
2022-08-16 22810, 2022
ansh
Sounds good
2022-08-16 22819, 2022
ansh
I'll fix the indentation issues
2022-08-16 22832, 2022
alastairp
Pratha-Fish: do you want to talk about what you're up to? What you're planning on doing, and if you think that we should extend the finish date of the project?
2022-08-16 22855, 2022
lucifer
alastairp: another thing i was thinking about MsB future. since we now only have track_name, artist_name and release_name as fields. we shouldn't even need a hash.
2022-08-16 22829, 2022
alastairp
hmm. just a unique index over the 3 columns?
2022-08-16 22834, 2022
lucifer
yup
2022-08-16 22858, 2022
lucifer
fwiw, we can't have a unique index in PG because of historical duplicates but can try to emulate the same manually.
2022-08-16 22843, 2022
alastairp
right
2022-08-16 22858, 2022
lucifer
should i go ahead with implementing this?
2022-08-16 22815, 2022
alastairp
yeah, sounds good. how do you plan to work around the duplicates?
2022-08-16 22815, 2022
lucifer
i think we can keep the duplicates around. we can create a non-unique index. for incoming listens, do a where = 3 fields to see if a msid already exists for that data, if so return it otherwise insert a new one.
2022-08-16 22801, 2022
lucifer
alternatively we can add a msid_redirect table but not sure if its worth it.
2022-08-16 22859, 2022
ansh
alastairp: I have fixed indentation issues in both the PRs. Now we can merge them in order, first 453 and then 455. But there would be some merge conflicts I see because of the new way to filter reviews on the reviews page.
2022-08-16 22849, 2022
alastairp
ansh: because they are all on your fork, you should be able to do the merge yourself. Can you merge them into #452 yourself, fix the conflicts, and update the sql migration files so that there is just 1 for the 3 new entities?
2022-08-16 22816, 2022
alastairp
lucifer: as long as that's as easy/fast as the hash column, that sounds fine
alastairp: oh and another thing i realised was that we could probably retire the mbid_mapping_metadata once we start updating mb_metadata_cache table regularly.
2022-08-16 22838, 2022
lucifer
this would be beneficial in multiple ways, one that afair we don't update mbid_mapping_metadata (unless a new listen matches the exact mbid resulting in an upsert). so the mbid_mapping_metadata isn't updated automatically after MB edits whereas the mb_metadata_cache will be so more accurate data.
2022-08-16 22835, 2022
rozlav has quit
2022-08-16 22841, 2022
lucifer
secondly, mb_metadata_cache has more recordings in it. there are some use cases in LB where it can be helpful (LB#2098)
lucifer: yes, using mb_metadata_cache makes sense, as long as we can select the data out of it as quickly as the mapping metadata table (it's much much larger, so I'm not sure if that'll have an effect on speed)
2022-08-16 22818, 2022
ansh
alastairp: for CB#457, I made the change to return ratings: null, count: 0
I don't think that it's necessary in these lists, otherwise it seems that there is some importance to this ordering (I don't think that there is, is there?)
2022-08-16 22830, 2022
alastairp
however, I guess works in a series might make sense to have it?
2022-08-16 22802, 2022
ansh
Yep, in series it would
2022-08-16 22811, 2022
alastairp
ansh: a few other things I noticed: I don't think that "Ordering Type" is useful in series search, I'd remove that column.
tell me if you would prefer these comments in the PRs
2022-08-16 22816, 2022
ansh
Maybe then we could replace by the series total Items count?
2022-08-16 22844, 2022
alastairp
sure, that sounds fine. We don't always have to have 4 columns though, maybe 3 is enough in some cases
2022-08-16 22852, 2022
ansh
Let me check the BB code again to see how it is sorted
2022-08-16 22840, 2022
alastairp
does the "list of works" on an author page have paging? I see some examples that have ~30 works. what's the maximum number of works for an author in the bb database?
2022-08-16 22801, 2022
ansh
Not right now
2022-08-16 22811, 2022
alastairp
ok 👍
2022-08-16 22824, 2022
ansh
alastairp: We cannot put this PR on beta. Because the production code would not know how to handle the new entities. It would result in crashing similar to what happened last time.
2022-08-16 22822, 2022
alastairp
right, good point
2022-08-16 22846, 2022
alastairp
maybe we should set up a full test environment which has a separate database
2022-08-16 22805, 2022
ansh
Yes, I was thinking of the same
2022-08-16 22811, 2022
alastairp
but otherwise, I'm testing this now, it looks really good. I think that we can go straight to the production deployment
we can probably wait for the new design, I was talking to monkey about that
2022-08-16 22810, 2022
ansh
yes that sounds good
2022-08-16 22850, 2022
alastairp
ansh: I've tried entity view/write review/view review for all 3 new entity types, and it looks fine to me. no obvious bugs
2022-08-16 22826, 2022
alastairp
let's fix these few small issues (series work ordering, remove # from artist work list, remove ordering type from series search) and then we can release
2022-08-16 22800, 2022
ansh
Sure! I'll make these changes
2022-08-16 22811, 2022
lucifer
alastairp: with an index, i think it shouldnt matter much but yes makes sense to measure.
2022-08-16 22817, 2022
ansh
alastairp: For series work ordering, I think it requires some time. BB does it differently which I am not able to figure out at the moment. eg: https://bookbrainz.org/series/968ef651-6a70-410f-… I'll open a separate PR for it
2022-08-16 22830, 2022
alastairp
ansh: right, it looks like there is an internal ordering, but there is also the label which says where it comes. let me have a quick look
2022-08-16 22814, 2022
elomatreb[m]
aerozol: looks great
2022-08-16 22820, 2022
ansh
alastairp: Were you able to find how it is working?
2022-08-16 22835, 2022
alastairp
ansh: no, I'm looking through the code and I can't find the bit that makes the query
