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

The Final Form PR #221

Merged
merged 47 commits into from
Aug 15, 2019
Merged

The Final Form PR #221

merged 47 commits into from
Aug 15, 2019

Conversation

shrugs
Copy link
Contributor

@shrugs shrugs commented Jul 31, 2019

Thoughts, @Fang-? should support our InviteEmail use-case along with hasReceived, etc as a async validator. had some problems with final-form (re: email and some un-told), so any weird architecture you see might be because of those. lmk what you think. definitely 'feels' better to me.

@shrugs shrugs changed the base branch from master to mino July 31, 2019 19:34
Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Preliminary looks good to me. These diffs seem to appear larger than they really are.

src/form/Autosaver.js Outdated Show resolved Hide resolved
src/views/Activate/ActivateCode.js Outdated Show resolved Hide resolved
src/views/Activate/ActivateCode.js Outdated Show resolved Hide resolved
src/views/Login/Ticket.js Outdated Show resolved Hide resolved
@shrugs
Copy link
Contributor Author

shrugs commented Aug 5, 2019

I don't think there's anything wrong with not submitting the forms that we're creating, but we're technically not fulfilling the form API as part of the onValues type interactions (namely when also using InlineEthereumTransaction). I expect there's a nice clean API that can be use when creating a form that will also be submitted as an Ethereum transaction, but imo that's out of scope here since this is big enough as-is.

@shrugs
Copy link
Contributor Author

shrugs commented Aug 5, 2019

(i.e. returning a promise from onSubmit that resolves when the ethereum tx is confirmed and then using the state from final-form to handle some logic for presentation instead of using useEthereumTransaction#completed. idk, just spitballing at 10pm

@Fang-
Copy link
Member

Fang- commented Aug 6, 2019

I don't think there's anything wrong with not submitting the forms that we're creating, but we're technically not fulfilling the form API as part of the onValues type interactions

I don't think I understand what this means. I might be missing some context here on what we should be doing.

@shrugs
Copy link
Contributor Author

shrugs commented Aug 6, 2019

I don't actually know if we 'should' be submitting the forms or not, only that for our forms that use the live 'onValues' type api, they're never submitted. this probably isn't a problem, but worth noting that the form library may expect that the form is submitted at some point (but I don't think we use any of its features around submission, which is why this isn't a big deal—more of a 'heads up' to myself, really)

@shrugs shrugs marked this pull request as ready for review August 6, 2019 08:47
Copy link
Contributor Author

@shrugs shrugs left a comment

Choose a reason for hiding this comment

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

diff is slightly off on certain files, making the add/remove count a bit larger than it should be

@shrugs shrugs requested a review from Fang- August 6, 2019 10:20
@shrugs shrugs closed this Aug 6, 2019
@shrugs shrugs reopened this Aug 6, 2019
@shrugs
Copy link
Contributor Author

shrugs commented Aug 6, 2019

(accidentally closed instead of cancelling a comment I wrote lol)

@shrugs
Copy link
Contributor Author

shrugs commented Aug 13, 2019

here's what that looks like
Untitled

@Fang-
Copy link
Member

Fang- commented Aug 13, 2019

which comments are still pending?

Hmm, I think we're good actually, with recent changes. Functional review incoming!

The on-focus unmasking seems nice, but I'd maybe vote against it still. For reference.

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

login

If I change the contents after getting a warning, the "login anyway" should change back into "continue" and do validation again on-click, instead of proceeding with the previously-derived value.

Also, as you can see, it's not doing the on-focus showing/hiding for me...

Trying to generate email invites gives me this:

image

And it just seems to hang forever until I quit it. I don't think it's generation, that should complete eventually (and not happen anyway, in my current case where the email sender is down), so maybe some loop got introduced here?

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019 via email

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

@Fang- actually, simply disabling the inputs after successful (but warned) submit isn't a real solution, since now the user can't go 'back' and edit their values.

I've changed the approach in the latest commit to use final-form's error feature instead of working around it. Now we have a custom WARNING error that we can return from a failed submit, which means final-form doesn't consider the form 'succeeded'. We keep track of whether or not we're double-submitting using the didWarn reference, so the second submission from a pristine (i.e. !dirtySinceLastSubmit) form will pass through correctly.

This does have the downside that our submit function is called twice, which means generation happens twice in the case that you want to auth with 'invalid' credentials or if your invite code has multiple points to be claimed, both of which are highly unlikely scenarios.

We also correctly update the state after you edit, so the warning will go away until the submission fails again.

I also added the submitSucceeded check on all of the inputs cause it's almost definitely the right thing to do anyway, since we never (?) want anyone editing the form after we've successfully submitted (we always either hide the inputs or redirect away anyway).

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

give it an 👀 and lmk what jimmy says about the masking toggle so I can implement that monday morning.

@shrugs shrugs mentioned this pull request Aug 14, 2019
@Fang-
Copy link
Member

Fang- commented Aug 14, 2019

re: masking toggle: we should just do a "check to reveal" checkbox underneath the ticket input. This should be consistent on both login and ticket verification during activation & reticketing.

That is separate work for a separate PR though, this one just needs to be merged after this long. Same goes for your fix for #229. Smaller PRs is more better!

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

@Fang- I'm worried that the checkbox could be confusing on login, since we also have additional checkboxes below that. probably needs some sort of visual hierarchy/separator.

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

we can merge this now and I can make masking part of a separate PR as well. up to you

@Fang-
Copy link
Member

Fang- commented Aug 14, 2019

@jyng do you have thoughts/mockup for us?

This is not fine to be merged, since invite email sending is still broken for me. It starts hanging before it even gets to doing derivation, and it didn't do this before.

Behavior is much more functional if mailer service is online, though it doesn't display the "email has already received" message until you un-focus the field, which you might not make if the "generate invites" button is unclickable. That's a bit confusing.

@jyng
Copy link
Contributor

jyng commented Aug 14, 2019

@shrugs @Fang- What are the original fixes that this PR is addressing? I'm a sort of losing the trail of conversation here.

@jyng
Copy link
Contributor

jyng commented Aug 14, 2019

Screen Shot 2019-08-14 at 1 40 20 PM

Nice

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

This is not fine to be merged, since invite email sending is still broken for me. It starts hanging before it even gets to doing derivation, and it didn't do this before.

oops, skipped my mind. do you have those details about what browser you're using & how many emails you're generating etc? fwiw I'm able to go through the happy path for invite sending with one email, but i realize 'works on my device' is unhelpful. will test this with a bunch of invites and try to debug.

@jyng

  1. replaces our home-grown form logic with a more popular/standard lib, though not without its own quirks. this results in a lot cleaner and less confusing codebase, this PR excluded
  2. lets us do async validation/submission, the lack of which was causing a bunch of inconsistency errors in the mino version (i.e. generating keys, checking whether or not an email has already received an invite, seeing if a star is able to be spawned, etc)
  3. the ticket masking problem (partial input masking is a no-go, so next best is definitely the checkmark, see Mark's http://passwordmasking.com/ link for deets
  • I'm waiting for your opinion on what that should look like, since, imo, chucking another checkbox into the list below the input on login could be confusing due to the lack of hierarchy, so it needs some distinction that it's attached to the input and is not a separate option on its own.

@jyng
Copy link
Contributor

jyng commented Aug 14, 2019

@shrugs Ok, I think 3 should be in a different PR, as its not blocking us from launching. Ping me and we'll continue the discussion for that once that new PR has been made. Let's try focus on fixing 2 and get it merge ready asap.

@Fang-
Copy link
Member

Fang- commented Aug 14, 2019

do you have those details about what browser you're using & how many emails you're generating etc?

Firefox, and just one email. As I said, it doesn't get into doing generation, so the amount of emails shouldn't matter here. (And it didn't take >10 seconds before, and it just hangs forever (>minute) rn.)

fwiw I'm able to go through the happy path for invite sending with one email

Is it connecting to an email service successfully? If you stub that out to localhost (I think it already is) and don't actually have that running, what happens? In my case, it hangs.

@Fang-
Copy link
Member

Fang- commented Aug 14, 2019

fwiw I can reproduce in chrome

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

I've never had the email service running, unfortunately. I remember receiving the code for the gas tank, which I got running, but I'm not sure where the email code lives.

Regardless, the invite email page hadn't been updated to the new format of buildValidator, which is why things were being wonky. The stubbing definitely caused me to not run into that problem when manually testing.

I've coded up a fix but it's late so I'm going to review it tomorrow morning and then submit it here.

@shrugs
Copy link
Contributor Author

shrugs commented Aug 14, 2019

ty for patience on that one

NOTE: the render function diff will be large because of the indentation
change within the `FieldArray` component.

+ update the buildValidator usage to allow additional (optionally async)
  validators per-field and in buildArrayValidator
+ fix: FormError was listening to a wrong error value in a subscription
+ commented more about FormError logic for clarity
+ pull Input.js error logic forward
+ fix FF-specific box-shadow css issue on required & invalid inputs
+ for plurality, we want to use the plural form for 0 counts as well
  (i.e. '0 errors' vs '0 error')
+ simulate a delay when stubbing useMailer
+ switch back to using `name` as the key for invites, receipts,
  and errors in InviteEmails.js, which fixes the accessory bug
  (accessories were not updating when they should have)
+ use the new WARNING pattern for messaging about errors during
  submission while still allowing submission anyway
+ add copy clarifying that the form can still be submitted
+ we now only send the valid invites, avoiding a `throw` case
  + we only tell the user about valid invites that were sent
+ if there was a warning generating invites, we allow the user to send
  the invites anyway

known issues that could/should be handled in a future PR:

- InviteEmails error handling still needs some 👀
- InviteEmails Inputs don't respond to the `error` state tracked within
  InviteEmails, only the error state within final-form's data-model
- you can "Send Invites" with 0 valid invites. Nothing bad happens
  (nothing happens at all, really — you get a success state with
  '0 invites were successfully sent')
@shrugs
Copy link
Contributor Author

shrugs commented Aug 15, 2019

here's a dupe of that last commit message to describe why I made some of the changes. give it a run through and lmk.

I wasn't able to reproduce the 'this page is taking a while' error, but I expect it's solved due to correctly handling the validation promise now


feat: tidy up InviteEmails.js and cover a few error edge cases

NOTE: the render function diff will be large because of the indentation
change within the FieldArray component.

  • update the buildValidator usage to allow additional (optionally async)
    validators per-field and in buildArrayValidator
  • fix: FormError was listening to a wrong error value in a subscription
  • commented more about FormError logic for clarity
  • pull Input.js error logic forward
  • fix FF-specific box-shadow css issue on required & invalid inputs
  • for plurality, we want to use the plural form for 0 counts as well
    (i.e. '0 errors' vs '0 error')
  • simulate a delay when stubbing useMailer
  • switch back to using name as the key for invites, receipts,
    and errors in InviteEmails.js, which fixes the accessory bug
    (accessories were not updating when they should have)
  • use the new WARNING pattern for messaging about errors during
    submission while still allowing submission anyway
  • add copy clarifying that the form can still be submitted
  • we now only send the valid invites, avoiding a throw case
    • we only tell the user about valid invites that were sent
  • if there was a warning generating invites, we allow the user to send
    the invites anyway

known issues that could/should be handled in a future PR:

  • InviteEmails error handling still needs some 👀
  • InviteEmails Inputs don't respond to the error state tracked within
    InviteEmails, only the error state within final-form's data-model
  • you can "Send Invites" with 0 valid invites. Nothing bad happens
    (nothing happens at all, really — you get a success state with
    '0 invites were successfully sent')

@Fang-
Copy link
Member

Fang- commented Aug 15, 2019

This still gives me the hanging behavior for clean, not stubbed out mailer logic pointing to a mailer URL that doesn't actually run. (ie whatever on localhost, but you're not running it) Did you try that case out?

@Fang-
Copy link
Member

Fang- commented Aug 15, 2019

I'll go ahead and merge this regardless. Mailer being unreachable is a bit of an edge case, anyway. But I do think we should track down and fix whatever's causing that hang.

@Fang- Fang- merged commit 2bf1af9 into mino Aug 15, 2019
@Fang- Fang- deleted the feat/final-form branch August 15, 2019 18:11
@shrugs
Copy link
Contributor Author

shrugs commented Aug 15, 2019

This still gives me the hanging behavior for clean, not stubbed out mailer logic pointing to a mailer URL that doesn't actually run. (ie whatever on localhost, but you're not running it) Did you try that case out?

I didn't try that case, will take a peek on monday. perhaps an issue with the fetch code never timing out (maybe we need a Promise.race?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants