Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MBS-12395: Report for videos in mediums that shouldn't support video #2562

Merged
merged 1 commit into from Jun 13, 2022

Conversation

reosarevok
Copy link
Member

Implement MBS-12395

This will help find both videos that have been wrongly reused in audio-only releases, and stuff like video CDs marked as just "CD". I hand-picked a list of formats that do not support video rather than a list of those that can because I'd rather have missing entries if we add a new format that doesn't support video than false positives if we add a new format that does.

Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me, except a minor comment about.

👍 for using “a list of formats that do not support video” (rather than a list of those that can) which I just skimmed. These formats should probably be mentioned in the ticket for advanced editors to double-check.

It is not obvious what to do about these recordings. Splitting these and linking via the Recording-Recoding music video relationship? Maybe extraInfo would be useful here.

Did not test this report otherwise. Has it been tested?

lib/MusicBrainz/Server/Report/VideosInNonVideoMediums.pm Outdated Show resolved Hide resolved
lib/MusicBrainz/Server/Report/VideosInNonVideoMediums.pm Outdated Show resolved Hide resolved
@reosarevok
Copy link
Member Author

reosarevok commented Jun 10, 2022

Fair enough about the lack of guidance - I added some, hopefully it's clear :)

Yes, I did test it with sample data and got 43 hits that seemed legit.

Added the formats list for possible review as a comment on the ticket, will wait to merge this until Monday even if it gets approved just in case someone points out issues with the list.

Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all!

A couple more open questions about editing:
Should the two recordings be linked through a music video relationship?
Or does it depend on the editing history and the possibly attached AcoustIDs?

root/report/VideosInNonVideoMediums.js Outdated Show resolved Hide resolved
This will help find both videos that have been wrongly reused
in audio-only releases, and stuff like video CDs marked as just "CD".
I hand-picked a list of formats that do not support video
rather than a list of those that can because I'd rather have
missing entries if we add a new format that doesn't support video
than false positives if we add a new format that does.
@reosarevok reosarevok merged commit 4e69325 into metabrainz:master Jun 13, 2022
@reosarevok reosarevok deleted the MBS-12395 branch June 13, 2022 16:37
reosarevok added a commit that referenced this pull request Jun 13, 2022
* master:
  Update POT files using the production database
  Update translations from Transifex
  MBS-12395: Report for videos in mediums that shouldn't support video (#2562)
  MBS-12356: Correctly select + clean up Tidal store pages (#2515)
  Add comment to ensure test data is kept
  Save AC redirects before swapping AC uses
  Document and standardize Controller::Aliases tests
  Document and standardize Controller::EditAlias tests
  Document and standardize Controller::DeleteAlias tests
  Document and standardize Controller::AddAlias tests
  MBS-9188: Improve LinkedIn URL cleanup (#2553)
  MBS-12419: Block Genius.com links at release level (#2560)
  MBS-12417: Update Soundcloud cleanup to remove ? parameters (#2552)
  Avoid declaring my $tx twice in one test
  MBS-12351: Also trim space only disambiguations (#2510)
  MBS-12376: Don't show spammers on area pages (#2554)
  MBS-12447: Also show area-series rels on series page (#2563)
  MBS-12393: Change TOWER RECORDS to all-caps as per store Japanese usage (#2558)
  Simplify the allowed hosts shortener list
  MBS-12383: Block smart links: bfan.link
  MBS-12396: Block smart links: hyperfollow.com
  MBS-12401: Block smart links: hypeddit.com
  MBS-12350: Block smart links: bio.link
  MBS-12352: Block smart links: streamerlinks.com
  Add basic tests for the artist credit page
  Add IDs to sections of ArtistCreditIndex
  MBS-12354: Check if AC IDs are valid before passing to the DB
  MBS-12312: Convert edit.tt to React
  MBS-12312: Convert history.tt to React
  MBS-12312: Convert diff.tt to React
  MBS-12312: Convert revision.tt to React
  MBS-12312: Remove unused summary.tt
  MBS-12400: fix non-musicbrainz-schema dumps (#2541)
  Sync incremental JSON dumps to trille
reosarevok added a commit that referenced this pull request Jun 20, 2022
* beta:
  Update translations from Transifex
  Add custom hydration event for userscripts (#2566)
  Update POT files using the production database
  Update translations from Transifex
  MBS-12311: Allow adding annotations to genres (#2492)
  Don't ISE on non-existing latest annotation
  MBS-12456: Load editor for latest_annotation on annotation page
  MBS-12455: Show annotation info when loading empty revision
  MBS-12453: Don't crash on null annotation comparison
  Use index, not ID, to enable/disable annotation comparison
  Add basic genre create/edit tests
  Update POT files using the production database
  Update translations from Transifex
  MBS-12395: Report for videos in mediums that shouldn't support video (#2562)
  MBS-12356: Correctly select + clean up Tidal store pages (#2515)
  Add comment to ensure test data is kept
  Save AC redirects before swapping AC uses
  Document and standardize Controller::Aliases tests
  Document and standardize Controller::EditAlias tests
  Document and standardize Controller::DeleteAlias tests
  Document and standardize Controller::AddAlias tests
  MBS-9188: Improve LinkedIn URL cleanup (#2553)
  MBS-12419: Block Genius.com links at release level (#2560)
  MBS-12417: Update Soundcloud cleanup to remove ? parameters (#2552)
  Avoid declaring my $tx twice in one test
  MBS-12351: Also trim space only disambiguations (#2510)
  MBS-12376: Don't show spammers on area pages (#2554)
  MBS-12447: Also show area-series rels on series page (#2563)
  MBS-12393: Change TOWER RECORDS to all-caps as per store Japanese usage (#2558)
  Remove useless Area::Create _insert_hash
  MBS-10165: Use edit system for genre editing
  MBS-10165: Use edit system for genre adding
  MBS-10165: Use edit system for genre deletion
  MBS-10165: Basic preparations for genre edits
  Support genres in formatEntityTypeName
  Simplify the allowed hosts shortener list
  MBS-12383: Block smart links: bfan.link
  MBS-12396: Block smart links: hyperfollow.com
  MBS-12401: Block smart links: hypeddit.com
  MBS-12350: Block smart links: bio.link
  MBS-12352: Block smart links: streamerlinks.com
  Add basic tests for the artist credit page
  Add IDs to sections of ArtistCreditIndex
  MBS-12354: Check if AC IDs are valid before passing to the DB
  MBS-12312: Convert edit.tt to React
  MBS-12312: Convert history.tt to React
  MBS-12312: Convert diff.tt to React
  MBS-12312: Convert revision.tt to React
  MBS-12312: Remove unused summary.tt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature Non urgent new stuff
Projects
None yet
2 participants