yvanzo: reosarevok: the reindex worked fine on bono again. so i think the two workarounds can be get rid of. I'll document the changes and then the SQLAlchemy PR should be ready.
2021-09-15 25814, 2021
lucifer
riksucks: will take a look soon.
2021-09-15 25837, 2021
reosarevok
<3
2021-09-15 25816, 2021
reosarevok
Once that's working I could probably work on porting to python3 myself
2021-09-15 25828, 2021
Toasty has quit
2021-09-15 25813, 2021
Etua joined the channel
2021-09-15 25818, 2021
alastairp
morning
2021-09-15 25820, 2021
alastairp
great work lucifer
2021-09-15 25806, 2021
ruaok
moooin!
2021-09-15 25843, 2021
zas
legoktm[m]: we need to upgrade wiki VM, ping me when you're around
2021-09-15 25841, 2021
ruaok
monkey: when I try and run your code, it appears that /static/userFeedback.js is missing.
2021-09-15 25802, 2021
monkey
Ah, you'll have to rebuild the static_builder image
2021-09-15 25808, 2021
ruaok
I did that.
2021-09-15 25828, 2021
ruaok
I did a docker-compose build and then am runing with static_builder
I know I suggested just yesterday to name the field recording_metadata, and TBH I can live with that. Our usual listen format uses `track_metadata` so we should probably harmonize
zas: I noticed I did not merge it after updating :/ thus alerts on aretha, it's too late for today but I’m looking at setting -T directly instead.
2021-09-15 25801, 2021
reosarevok
bitmap: re MBS-11896 is that something that'd make sense to release on our own servers for now and just put out with the next schema change, whenever that happens?
yvanzo: I'm reviewing the tests rn - I see there's at least one where we have a test already, but no expected_clean_url, I think that's why I made that possible to have without that:
But I can add expected_clean_url there too if we want, shouldn't be a problem (just want to make sure it's not too confusing to have that if we're not cleaning it up)
If the normalized form of links to an external website is going to change (it happens frequently), it seems the favicon should still be shown for both old and new URLs.
2021-09-15 25834, 2021
reosarevok
But then we have to have super-wide regexes, since sometimes we change stuff quite a bit. I personally would actively *not* show favicons for URLs which need updating, because that's a hint that they need fixing
2021-09-15 25856, 2021
reosarevok
But maybe the distraction of not having a favicon is worse than the incitement to fix it is good :)
2021-09-15 25805, 2021
yvanzo
So having favicon test to be run against both input_url and expected_clean_url (if available, otherwise cleanURL() result) seem to make sense.
2021-09-15 25806, 2021
yvanzo
reosarevok: I see you already added optional parts to some of the regular expressions: for example www. to videogam.in
2021-09-15 25818, 2021
reosarevok
Yeah, those are the ones we do not clean up right now
2021-09-15 25832, 2021
reosarevok
I was hoping to actually eventually clean them up and restrict the favicon further :)
2021-09-15 25838, 2021
yvanzo
Do you have “super-wide regexes” in mind?
2021-09-15 25841, 2021
reosarevok
Well, most of these would need to be given ([^/]+\\.)? at the very least
2021-09-15 25800, 2021
reosarevok
I guess we'd also need to match stuff like amzn.com and whatnot
2021-09-15 25817, 2021
yvanzo
reosarevok: Would it be the same RegExp as the `match` property of CLEANUPS? Should favicon be added to CLEANUPS?
2021-09-15 25827, 2021
reosarevok
I was actually going to suggest that too
2021-09-15 25840, 2021
reosarevok
Although I guess we do have a few cases where we match something *then reject it*
2021-09-15 25855, 2021
yvanzo
Yes
2021-09-15 25851, 2021
Sophist_UK joined the channel
2021-09-15 25851, 2021
Sophist_UK has quit
2021-09-15 25851, 2021
Sophist_UK joined the channel
2021-09-15 25811, 2021
yvanzo
It's rejected as “invalid relationship type” most often, not as “this doesn’t match this external website”
2021-09-15 25830, 2021
reosarevok
So you'd still suggest showing the favicon for those? Maybe :)
2021-09-15 25821, 2021
yvanzo
If we can deduplicate code as we handle those too, yes.
monkey hates writing tests for date/time formatting.
2021-09-15 25836, 2021
Sophist-UK has quit
2021-09-15 25850, 2021
reosarevok
yvanzo: can you leave a comment on the PR with what you'd like to see happen based on this conversation? I understand it's mostly "move the check to inside the URLCleanup objects, use those for getFaviconClass(), and check against both the input and cleaned url"?
^ CI front-end tests log output reduced from ~14500 lines to ~1500
2021-09-15 25850, 2021
yvanzo
reosarevok: yes, and I also left a comment on the PR.
2021-09-15 25831, 2021
ruaok
alastairp, lucifer: about?
2021-09-15 25851, 2021
lucifer
yes
2021-09-15 25858, 2021
ruaok
hey
2021-09-15 25800, 2021
lucifer
!m monkey
2021-09-15 25800, 2021
BrainzBot
You're doing good work, monkey!
2021-09-15 25806, 2021
lucifer
hi!
2021-09-15 25812, 2021
ruaok
I'm working on the feedback page with monkey right now.
2021-09-15 25834, 2021
ruaok
in order to populate the page we need data from LB, TS and MSB.
2021-09-15 25836, 2021
alastairp
hi
2021-09-15 25845, 2021
ruaok
oh, perfect timing.
2021-09-15 25807, 2021
ruaok
I hacked out the code to get this done and we're testing it now.
2021-09-15 25830, 2021
ruaok
I'm starting to look to add a test for it, but we have no provisions for testing anything in MSB right now.
2021-09-15 25845, 2021
ruaok
that means createing MessyBrainzTestCase (from DatabaseTestCase), which then needs config.MESSYBRAINZ_ADMIN_URI and MESSYBRAINZ_SQL_DIR and all that.
2021-09-15 25849, 2021
ruaok
is that right?
2021-09-15 25858, 2021
lucifer
right.
2021-09-15 25858, 2021
alastairp
hmm, right. we did have some msb-specific tests from back when it was an independent webapp, and there are some tests that run, but I don't know how much they cover.
2021-09-15 25801, 2021
ruaok
because the msb_db connection is not available during testing.
2021-09-15 25822, 2021
lucifer
there's the msb directory in LB. which have the msb connection
2021-09-15 25823, 2021
alastairp
do you specifically want a standalone msb test, or do you want to access the msb db during a regular test?
2021-09-15 25826, 2021
ruaok
they cover kartekeya's work -- which needs to be discarded.
2021-09-15 25838, 2021
lucifer
those used to run earlier, but we disabled them sometime ago
2021-09-15 25839, 2021
ruaok
alastairp: the latter.
2021-09-15 25856, 2021
lucifer
i see. but it should be possible to get the db setup from those.
2021-09-15 25802, 2021
ruaok
lucifer: the msb connection we already have is not being setup for testing
2021-09-15 25826, 2021
ruaok
but I dive into this, is this what we want to do?
2021-09-15 25850, 2021
alastairp
I'd consider setting up the msb db unconditionally in all tests, and not creating a new base class - unless we can show that it poorly affects test runtime
2021-09-15 25809, 2021
lucifer
yes, it sounds like the right way to me. mocks are brittle.
2021-09-15 25841, 2021
lucifer
if we do itunconditionally, we'd need to create/delete the db after each test.
2021-09-15 25841, 2021
ruaok
I think adding a MessyBrainzTestCase is the right way forward. it just needs yet two more defines added to config.py
2021-09-15 25854, 2021
ruaok hates mocks
2021-09-15 25806, 2021
lucifer
+1
2021-09-15 25858, 2021
Etua has quit
2021-09-15 25800, 2021
reosarevok
Thanks!
2021-09-15 25803, 2021
reosarevok
yvanzo: ^
2021-09-15 25842, 2021
monkey
ruaok: I get this error :
2021-09-15 25842, 2021
monkey
`listenbrainz/listenbrainz/db/model/feedback.py", line 28, in to_api fb.created = fb.created.timestamp() AttributeError: 'NoneType' object has no attribute 'timestamp' `
2021-09-15 25803, 2021
monkey
Although items are indeed returned by the api
2021-09-15 25804, 2021
ruaok
lemme look
2021-09-15 25811, 2021
monkey
That's for API endpoint /feedback/user/mr_monkey/get-feedback-for-recordings
