[#161224146] Refactor light identification #658

fpersico · 2018-10-15T08:55:54Z

In the PR:

A IdentificationModal that allow the user to do a light identification. The modal is rendered by the RootContainer and outside the navigation. For the moment the identification is only by PIN. When going in background the identification is started by the react-native-background-timer module so we don't need the BackgroundScreen anymore.

The identification process can be started:

Calling a saga method directly and synchronously waiting for the identification result: this method is used by the startup saga
Dispatching and action: in this case you can pass Actions that the internal process will dispatch on success and cancel.

WIP:

Test if background-timer fire on iOS.
Attach the new identification to the payment process (already done locally, need to review the code after the latest payment refactor)
Remove old unused code/files

digitalcitizenship · 2018-10-15T08:58:29Z

Affected stories

🌟 #161224146: Light identification refactoring

New dependencies added: react-native-background-timer and @types/react-native-background-timer.

react-native-background-timer

Author: David Ocetnik

Description: Emit event periodically (even when app is in the background)

Homepage: https://github.com/ocetnik/react-native-background-timer#readme


Created	over 2 years ago
Last Updated	about 2 months ago
License	MIT
Maintainers	1
Releases	23
Keywords	android, ios, react-component, react-native and timer

@types/react-native-background-timer

Author: Unknown

Description: TypeScript definitions for react-native-background-timer

Homepage: http://npmjs.com/package/@types/react-native-background-timer


Created	4 months ago
Last Updated	6 days ago
License	MIT
Maintainers	1
Releases	1
Direct Dependencies

README

Installation

npm install --save @types/react-native-background-timer

Summary

This package contains type definitions for react-native-background-timer (https://github.com/ocetnik/react-native-background-timer#readme).

Details

Files were exported from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-native-background-timer

Additional Details

Last updated: Tue, 26 Jun 2018 01:10:51 GMT
Dependencies: none
Global values: none

Credits

These definitions were written by Tjark Smalla https://github.com/chillkroeteTTS.

Generated by 🚫 dangerJS

cloudify · 2018-10-15T11:02:15Z

ts/IdentificationModal.tsx

+import { IdentificationState } from "./store/reducers/identification";
+import { GlobalState } from "./store/reducers/types";
+
+type MapStateToProps = {


we call this ReduxMappedStateProps in the rest of the codebase

cloudify · 2018-10-15T11:04:58Z

ts/IdentificationModal.tsx

+import { GlobalState } from "./store/reducers/types";
+
+type MapStateToProps = {
+  identification: IdentificationState;


identificationState ?

cloudify · 2018-10-15T11:05:32Z

ts/IdentificationModal.tsx

+  return null;
+};
+
+class IdentificationModal extends React.PureComponent<Props, State> {


add a comment describing the purpose of this component

cloudify · 2018-10-15T11:06:52Z

ts/IdentificationModal.tsx

+
+type Props = MapStateToProps & ReduxProps;
+
+type IdentificationByPinState = "unstarted" | "failure";


I don't understand the difference between IdentificationState and IdentificationByPinState, could you add some comments?

Yes going to add comments. For the moment we have only PIN identification but in future we'll have also other identification methods (like fingerprint). IdentificationState is to store the global user identification. IdentificationByPinState is a local state to check the result of the matching in Pinpad component.

cloudify · 2018-10-15T11:12:46Z

ts/IdentificationModal.tsx

+      };
+
+      return (
+        <Modal


why do we need a modal? can't this be just a screen rendered on top of everything?

A screen? You mean a react-navigation screen or something else? The purpose is to leave the AppNavigator untouched and just display something that force the user to identify.

I mean something simple like ConnectionBar or VersionInfoOverlay

cloudify · 2018-10-15T11:31:45Z

ts/sagas/startup/watchApplicationActivitySaga.ts

-      //        the app we don't ask a PIN
-      yield put(navigateToBackgroundScreen);
+      // Start the background timer
+      identificationBackgroundTimer = yield fork(


it doesn't look like startIdentificationBackgroundTimer returns anything

The return of "fork" effect is a redux-saga Task you can cancel.

cloudify · 2018-10-15T11:32:32Z

ts/sagas/startup/watchApplicationActivitySaga.ts

  });
 }
+
+function* startIdentificationBackgroundTimer(


this function is only two instructions and it's only called once, why not inline it?

cloudify · 2018-10-15T11:37:57Z

ts/utils/timer.ts

+import BackgroundTimer from "react-native-background-timer";
+
+export function startTimer(t: number): Promise<never> {
+  // tslint:disable-next-line:promise-must-complete


why tslint was complaining here?

It seem related to microsoft/tslint-microsoft-contrib#329

cloudify · 2018-10-15T11:41:04Z

ts/components/Pinpad/index.tsx

@@ -19,9 +19,10 @@ const blurElement = (el: TextInput) => el.blur();
 const current = (ref: React.RefObject<TextInput>) => ref.current;

 interface Props {
+  activeColor: string;
+  clearOnInvalid?: boolean;


it seems that now PinPad is used only in one place and clearOnInvalid is set to true, why we need it to be configurable?

I have to check but isn't the Pinpad also used during the Onboarding?

that's right!

cloudify · 2018-10-15T11:42:10Z

ts/sagas/identification.ts

+  }
+
+  // Identification was success return true
+  return IdentificationResult.success;


I would not default to success here, wrap it with an if as well

fpersico requested a review from cloudify October 15, 2018 08:55

cloudify suggested changes Oct 15, 2018

View reviewed changes

fpersico force-pushed the light-identification branch from fa8a06d to cf23a70 Compare October 15, 2018 14:44

fpersico changed the title ~~[WIP] Refactor light identification~~ [#161224146] Refactor light identification Oct 15, 2018

francescopersico added 6 commits October 15, 2018 18:17

Refactor light identification

84ecb48

Add payment integration

848ef78

Improve iOS integration

78a723f

Add code fix and improvements

422e79a

Use a normal view instead of a modal for the identification overlay

c1a9d95

Handle identification results that need to reinizialize the application

9616734

fpersico force-pushed the light-identification branch from c9b17d1 to 9616734 Compare October 15, 2018 16:21

cloudify approved these changes Oct 16, 2018

View reviewed changes

Merge branch 'master' into light-identification

fa4523d

fpersico merged commit 369221e into master Oct 16, 2018

fabriziofff deleted the light-identification branch January 21, 2022 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#161224146] Refactor light identification #658

[#161224146] Refactor light identification #658

fpersico commented Oct 15, 2018 •

edited

Loading

digitalcitizenship commented Oct 15, 2018 •

edited

Loading

Installation

Summary

Details

Credits

cloudify Oct 15, 2018

cloudify Oct 15, 2018

cloudify Oct 15, 2018

cloudify Oct 15, 2018

fpersico Oct 15, 2018

cloudify Oct 15, 2018

fpersico Oct 15, 2018

cloudify Oct 15, 2018 •

edited

Loading

cloudify Oct 15, 2018

fpersico Oct 15, 2018

cloudify Oct 15, 2018

cloudify Oct 15, 2018

fpersico Oct 15, 2018

cloudify Oct 15, 2018

fpersico Oct 15, 2018

cloudify Oct 15, 2018

cloudify Oct 15, 2018


		type Props = MapStateToProps & ReduxProps;

		type IdentificationByPinState = "unstarted" \| "failure";

[#161224146] Refactor light identification #658

[#161224146] Refactor light identification #658

Conversation

fpersico commented Oct 15, 2018 • edited Loading

digitalcitizenship commented Oct 15, 2018 • edited Loading

Affected stories

Installation

Summary

Details

Credits

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

cloudify Oct 15, 2018 • 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

fpersico commented Oct 15, 2018 •

edited

Loading

digitalcitizenship commented Oct 15, 2018 •

edited

Loading

cloudify Oct 15, 2018 •

edited

Loading