MBS-12761 / MBS-14180: Convert recording edit form to React #3378
Conversation
reosarevok
force-pushed
the
MBS-12761
branch
4 times, most recently
from
b06b776 to
7ee0573
Compare
reosarevok
force-pushed
the
MBS-12761
branch
6 times, most recently
from
4c9a7be to
0d853c8
Compare
reosarevok
force-pushed
the
MBS-12761
branch
3 times, most recently
from
958b25f to
29cca61
Compare
reosarevok
force-pushed
the
MBS-12761
branch
2 times, most recently
from
4b6ca3f to
ace6a5b
Compare
reosarevok
force-pushed
the
MBS-12761
branch
3 times, most recently
from
f803754 to
ca422fc
Compare
|
I think this is ready for review now, although before merging the commits probably could be put together in a more reasonable way (not sure exactly whether to combine most into one or not) |
reosarevok
force-pushed
the
MBS-12761
branch
from
bc757e1 to
700c20a
Compare
reosarevok
changed the title
reosarevok
force-pushed
the
MBS-12761
branch
2 times, most recently
from
2ca4fcd to
8a802d9
Compare
|
A few issues I found while testing:
|
reosarevok
force-pushed
the
MBS-12761
branch
2 times, most recently
from
16872e5 to
8f1ac08
Compare
|
Done. Now I can't think of anything else to do, please feel free to find anything I broke and still failed to test :) I didn't test seeding very deeply, but we do have some automatic tests for that - that said, @mwiencek already broke the page once like that so there might be more cases. |
mwiencek
left a comment
mwiencek
left a comment
There was a problem hiding this comment.
A few other minor things I found...
- If I enter an invalid ISRC, the margin or padding of the error message appears misaligned (it's in the center of the field, but should be left-aligned). Also, the server uses "This is not a valid ISRC.", while the JS uses "Invalid value."
- Causing the bubble doc to change (by switching focus on another field) makes the bubble flicker and appear/disappear in the top left of the page.
- If I seed an invalid length, it doesn't show an error message.
| @@ -0,0 +1,301 @@ | |||
| /* | |||
There was a problem hiding this comment.
Is there a reason the entire file needed to be forked? I understand the old one is setting the values directly on knockout observables. But it seems like the old guessFeat function should be able to just call into the new one, setting the observables based on the return value of the new function.
There was a problem hiding this comment.
The main point was to avoid mixing up stuff and just have a ready-to-go file that we can rename when we delete the other one once we get rid of ko, but if you think it would be better to have one file, I can look into it.
There was a problem hiding this comment.
I think it should be mostly reusable, but if you prefer you can just add some brief comments on the files saying to keep them in sync. We don't change them often anyway. Edit: But one nice thing about modifying that existing file is that it'd preserve the git history better.
There was a problem hiding this comment.
Oh, now I remember another reason, which was to not have to figure out the flow typing for the ko stuff and still have a proper flow-typed file for use with the new code.
| const formCtx = mutate(form); | ||
| // $FlowExpectedError[incompatible-call] | ||
| const nameFieldCtx = formCtx.get('field', 'name'); | ||
| updateNameFieldErrors(nameFieldCtx); |
There was a problem hiding this comment.
Hmm, one problem with disabling the submit button outright is that I can't find a way to get the pendingErrors on the name field to actually display. While it should be obvious that a name is required, it would also make sense to actually show that as an error message if the user tries to submit the form.
There was a problem hiding this comment.
I guess we could leave the button for required fields. The same happens with the AC though which I don't think will be as obvious, so we should probably at most prevent sending and show pending errors in case of submission?
reosarevok
force-pushed
the
MBS-12761
branch
3 times, most recently
from
ba17d88 to
8830616
Compare
reosarevok
changed the title
This was very useful shortly after NGS migration, but it hasn't been really needed for years since most of the old-style MB classical data has been fixed. New classical releases (from data outside MusicBrainz) do not use the old MB style this is built to fix at all, so there's no need to keep it and we can simplify the guess feat code.
We will need an updated guessFeat for the recording form on a subsequent commit. Since this makes a fair amount of changes and improves the tests, I'm splitting it into its own commit for ease of review. I'm not touching the existing knockout guessFeat, which will still be needed in the release editor and the RG form until those two get converted to React as well. Once that is complete, we can remove the last two functions of the guessFeat file and simplify the tests a bit.
This uses the new guessFeat introduced in the previous commit. This is an initial conversion; further commits introduce extra features like a lot more validation at the JS level before the user submits the form.
This removes the need for initializeValidation() and replaces it with a more proper React way of disabling submit using state. I made updateNameFieldErrors reusable so we can update the form when creating the state, otherwise the submit button won't get disabled until the user writes something in the name field.
This avoids blanking the artist credit when reloading the page on the recording creation page (now it will keep any AC the user had entered).
This can be triggered via `/recording/create?edit-recording.artist_credit.names.0.artist.id=foo`.
I'll next use it as part of a new isInvalidLength JS check.
Our unformatTrackLength JS implementation was a lot different from the Perl one. This worked fine when it was only used for the release editor, but now we want to also use it to test for validity at the JS level, so it should probably consider as valid the same things the Perl backend will. This commit mostly changes the unformatTrackLength JS version to work more like the Perl one. There's two things that the Perl version did not allow that probably make sense, so I've added them there: :SS times without minutes, and just parsing a number of seconds. I'm removing the test for 3723494 seconds from fields.js because that's actually a larger number in ms than we accept in the DB (and was being rejected by the new method). Similarly, :10 should now be an accepted track time, so changing that test to use a time that is actually still disallowed.
This modifies FormRowTextList to make it properly usable inside a larger form; rather than have its own state, it accepts dispatch and state props. Additionally, I'm moving currentTextValues outside the state since it is a constant, and just passing it as a prop. This was in the state for ease of use in the reducer, but it can just be passed by the dispatch as needed. For uses where the FormRow still needs its own state (because it's just a standalone component used in a TT form) I added StandaloneFormRowTextlist, to which the old local state has moved. I added isValidIsrc for JS-level ISRC validation; this combines is_valid_isrc and format_isrc from Server::Validation since we don't want to reject ISRCs that are valid once formatted by the system. I expect it'd be more confusing for the user to automatically change the format of the ISRC on the form itself so I'm letting it happen at the Perl level as earlier.
For creating standalone recordings, it makes sense to display an error saying a note is required from the get go. This additionally set errors for seeded bad edit notes.
The previous code invokes `createInitialState` on every single render. I noticed when changing the recording form that we still had these three around.
We don't intend to change nameStateCtx after assigning it to nameState in any of these forms, so it is safer to use final() to make sure it's not writable anymore.
useEffect causes an annoying flickering effect on changing the bubble position that seems to be avoided by useLayoutEffect. Thanks @mwiencek for the suggestion.
This is useful only for seeded lengths, but it makes sense to show an error for those when invalid.
3 participants
Implement MBS-12761, MBS-14180
Description
Should be mostly a 1:1 conversion, except that this also blocks submission at the JS level for more invalid data issues (empty artist credit, invalid length or ISRC), which we didn't do in the TT form.
A commit implements MBS-14180 to drop classical guess feat - this simplifies the rest of the
guessFeatupdate somewhat.Testing
Tested adding and editing recordings, including:
/create) and when seeding.potfiles with./po/update_pot.shand updating thepofile withmsgmerge --no-wrap --no-fuzzy-matching --update po/mb_server.fr.po po/mb_server.pot, then recompiling resources.Additionally, the automatic tests we have for this still pass.