all listens submitted the same way from picard-tagged files with the same mb artist id
riksucks
Is there any plan for LB to import data from soundcloud for the user?
prabaaaaal joined the channel
peterhil has quit
reosarevok
yyoung[m]: I looked at https://tickets.metabrainz.org/browse/MBS-11951 again and you're right it's the same, but it seems it can't be fixed without removing both relationships - am I missing something? :)
BrainzBot
MBS-11951: Can't fix existing duplicate URL relationship and submit the fix
yyoung[m]
reosarevok: Sorry I have a deadline approaching this evening, I'll get back to you later
monkey: hi, I'm looking at https://github.com/metabrainz/listenbrainz-serv... again, do you think that if we're going to add ctrl-click functionality in that we should put the event management back into the called method?
monkey
Hi !
I think we probably want the event in there
And maybe have a reusable helper method we can pass the event to that will do the ctrl+click fix separately
That way we don't have to reimplement that fix everywhere, we just have to call the right method in the appropriate event handlers
alastairp
sorry, "in there" you mean in `this.changeRange`?
onclick={(e) => this.changeRange("week", e)}; and then have this.handleClick which checks ctrl and prevents as necessary, and call that from changeRange?
after writing that down, my preference would be to have this.handleClick, but call it directly from the onClick= definition, again e doesn't make sense in changeRange except for the purpose of passing it through to another method
yvanzo
reosarevok: not yet
alastairp
monkey: oh, I forgot that when we do the event check we have to decide to run the method inside the guard, as I mentioned in LB-925
so I guess we need to set the actual action to perform as a callback, right? but this callback could be anything... so in TS should we be using `any` for argument lists?
Hm. Indeed. I guess we can make them anchor tags with button styles in another PR
alastairp
ok, will leave those for now. I mentioned in this PR that there was also an issue with the Recommendations page, I'll try and make the same changes on that page too
handling of range and entity params can be done in another pr too, not directly related with this
I don't think we're going to change that a whole lot, I'd say it's not a problem until we add another place that requires the string "year" (or whatever)
alastairp
ok, good for now then
thanks
ruaok
uhm, is develop.sh on LB broken for anyone else?
(on master)
ah, wait. nm.
yvanzo
reosarevok: OAuth PR doesn’t work here. About React PRs, was there a tool to check structure of older edits?
alastairp
monkey: mm, one more thing that I could ask your help on
here I cap the minimum value to 1. it'd be great to also cap the maximum based on this.state.totalPages, but because of the code flow it's not really possible
from what I can understand, `syncStateWithURL` first gets the parameters (`getURLParams`) and then queries the data. After it gets the data (including the total number of items) it can calculate the max number of pages, but there's no other place where we set the url again after this
I think it's a bit too much work to rewrite this functionality in this PR, so I'll leave it as-is and open a ticket for a future improvement
while looking at this, I noticed calls to setState within syncStateWithUrl - https://github.com/metabrainz/listenbrainz-serv..., I recall reading up about this elsewhere and understanding that you can't guarantee that state will change from 1 line to the next unless you use a callback - is that right?
monkey
Not sure I follow. `syncStateWithURL` is called on each browser history change (so for example clicking before/after browser buttons). So we could check if totalPages has been set: i fit has, cap the max page number; if it hasn't, just proceed without checking max page value
>you can't guarantee that state will change from 1 line to the next unless you use a callback - is that right?
but on first page load, the first time syncStateWithURL is called, there is no data yet
so we don't know what the last page will be
monkey
Right. On first load we can't check the max page #
(unless we pass it as a prop from the backend)
alastairp
but we could - load page, do API query, see that page number is too high, limit to max, update url, re-do the API query to get that page
monkey
I mean, if a user enters a random page number that's too high and they get an error page instead of being redirected to the last possible page, I'd be OK with that.
As long as our navigation system doesn't do that, i think it's acceptable to fail if the user's query is incorrect)
alastairp
I see
we already have the error message for invalid entities and range (in the image I posted above), so we could handle this case in the same way
monkey
Maybe I'm just too lazy, but I think that would work well.
alastairp
although I was just about to propose that if one of those values were invalid that we should just reset it to a default value too
monkey
It's a bit of an edge case, I feel
alastairp
yes, it definitely is
so not absolutely urgent. I see it as a nice-to-have