The Final Form PR

[shrugs]

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.

[Fang-]

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

[shrugs]

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]

(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-]

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]

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]

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

[shrugs]

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

[shrugs]

here's what that looks like

[Fang-]

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.

[Fang-]

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:

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]

ah yeah, form inputs need to be disabled post submission. good catch. the focus masking is on a separate branch, not on this one, so that's ok. I'm very down to go with that website's result. only design question is where the checkbox goes, so Jimmy would need to weigh in on that. can you ping him in person and get some feedback? then we can wrap this up in reasonable time. I've experienced the hanging on mobile devices but not yet on a desktop. you're using Firefox? how many emails were you generating, and how long did you give it before you killed the tab? perhaps browsers slow down problem threads, which may exacerbate the problem?

…

On Wed, Aug 14, 2019, 00:08 Fang ***@***.***> wrote: ***@***.**** requested changes on this pull request. [image: login] <https://user-images.githubusercontent.com/3829764/62980306-08860880-bddb-11e9-96ae-d82a1ba60ad9.gif> 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: image] <https://user-images.githubusercontent.com/3829764/62980575-d45f1780-bddb-11e9-9485-14aca15350f5.png> 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? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#221?email_source=notifications&email_token=AALWYGOL7OM76CHL26BTJJ3QEMWFHA5CNFSM4IIJ4GW2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBO43AI#pullrequestreview-274582913>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALWYGKR2ZFIN4FBHAA2KOLQEMWFHANCNFSM4IIJ4GWQ> .

[shrugs]

@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]

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

[Fang-]

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]

@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]

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

[Fang-]

@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]

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

[jyng]

Nice

[shrugs]

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]

@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-]

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-]

fwiw I can reproduce in chrome

[shrugs]

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]

ty for patience on that one

[shrugs]

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-]

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-]

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.

[shrugs]

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?)