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-11852: Only call TO_JSON on editor if it exists #2246

Merged
merged 1 commit into from Sep 22, 2021

Conversation

reosarevok
Copy link
Member

Fix MBS-11852

When trying to approve an edit with a No vote + note, we load edit notes for the edit (to see if one of the editor_ids matches the approver's) but we do not load the actual editors themselves.
On doing TO_JSON for edit when passing it to NoteIsRequired, we were (correctly) running to_json_array on the edit notes, and this was running TO_JSON directly on the note's editor, which, of course, had not been loaded. This changes it to run to_json_object, which is smarter and actually ensures TO_JSON can be called before calling it.

To test, create an open edit with one editor, vote No and leave a note with a second editor, and then try and approve the edit with a third editor. Of course, editor 2 will need to be allowed to vote/leave notes (so, not a beginner) which can be annoying to trigger with sample data, but certainly doable (worst case, just do what I did and enter 10 open edits, then accept them).

@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label Sep 1, 2021
When trying to approve an edit with a No vote + note, we load edit notes
for the edit (to see if one of the editor_ids matches the approver's)
but we do not load the actual editors themselves.
On doing TO_JSON for edit when passing it to NoteIsRequired,
we were (correctly) running to_json_array on the edit notes, and this
was running TO_JSON directly on the note's editor, which, of course,
had not been loaded. This changes it to run to_json_object,
which is smarter and actually ensures TO_JSON can be called
before calling it.
Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

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

thanks!

@reosarevok reosarevok added this to the 2021-10-04 milestone Sep 16, 2021
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.

LGTM but did not test.

@reosarevok reosarevok merged commit 749ebf2 into metabrainz:master Sep 22, 2021
@reosarevok reosarevok deleted the MBS-11852 branch September 22, 2021 10:41
reosarevok added a commit that referenced this pull request Sep 23, 2021
* master:
  Update POT files using the production database
  Update translations from Transifex
  MBS-10902: Stop changing zxx language name at the Perl level (#2255)
  MBS-11961: Rename "random" in edit search since it's not random (#2264)
  MBS-11970: Add recaptcha.net to script-src CSP (#2271)
  MBS-11852: Only call TO_JSON on editor if it exists (#2246)
  Change EOSQL to SQL for TextMate/VSCode
  Re-enable running Perl::Critic now that errors are fixed
  Remove useless .js extensions in component_path
  Fix Perl::Critic useless interpolation and endspace in remaining files
  Fix Perl::Critic useless interpolation and endspace in Controller:: files
  Fix Perl::Critic useless interpolation and endspace in Form:: files
  Fix Perl::Critic useless interpolation in EditSearch:: files
  Fix Perl::Critic useless interpolation and endspace in Edit:: files
  Fix Perl::Critic useless interpolation and endspace in Report:: files
  Fix Perl::Critic useless interpolation in Script:: files
  Fix Perl::Critic useless interpolation in Sitemap:: files
  Fix Perl::Critic useless interpolation in WebService:: files
  Fix Perl::Critic useless interpolation in Entity:: files
  Fix Perl::Critic useless interpolation and endspace in Data:: files
  Fix Perl::Critic useless interpolation in Data::Role files
  Fix Perl::Critic useless interpolation in Data::Statistics
  MBS-11978: Move link action icons to the left (#2273)
  Add seeded links after any existing ones
  MBS-11960: Also assign seeded URL to rawURL
  Log copying JSON dump and search index dump to FTP
  Allow limiting compression threads from config
  Set hostname of MB data export
  Log copying JSON dump and search index dump to FTP
  Allow limiting compression threads from config
  Make user/tag pages 403 if tag is meant to be private
  Fix wrongly named "args" variable
  MBS-10639: Convert release merge edit to React
reosarevok added a commit that referenced this pull request Oct 4, 2021
* beta:
  Update POT files using the production database
  Update translations from Transifex
  MBS-12000: Fix error msg for incompatible entity (#2295)
  Update POT files using the production database
  Update translations from Transifex
  MBS-11998: Also include places on rating stats sum (#2291)
  MBS-11952: Autoselect Vimeo On Demand URLs correctly (#2262)
  MBS-11830: Add more collection statistics (#2198)
  Remove unneeded non-capturing groups
  MBS-11975: Clean up CDJapan detailview URLs
  MBS-10621 (2/2): Normalize and validate Tidal URLs
  Fix URL module alphabetical sort (again)
  MBS-10621 (1/2): Display Tidal URLs in the sidebar
  MBS-11959: Allow RYM links for music video recordings (#2261)
  MBS-11957: Clean up twitch.com to twitch.tv (#2260)
  MBS-11941: Normalize Worldcat identities URLs (#2257)
  MBS-11968: Allow LoC links for series
  MBS-11968: Allow DNB links for series
  MBS-11968: Autoselect and restrict VIAF for series
  MBS-11989: Also correct rg to release_group for delete_alias (#2286)
  Coding style: Restore multi-line strings indent
  MBS-11977 Fix targeting ENTITY for URL rel error
  Add URL error messages for mismatched entity types
  MBS-11973: Also load edits_pending for appears_on (#2278)
  MBS-11896: Remove the unique_primary_for_locale triggers and functions (#2266)
  Bump Flow to 0.160.2
  Also use localizeLanguageName for entity sidebars
  Update POT files using the production database
  Update translations from Transifex
  MBS-10902: Stop changing zxx language name at the Perl level (#2255)
  MBS-11961: Rename "random" in edit search since it's not random (#2264)
  MBS-11970: Add recaptcha.net to script-src CSP (#2271)
  MBS-11852: Only call TO_JSON on editor if it exists (#2246)
  Change EOSQL to SQL for TextMate/VSCode
  Re-enable running Perl::Critic now that errors are fixed
  Remove useless .js extensions in component_path
  Fix Perl::Critic useless interpolation and endspace in remaining files
  Fix Perl::Critic useless interpolation and endspace in Controller:: files
  Fix Perl::Critic useless interpolation and endspace in Form:: files
  Fix Perl::Critic useless interpolation in EditSearch:: files
  Fix Perl::Critic useless interpolation and endspace in Edit:: files
  Fix Perl::Critic useless interpolation and endspace in Report:: files
  Fix Perl::Critic useless interpolation in Script:: files
  Fix Perl::Critic useless interpolation in Sitemap:: files
  Fix Perl::Critic useless interpolation in WebService:: files
  Fix Perl::Critic useless interpolation in Entity:: files
  Fix Perl::Critic useless interpolation and endspace in Data:: files
  Fix Perl::Critic useless interpolation in Data::Role files
  Fix Perl::Critic useless interpolation in Data::Statistics
  MBS-11978: Move link action icons to the left (#2273)
  Add seeded links after any existing ones
  MBS-11960: Also assign seeded URL to rawURL
  Log copying JSON dump and search index dump to FTP
  Allow limiting compression threads from config
  Fix wrongly named "args" variable
  MBS-10639: Convert release merge edit to React
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish
Projects
None yet
3 participants