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/ln8XiFLit634KC3YkUW2...
tandy1000
<aerozol> "tandy: so much later sorry..." <- no worries, i meant the artist page redesign!
CatQuest
lol I like hitbug is an anagram of github
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
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?
lucifer
if there's some precedent in CB api, probably better to stick with that. otherwise i think None makes sense.
alastairp
I don't think we have any specific pattern for this
lucifer
instead of empty dict, count = 0, rating = None may be more sensible.
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
mmm, rating=None vs rating=0 is interesting
I don't think it's possible to rate something 0
lucifer
yeah, iirc then we can only rate 1-5 then returning rate = 0 is probably fine but may be confusing sometimes.
alastairp
yeah, I think None is correct because it explicitly indicates the absence
ansh
Hi!
alastairp
hi ansh :)
just looking through your stuff
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.
alastairp
ah, right. seems reasonable to put it in, then
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.
alastairp
ah
is putting it in timescale_writer enough? will this cause problems in other places too? Do we need to put it somewhere more global?
lucifer
we'll need to put it in each consumer. ts writer, spark reader, mbid mapper etc.
alastairp
we don't need it in the webserver?
lucifer
it should probably be fine. but will need to check if it interacts with uwsgi before putting it in the api.
alastairp
yeah, good point
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.
we are currently using a forked version because main repo was unmaintained for many months.
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.
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
It was kinda peaceful without internet NGL
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
I see a few new files that you created have tab indentation instead of spaces, can you check those and fix the necessary files?
ansh
Sounds good
I'll fix the indentation issues
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?
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.
alastairp
hmm. just a unique index over the 3 columns?
lucifer
yup
fwiw, we can't have a unique index in PG because of historical duplicates but can try to emulate the same manually.
alastairp
right
lucifer
should i go ahead with implementing this?
alastairp
yeah, sounds good. how do you plan to work around the duplicates?
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.
alternatively we can add a msid_redirect table but not sure if its worth it.
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.
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?
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.
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.
rozlav has quit
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)
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?)
however, I guess works in a series might make sense to have it?
ansh
Yep, in series it would
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
ansh
Maybe then we could replace by the series total Items count?
alastairp
sure, that sounds fine. We don't always have to have 4 columns though, maybe 3 is enough in some cases
ansh
Let me check the BB code again to see how it is sorted
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?
ansh
Not right now
alastairp
ok 👍
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.
alastairp
right, good point
maybe we should set up a full test environment which has a separate database
ansh
Yes, I was thinking of the same
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
ansh
yes that sounds good
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
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
ansh
Sure! I'll make these changes
lucifer
alastairp: with an index, i think it shouldnt matter much but yes makes sense to measure.
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-410... I'll open a separate PR for it
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
elomatreb[m]
aerozol: looks great
ansh
alastairp: Were you able to find how it is working?
alastairp
ansh: no, I'm looking through the code and I can't find the bit that makes the query
