MBS-11117: Create report for too long discIDs #1700
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
loujine
force-pushed
the
MBS-11117-long_discids_report
branch
from
eab9a16
to
1e39e2a
Compare
loujine
commented
Sep 19, 2020
loujine
force-pushed
the
MBS-11117-long_discids_report
branch
5 times, most recently
from
5fec551
to
8e89e6d
Compare
yvanzo
reviewed
Sep 28, 2020
There was a problem hiding this comment.
It is currently correctly creating and filling the report table but it fails to display it on error:
TypeError: Cannot read property 'total_entries' of undefined
at CDTocDubiousLength (webpack:///./root/report/CDTocDubiousLength.js?:113:18)
I suspect the code around inflate_rows did not find the cause yet.
DiscIDs should be assigned to only CD format mediums and should not be above 80-something minutes. CDTOC above the cutoff (set to 100 minutes here) were probably created with a buggy version of foo_musicbrainz and can be removed.
Support for MediumCDTocT was added in 50ba95d
reosarevok
force-pushed
the
MBS-11117-long_discids_report
branch
from
36fc4a9
to
686e18a
Compare
|
Pushed a rebased fix. Also changed the Perl files to use CDTOC as we do elsewhere :) |
reosarevok
approved these changes
Oct 9, 2020
yvanzo
approved these changes
Oct 12, 2020
|
|
||
| with 'MusicBrainz::Server::Report::CDTOCReport'; | ||
|
|
||
| sub table { 'cd_toc_dubious_length' } |
There was a problem hiding this comment.
I guess that was the missing bit.
There was a problem hiding this comment.
I think the main issue was the link generated from the reports index was wrong, too :D But a bit of everything maybe :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
MBS-11117: new report about discid durations above 100 minutes, that should be fixed or removed.
cf https://community.metabrainz.org/t/sacd-types-and-discid/494807/8
Solution
cf screenshot on MBS-11117
Action
There are a few things that need a serious review and will probably need rework:
that just passes the parameters of the report SQL query. Not sure it would make more sense to pass the 'id' there, since we don't have gid on the cdtoc table.(Edit: I had to use "id" in the end in order to use CDTocLink)* CDTOCs are not defined with React yet. I didn't want to modify EntityLink so I just defined the href=/cdtoc/... link in root/report/components/CdTocList.js, but that should probably be changed for other conversions to React.I pass durations from SQL in seconds and convert to milliseconds in JS to use formatTrackLength. Exporting durations in millisecond in SQL would be over the 2**31 integer limit in postgres
I don't speak Perl so I just copy-pasted lib/MusicBrainz/Server/Report/CdTocReport.pm, I'm not sure what's inside is correct