MBS-8952: Can rename recordings via autocomplete #3039
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
|
I understand some tests are coming? This looks good otherwise :) |
mwiencek
force-pushed
the
mbs-8952
branch
4 times, most recently
from
5863cb9
to
11629d1
Compare
|
Trying to figure out a heisenbug in the added Selenium test. It passes locally and on paco (the same host and Docker image that the tests use!) when run manually. Perhaps some state is leaking between different test runs. |
I was able to reproduce this using the steps in https://tickets.metabrainz.org/browse/MBS-13021. The primary issue is that recordings from the search server don't include a database row ID, so the `if (currentSelection.id)` check modified here didn't clear the selection when modifying text. The `else` branch updated the recording name with the entered text and notified all subscribers of the autocomplete observable. Checking for `currentSelection.gid` there fixes the bug. I also changed the edit generation code to revert any recording changes if the necessary settings to update them aren't enabled.
Yes, the track parser cookies. |
reosarevok
approved these changes
Oct 4, 2023
reosarevok
added a commit
that referenced
this pull request
Nov 1, 2023
* master: Update POT file from the current database Upgrade Flow to 0.220.0 MBS-13320 (II): Warn on form unload with rel changes (#3064) Add labels to all tests in Data::Editor Give a name to remaining 'all' test and add labels Split various_edit_counts into separate test Split editor subscription methods into separate test MBS-13334: Also find emails with capitalization differences Split and expand email checking tests MBS-13320: Prompt before unloading modified forms (#3054) Split new editor creation test to new subtest Upgrade Flow to 0.219.3 Remove a few $FlowIgnores Remove uses of any in SeriesIndex.js Replace $MakeReadOnly type with one using infer Upgrade Flow to 0.219.2 Upgrade Flow to 0.219.0 Upgrade Flow to 0.218.1 Upgrade Flow to 0.218.0 Upgrade Flow to 0.217.2 Upgrade Flow to 0.217.1 Upgrade Flow to 0.217.0 Remove unnecessary check in concatStringMatch Fix indentation in PhraseVarArgs class Upgrade Flow to 0.216.1 Upgrade Flow to 0.216.0 Upgrade Flow to 0.215.1 Upgrade Flow to 0.215.0 MBS-13319: Show help bubble for language/script on release editor (#3053) Use our new contains_ functions in more places (#3051) MBS-8952: Can rename recordings via autocomplete (#3039) MBS-13317: Remove Kget.jp from lyrics whitelist (#3052) Fix newly detected eslint issues Upgrade ESLint and associated plugins Upgrade Babel dependencies Regenerate yarn.lock Upgrade Flow to 0.214.0 Upgrade Flow to 0.213.1 Upgrade Flow to 0.213.0 Upgrade Flow to 0.212.0 Upgrade Flow to 0.211.1 Upgrade Flow to 0.211.0 Upgrade Flow to 0.210.2 Upgrade Flow to 0.210.1 Upgrade Flow to 0.210.0 Upgrade Flow to 0.209.0 Upgrade Flow to 0.208.1 Upgrade Flow to 0.208.0 Switch Babel & ESLint parsers to hermes Upgrade Flow to 0.207.0 Upgrade Flow to 0.206.0 Upgrade Flow to 0.205.1 Upgrade Flow to 0.205.0 Upgrade Flow to 0.204.1 Upgrade Flow to 0.204.0 Upgrade Flow to 0.203.1 Upgrade Flow to 0.203.0 Upgrade Flow to 0.202.1 Upgrade Flow to 0.202.0 Remove all uses of the switch feature (given/when) Remove all uses of the smartmatch operator Link directly to the anchor in Markdown Update INSTALL.md's building static resources section (#3042)
yvanzo
added a commit
that referenced
this pull request
Nov 13, 2023
* beta: Translated using Weblate (Italian) Translated using Weblate (Lithuanian) Added translation using Weblate (Thai) Added translation using Weblate (Thai) Translated using Weblate (Russian) Translated using Weblate (French) Update translation files Update POT files using the production database Translated using Weblate (Italian) Update translation files Fix wrongly ordered calls l(addColonText(() Update POT files using the production database MBS-13336: Add a script to rebuild all indexes using collations (#3062) MBS-13309: Restrict cross-origin requests to /ws/js/edit (#3075) MBS-13348: Fix edit_data_idx_link_type (#3073) MBS-13349: Support LibraryThing disambiguation URLs (#3077) MBS-13347: Relationship Type edit search times out (#3074) MBS-5987: Add "Submit votes & edit notes" to top (#3072) MBS-13350: Return all barcode matches in release editor (#3076) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (German) Translated using Weblate (Italian) Translated using Weblate (French) Update translation files Add test for isObjectEmpty Add test for isBlank Add test for escapeLuceneValue Add test for escapeRegExp Add test for relationshipDateText Add test for sortByEntityName Move getSortName entities to constants Add test for isDisabledLink Add test for isFutureDate Add tests for strings Make renderMergeCheckboxElement function a component Fix require sort order Add test for isDateEmpty Add test for formatEndDate Add test for entityHref Make generic entity consts reusable Add test for isGuid Add test for isolateText Add test for natatime Add test for primaryAreaCode Add test for isDatabaseRowId Add test for getSortName More precise comparison for null barcode Add test for formatBarcode Add tests for clean Add tests for arrays.js functions Add test for calculateFullToc Add test for bracketed Add renderToStaticMarkup wrapper Use compareDates as the base for areDatesEqual Add test for areDatePeriodsEqual Use exactCount for newline-after-import eslint rule (#3071) MBS-12727: Show genre alias connections for tags (#2757) MBS-4822: Drop duplicate colon strings Add context to "Cancelled" and drop duplicate colons Add context to "Ended" and drop duplicate colons Add context to "Location" and drop duplicate colons Add context to EditReleaseEvents headings Standardize election headers and drop duplicate colons Add context to "Old" and drop duplicate colons Add context to "Status" and drop duplicate colons Add context to "Merge" and drop duplicate colons Add context to "Preview" and drop duplicate colons MBS-13207 (II): Move "Add a new recording" above suggestions (#3069) MBS-12893: Add places column to country statistics MBS-12852: Add events column to country statistics Ensure release stats are calculated for every country Use "create", "add", "enter" in more specific ways (#3070) Bump hermes-eslint to 0.17.1 Bump eslint-plugin-ft-flow to 3.0.1 Move eslint-plugin-simple-import-sort to devDependencies Migrate to Yarn v4 Fix permission errors in run_selenium_tests.sh Bump SIR_TAG to v3.0.1 in the test images Switch to chrome-for-testing in tests Bump chromedriver to 119.0.6045.105 Create the pgtap extension as a superuser Bump musicbrainz-tests-perl-5-dot-30 image Fix phusion/baseimage focal tag Fix PGDATA permission issues MBS-13261: Upgrade the required version of Node.js to 20 Move duplicated CircleCI steps to script Regenerate cpanfile.snapshot Fix apt-key deprecation warning Upgrade the Docker base images to jammy Bump ARTWORK_REDIRECT_COMMIT Copy Dockerfile.tests to Dockerfile.perl5.30.tests Explicitly list the .gitignore'd Dockerfiles Replace xgettext-js with forked copy using hermes-parser (#3067) Use Unicode quotes for string being changed already Use Unicode quotes for string being changed already Remove i.e. from user-facing strings Remove e.g. from user-facing strings Capitalize URL in user-facing string (#3058) Normalize "track duration" to "track length" in UI (#3057) Normalize "pending edits" to "open edits" in UI (#3056) Update POT file from the current database Translated using Weblate (Turkish) Translated using Weblate (German) Translated using Weblate (Japanese) Added translation using Weblate (Chinese (Traditional)) Translated using Weblate (Japanese) Translated using Weblate (Spanish) Translated using Weblate (Portuguese) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Russian) Translated using Weblate (French) Translated using Weblate (Bengali) Translated using Weblate (Portuguese) Translated using Weblate (German) Translated using Weblate (Italian) Translated using Weblate (Lithuanian) Use "remove" consistently for non-editor cases Translated using Weblate (Turkish) Translated using Weblate (German) Translated using Weblate (Japanese) Added translation using Weblate (Chinese (Traditional)) Translated using Weblate (Japanese) Translated using Weblate (Spanish) Translated using Weblate (Portuguese) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Russian) Translated using Weblate (French) Translated using Weblate (Bengali) Translated using Weblate (Portuguese) Translated using Weblate (German) Translated using Weblate (Italian) Translated using Weblate (Lithuanian) Upgrade Flow to 0.220.0 MBS-13320 (II): Warn on form unload with rel changes (#3064) Add labels to all tests in Data::Editor Give a name to remaining 'all' test and add labels Split various_edit_counts into separate test Split editor subscription methods into separate test MBS-13334: Also find emails with capitalization differences Split and expand email checking tests MBS-13320: Prompt before unloading modified forms (#3054) Split new editor creation test to new subtest Upgrade Flow to 0.219.3 Remove a few $FlowIgnores Remove uses of any in SeriesIndex.js Replace $MakeReadOnly type with one using infer Upgrade Flow to 0.219.2 Upgrade Flow to 0.219.0 Upgrade Flow to 0.218.1 Upgrade Flow to 0.218.0 Upgrade Flow to 0.217.2 Upgrade Flow to 0.217.1 Upgrade Flow to 0.217.0 Remove unnecessary check in concatStringMatch Fix indentation in PhraseVarArgs class Upgrade Flow to 0.216.1 Upgrade Flow to 0.216.0 Upgrade Flow to 0.215.1 Upgrade Flow to 0.215.0 MBS-13319: Show help bubble for language/script on release editor (#3053) Use our new contains_ functions in more places (#3051) MBS-8952: Can rename recordings via autocomplete (#3039) MBS-13317: Remove Kget.jp from lyrics whitelist (#3052) Fix newly detected eslint issues Upgrade ESLint and associated plugins Upgrade Babel dependencies Regenerate yarn.lock Upgrade Flow to 0.214.0 Upgrade Flow to 0.213.1 Upgrade Flow to 0.213.0 Upgrade Flow to 0.212.0 Upgrade Flow to 0.211.1 Upgrade Flow to 0.211.0 Upgrade Flow to 0.210.2 Upgrade Flow to 0.210.1 Upgrade Flow to 0.210.0 Upgrade Flow to 0.209.0 Upgrade Flow to 0.208.1 Upgrade Flow to 0.208.0 Switch Babel & ESLint parsers to hermes Upgrade Flow to 0.207.0 Upgrade Flow to 0.206.0 Upgrade Flow to 0.205.1 Upgrade Flow to 0.205.0 Upgrade Flow to 0.204.1 Upgrade Flow to 0.204.0 Upgrade Flow to 0.203.1 Upgrade Flow to 0.203.0 Upgrade Flow to 0.202.1 Upgrade Flow to 0.202.0 Remove all uses of the switch feature (given/when) Remove all uses of the smartmatch operator Link directly to the anchor in Markdown Update INSTALL.md's building static resources section (#3042)
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.
MBS-8952
Problem
If you select any recording which was obtained from the search server in a "Suggested recordings:" list in the release editor, then updating the recording autocomplete text will allow you to change the recording's name and unintentionally submit edits doing the same.
Solution
I was able to reproduce this using the steps in https://tickets.metabrainz.org/browse/MBS-13021.
The primary issue is that recordings from the search server don't include a database row ID, so the
if (currentSelection.id)check modified here didn't clear the selection when modifying text. Theelsebranch updated the recording name with the entered text and notified all subscribers of the autocomplete observable.Checking for
currentSelection.gidthere fixes the bug. I also changed the edit generation code to revert any recording changes if the necessary settings to update them aren't enabled.Testing
Tested locally using a local search server.