Harvest: Facilitate trigger launching and two factor authentification with TriggerLauncher #415

gregorylegarec · 2019-05-01T11:13:55Z

This PR prepares the code for and add the TriggerLauncher component.

Using render props, it allows to easily launch a trigger and handle all steps of a konnector job lifecycle.

To launch a konnector job with a button:

render(){
  // trigger has already been fetched
  const { trigger } = this.props
  return (
    <TriggerLauncher trigger={trigger}>
      {({launch, running}) => (
        <Button
          onClick={launch}
          disabled={running}
        />
      )}
    </TriggerLauncher>

TriggerLauncher relies internally on a KonnectorJob object. KonnectorJob is an object which handles a konnector job lifecycle from launch to success or error. It use MicroEE mixins to emit events.

// trigger has already been fetched
const konnectorJob = new KonnectorJob(trigger)
konnectorJob
  .on('success', handleSuccess)
  .on('twoFARequest', handleTwoFARequest)
konnectorJob.launch()

KonnectorJob for now relies internally on existing classes KonnectorJobWatcher and KonnectorAccountWatcher, they should both be merged directly into KonnectorJob. But as they are also used in TriggerManager to handle 2FA, we prefer to not introduce regressions in this PR and keep this refactoring for a next PR. In this future PR TriggerManager will lose all logic related to 2FA, which it should not handle.

Tests for TriggerManager and KonnectorJob are coming in a future PR as well.

CPatchane

Looks good but we need much more tests for those new components/models

CPatchane · 2019-05-02T09:15:49Z

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

+
+export default withMutations(accountsMutations, triggersMutations)(
+  TriggerLauncher
+)


Could we have a test for this new component? At least a snapshot will be a good beginning than nothing

CPatchane · 2019-05-02T09:16:23Z

packages/cozy-harvest-lib/src/components/TwoFA/Modal.jsx

+  client: PropTypes.object
+}
+
+export default translate()(withMutations(accountsMutations)(TwoFAModal))


At least a snapshot would be great to avoid regressions when merging the two TwoFA components in the future

CPatchane · 2019-05-02T09:16:53Z

packages/cozy-harvest-lib/src/models/KonnectorJob.js

+
+MicroEE.mixin(KonnectorJob)
+
+export default KonnectorJob


Could we have a test for this model, since it will be the main used model, we have to have to start some tests here

In KonnectorJobWatcher, login success is no longer a success, to not block regular success to be triggered.

KonnectorJob is a model which encapsulate existing KonnectorJobWatcher and KonnectorAccountWatcher, aiming to merge them into it in a near future. For now we prefer to not introduce regressions into TriggerManager, event if it will also need a refactoring to use directly KonnectorJob. KonnectorJob manages trigger, account and job logic already existing into TriggerManager, which is not the place they belong to.

For now it is just a copy of TwoFAForm, to avoid introducing regression into TriggerManager. Both TwoFAForm and TwoFAModal will need to be merged.

ptbrowne

Quelques observations et retours non bloquants. Je suis un peu en questionnement quand au stockage du state du trigger dans le composant plutôt que dans l'objet KonnectorJob.

ptbrowne · 2019-05-06T11:43:22Z

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

+
+  render() {
+    const { showTwoFAModal, status } = this.state
+    const { account, children, konnector, submitting } = this.props


Nit: When using boolean variable, I tend to put has/is in front. isSubmitting.

I agree. As submitting is also used in TriggerManager, I suggest to keep it now for consistence and rename all props at the same time. I made an issue: #431.

@CPatchane account and konnector are data which can be retrieved with trigger infos. As konnectorJob is already fetching the related account, I suggest to also fetch the related konnector in it.

Then we pass the whole konnectorJob to TwoFAModal or we get account and konnector from it.

We should try to limit the number of props which can be deduced with other ones now, otherwise we will soon have a new Cozy-Home into Harvest. TriggerLauncher just expect a trigger as prop. For now it also take submitting but we will make it disappear in the future.

See fee5d47

ptbrowne · 2019-05-06T11:45:30Z

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

+      <div>
+        {children({
+          launch: this.launch,
+          running: ![ERRORED, IDLE, SUCCESS].includes(status) || submitting


Nit: I assume the triger does not have a status == running ? It seems a bit brittle because if we had another status, we will have running to true whereas the new status can be scheduled or something else

How I see it: If we introduce another status we should know it and add it in our model, which strengthen your suggestion to rely totally on konnectorJob to know get information about job status.

Handled directly in KonnectorJob now

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

ptbrowne · 2019-05-06T11:50:04Z

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

+            dismissAction={this.handleTwoFAModalDismiss}
+            handleSubmitTwoFACode={this.handle2FACode}
+            submitting={status === RUNNING_TWOFA}
+            retryAsked={status === TWO_FA_MISMATCH}


I am wondering if it would no be simpler for the TwoFaForm to get the job and listen by itself on it. Here we are adding external props to control the TwoFAForm whereas if it had the job it could listen to it by itself.

I was asking myself the same kind of question during development. What made me choose this solutions was to avoid passing a complex object like a KonnectorJob as prop. But I am still not sure this was a good solution.

How I see it if we pass the konnectorJob to TwoFAModal:

TriggerLauncher must still listen to twoFARequest event to show 2FA modal

TwoFAModal only listen to twoFAMismatch event

Do you agree ?

Now the TriggerLauncher just use KonnectorJob from props with fee5d47

ptbrowne · 2019-05-06T12:01:08Z

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

+      .on('success', this.handleSuccess)
+      .on('twoFARequest', this.handleTwoFARequest)
+      .on('twoFAMismatch', this.handleTwoFAMismatch)
+    this.konnectorJob.launch()


Observations:

In launch we are assigning this.konnectorJob, this means that TriggerLauncher has an internal state and it is not cleaned up on success or error. Do we need to save an internal variable ? Could we simplify and only use a closure ?

The component is responsible to remember the status of the konnector in its internal state. I wonder if it would no be simpler to have this status in the konnectorJob and rely on it for the render.

Seems clever. The konnectorJob is saved as an interanl variable to have access to konnectorJob.sendTwoFACode method to pass it to TwoFAModal.

Keeping the status in konnectorJob could also spare us all the handle{KonnectorJobEvent} methods here.

Agree about the status but we still need the internal variable for the sendTwoFACode prop passed to the form, I don't see another simpler way to handle it :/

See fee5d47

gregorylegarec · 2019-05-06T13:13:41Z

@ptbrowne Thanks for your feedback 👍

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx

Temporary duplicate TwoFAForm into TwoFAModal to avoid regressions at term, TriggerManager will use TwoFAModal

gregorylegarec · 2019-05-07T13:41:52Z

Thanks @CPatchane 👍

gregorylegarec requested a review from CPatchane as a code owner May 1, 2019 11:13

gregorylegarec self-assigned this May 1, 2019

gregorylegarec added the ✨ Enhancement New feature or request label May 1, 2019

gregorylegarec force-pushed the feat/harvest/TriggerLauncher branch 3 times, most recently from db0cb38 to 0a67664 Compare May 1, 2019 23:13

CPatchane approved these changes May 2, 2019

View reviewed changes

gregorylegarec and others added 11 commits May 6, 2019 12:12

refactor(harvest): Use MicroEE for KonnectorJobWatcher events 🛠

2ff1850

refactor(harvest): Create HOC to export components whit locales 🛠

3fb99f8

feat(harvest): Better success handling 📝"

a612edc

In KonnectorJobWatcher, login success is no longer a success, to not block regular success to be triggered.

feat(harvest): Enable success timer sooner 📝

f4598c2

feat(harvest): Provide getAccountId from triggers helper 📝

833d210

feat(harvest): Expose findAccount from accounts connections 📝

9a954c0

feat(harvest): Creates TwoFAModal ✨

0f30174

For now it is just a copy of TwoFAForm, to avoid introducing regression into TriggerManager. Both TwoFAForm and TwoFAModal will need to be merged.

feat(harvest): Add Trigger Launcher ✨

9d368a8

fix(harvest): Use the same 2FA Modal for Trigger Manager/Launcher

66646a8

fix(harvest): Handle Two FA code retries correctly

db6c561

CPatchane force-pushed the feat/harvest/TriggerLauncher branch from 24906ec to db6c561 Compare May 6, 2019 10:13

fix(harvest): ✏️ Minor wording change

48fd910

CPatchane force-pushed the feat/harvest/TriggerLauncher branch from d1a005d to 48fd910 Compare May 6, 2019 11:16

ptbrowne reviewed May 6, 2019

View reviewed changes

gregorylegarec commented May 6, 2019

View reviewed changes

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx Outdated Show resolved Hide resolved

gregorylegarec commented May 6, 2019

View reviewed changes

packages/cozy-harvest-lib/src/components/TriggerLauncher.jsx Outdated Show resolved Hide resolved

CPatchane mentioned this pull request May 7, 2019

Add banners for errors cozy/cozy-home#1213

Merged

feat(harvest): 💅 Let KonnectorJob handle statuses

addbecd

CPatchane force-pushed the feat/harvest/TriggerLauncher branch from fee5d47 to addbecd Compare May 7, 2019 11:58

CPatchane added 2 commits May 7, 2019 14:20

fix(harvest): 💅 Pass KonnectorJob directly to TwoFAModal

925fed8

Temporary duplicate TwoFAForm into TwoFAModal to avoid regressions at term, TriggerManager will use TwoFAModal

fix(harvest): 💅 Split withKonnectorJob from the model

ac75302

gregorylegarec merged commit 4b0ffe0 into cozy:master May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harvest: Facilitate trigger launching and two factor authentification with TriggerLauncher #415

Harvest: Facilitate trigger launching and two factor authentification with TriggerLauncher #415

gregorylegarec commented May 1, 2019 •

edited

Loading

CPatchane left a comment

CPatchane May 2, 2019

CPatchane May 2, 2019

CPatchane May 2, 2019

ptbrowne left a comment

ptbrowne May 6, 2019

gregorylegarec May 6, 2019

gregorylegarec May 6, 2019 •

edited

Loading

CPatchane May 7, 2019

ptbrowne May 6, 2019

gregorylegarec May 6, 2019

CPatchane May 7, 2019

ptbrowne May 6, 2019

gregorylegarec May 6, 2019

CPatchane May 7, 2019

ptbrowne May 6, 2019

gregorylegarec May 6, 2019

CPatchane May 7, 2019

CPatchane May 7, 2019

gregorylegarec commented May 6, 2019

gregorylegarec commented May 7, 2019

Harvest: Facilitate trigger launching and two factor authentification with TriggerLauncher #415

Harvest: Facilitate trigger launching and two factor authentification with TriggerLauncher #415

Conversation

gregorylegarec commented May 1, 2019 • edited Loading

CPatchane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptbrowne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorylegarec May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorylegarec commented May 6, 2019

gregorylegarec commented May 7, 2019

gregorylegarec commented May 1, 2019 •

edited

Loading

gregorylegarec May 6, 2019 •

edited

Loading