#metabrainz

yvanzo: ticket editing permissions seem too important to restrict IMO

So I guess we need to live with users changing priorities

Can we follow priority changes exclusively?

Mr_Monkey: yes, thanks for resolving that issue.

Cyna: make sure req is being outputted by the sanitizedContext function, we went over this in another branch

I made some changes, removed the uses of $c in the file that is being hydrated

Replaced by directly passing in params

`TypeError: $data.selectionMessage is not a function` getting this in the console now

bitmap, yvanzo: do you want to talk PRs then? :)

reosarevok, yvanzo: I'm around, just making coffee first

sure :)

We can have an automation rule that mails us when priority changes https://docs.automationforjira.com/reference/tr...

(there is no notification for field-level change)

aidanlw17: Cyna: please put your flight numbers on the wiki for the summit so we can coordinate arrival times

Well My flights haven't been booked yet, As the VISA process is still pending

soo where should we start? https://github.com/metabrainz/musicbrainz-serve... maybe? (seems to be the oldest open React PR)

Cyna: great, as soon as you know then :)

seems to have no conflicts, so should be in a good state. would it help to try doing the code review on irc?

yvanzo: ^ :) (works fo rme)

alastairp: I need help in defining a config var name.

maybe asking "why is line x doing y" or "i'm not sure about z" here would be faster than back-and-forth on github

We have a date time value = x and we wish to move date in x to a particular date, something like, date.replace(days=y)

What should be the name of y ?

Y is a integer here.

an*

bitmap, reosarevok: Rule added.

bitmap: `TypeError: $data.selectionMessage is not a function` what does this mean ?

dependency issue ?

I've ruled out that its a syntax error

ruaok can also help:)

Cyna: search for area_bubble root/components/forms.tt. it must not be applying the knockout bindings correctly ($data should be an area entity)

bitmap, reosarevok: I actually started to review it today, shouldn’t be long to finish, can we jump to the next one?

okay

https://github.com/metabrainz/musicbrainz-serve... ?

I'll quickly rebase

Not that the conflict is very relevant, but

we haven't added any new NotFound pages since, I hope

Cyna: Just for a test, can you please set Priority to Low at https://tickets.metabrainz.org/browse/MBS-9911

MBS-9911: Convert attributes admin templates to React/JSX

I guess it should be easy to see from components.js after you rebase

bitmap: we added genre but IIRC I already updated it for that

okay, good

Rebased, pushed

I don’t really the added value of refactoring all NotFound in a single file, there is almost no code deduplication here, am I missing something?

They're all the same concept, with mostly small variations on one string

It makes much more sense to me for all of it to be together

(if we decide to change something of how it works, we can change one file rather than 30)

yvanzo: done

alastairp: I’ll put mine up right now!

Cyna: thanks! yvanzo: works nicely!

the NotFound pages all previously shared the same structure, so they had the same imports and markup, just the messages differed

We could have the rule to automatically restore the previously set priority with an explanatory comment but I feel we should preferably review it.

so not much actual code/logic was duplicated, but a lot of boilerplate (imports/markup)

pristine__: can you pastebin the code with a bit more context?

Yeah, I agree that if it's not crazy (and it shouldn't be) we should review it :)

I suspect around half are just pointless changes, but half are relevant

bitmap: the bindings werent applied because I didnt add ```<!-- ko with: target() && target().area -->

{...}

<!-- /ko -->```

Its strange adding something like this to React

So I left it out

Dont know how to make it work, cause it doesnt work directly

Well, I don’t think it hurts.

line 19

yvanzo: the priority thing or the PR?

I don't think passing "1" is a good idea. I want to define this in config

alastairp: ^

ruaok: How have we configured the three workers. I don't understand it through the docker compose. On my local machine, I have one master container and one worker container as reflected in docker compose, and the same docker compose is used on leader, then how three worker containers?

reosarevok: the PR

whoever moderates the summit schedule of events https://wiki.musicbrainz.org/MusicBrainz_Summit/19 , the Monday is September 30, rather than October 1!

yvanzo: ok, other than that is there something in the code to comment etc? :)

aidanlw17: it's a wiki ;)

reosarevok: just dunno if it's ok for me to change the schedule? guessing that's a yes ;)

If it's a clear error, sure

(otherwise, suggest only maybe :) )

yvanzo: I'll take that as a no :)

Ok, thanks!

Oh, I saw the comment now

reosarevok: sorry, approved on github already

nice, wanna do a couple more?

Works for me

I need hours this week anyway, I've been too lazy :p

https://github.com/metabrainz/musicbrainz-serve... actually has a recent review already

Oh! :)

I guess the comment is a good one in that the variable is not very clearly named

Will rebase + rename!

though with xByY naming I usually think it's an object where you can lookup an x using y as a key

+ TODOs should probably be better named/placed to be meaningful outside of the PR.

bitmap: any suggestions that make it more clear?

maybe tracksWith instead of tracksBy?

Maybe :)

+1

for TODO: RecordingT should have an optional artistCredit property

it makes it sound like we should change RecordingT, but it should only be optional through recordings

I mean tracks, sorry

as in track.recording should be something like RecordingWithOptionalArtistCreditT :)

Is there a way to say "RecordingT, but with this one difference" without defining it all again

?

If there is, we could just change it now I guess

maybe $ReadOnly<{|...RecordingT, artistCredit?: ArtistCreditT|}> ?

haven't tested it

"Test" means "run flow, see if it screams", right? Can do

but I think later properties should override old ones in spreads like that

No errors, so I guess it's fine

For the other, is it enough to give " // TODO: cdtocs needs to be defined eventually: +cdtocs: $ReadOnlyArray<MediumCdTocT>" ? yvanzo?

Or "TODO: still missing cdtocs: etc"?

isn't it "recording" attribute rather than "RecordingT" here that should have optional "artistCredit"?

(but that's better if you fixed it rather than keeping a TODO :)

What's the difference? The recording attribute is a RecordingT, right? So it's "a RecordingT but with optional artistCredit

yup

reosarevok: no worry, I was just typing about the comment while you were fixing the real issue.

the difference is just the wording, adding attribute would make it clear you're not talking about the type

Oh!

Gotcha

this seems to {+artistCredit?: ArtistCreditT} & RecordingT

the other TODO wording is fine although I'm not sure it is necessary and correct (shouldn't cdtoc be optional in some case?)

I'm not sure they're always loaded, but we always output a cdtocs array in Medium::TO_JSON

so it would just be an empty array if they're not loaded

OK then!

IS something like this better?

Is there a way to check that JSON passed to the renderer matches flow types at run time?

we could add something like https://gajus.github.io/flow-runtime/#/ enabled in development mode

reosarevok: please add 💿 💿 💿 ;)

$ReadOnlyArray<💿>

Ok, more seriously, I'll push

bitmap: looks good, added https://tickets.metabrainz.org/browse/MBS-10298

MBS-10298: Check flow type at run time

thx

pristine__: hmm, a few ideas here

how many times do you use this method replace_days? if it's only to get the first of the month, then I'd name it something like get_first_of_this_month instead

in the case that you have at the moment, `days` is ok

however, in this case I wouldn't do it like this

instead, I'd pass in the year/month to create_training_data_from_window

back to the 1st PR, what is the purpose of https://github.com/metabrainz/musicbrainz-serve...

this way you could theoretically calculate the data for any month

yvanzo: https://github.com/metabrainz/musicbrainz-serve...

normally it's better to pass information like this (what month to act on) as an argument, and collect the data as close to the "user" as possible

e.g. if it's called by a user or by cron, either pass in this information as an argument, or in the main function get the current month and then pass that into your function

reosarevok: OK, thanks, would be nice to explain that we had both host and Referer in the commit message

Can do, how would you word it?

bitmap: ? :)

well, fixing the case allows us to access them consistently