From e1c0b486107ba6dff3df1e1e1f41aeb590f66114 Mon Sep 17 00:00:00 2001 From: CrisTofani Date: Mon, 8 Feb 2021 11:12:54 +0100 Subject: [PATCH 1/5] [#176825286] Clean usage of availableBonusSelector with memoized one --- .../screens/AvailableBonusScreen.tsx | 20 ++++++++----------- .../store/reducers/availableBonusesTypes.ts | 17 +++++++++++++++- .../BpdCTAStartOnboardingScreen.tsx | 16 +++++++-------- .../wallet/component/FeaturedCardCarousel.tsx | 7 +++---- ts/screens/wallet/WalletHomeScreen.tsx | 5 ++--- 5 files changed, 37 insertions(+), 28 deletions(-) diff --git a/ts/features/bonus/bonusVacanze/screens/AvailableBonusScreen.tsx b/ts/features/bonus/bonusVacanze/screens/AvailableBonusScreen.tsx index 89b2e59d0ba..98d007c0ec0 100644 --- a/ts/features/bonus/bonusVacanze/screens/AvailableBonusScreen.tsx +++ b/ts/features/bonus/bonusVacanze/screens/AvailableBonusScreen.tsx @@ -1,4 +1,3 @@ -import * as pot from "italia-ts-commons/lib/pot"; import { Content, View } from "native-base"; import * as React from "react"; import { @@ -33,7 +32,8 @@ import { bonusVacanzeStyle } from "../components/Styles"; import { navigateToBonusRequestInformation } from "../navigation/action"; import { loadAvailableBonuses } from "../store/actions/bonusVacanze"; import { - availableBonusTypesSelector, + isAvailableBonusNoneErrorSelector, + isAvailableBonusLoadingSelector, visibleAvailableBonusSelector } from "../store/reducers/availableBonusesTypes"; import { @@ -189,16 +189,12 @@ class AvailableBonusScreen extends React.PureComponent { } } -const mapStateToProps = (state: GlobalState) => { - const potAvailableBonuses = availableBonusTypesSelector(state); - return { - // fallback to hardcode data if pot is none - availableBonusesList: visibleAvailableBonusSelector(state), - isLoading: pot.isLoading(potAvailableBonuses), - // show error only when we have an error and no data to show - isError: pot.isNone(potAvailableBonuses) && pot.isError(potAvailableBonuses) - }; -}; +const mapStateToProps = (state: GlobalState) => ({ + availableBonusesList: visibleAvailableBonusSelector(state), + isLoading: isAvailableBonusLoadingSelector(state), + // show error only when we have an error and no data to show + isError: isAvailableBonusNoneErrorSelector(state) +}); const mapDispatchToProps = (dispatch: Dispatch) => ({ navigateBack: () => dispatch(navigateBack()), diff --git a/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts b/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts index 6c3c49de0b9..6d5c9173860 100644 --- a/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts +++ b/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts @@ -49,7 +49,7 @@ const reducer = ( }; // Selectors -export const availableBonusTypesSelector = ( +const availableBonusTypesSelector = ( state: GlobalState ): AvailableBonusTypesState => state.bonus.availableBonusTypes; @@ -80,6 +80,21 @@ export const visibleAvailableBonusSelector = createSelector( ) ); +export const isAvailableBonusLoadingSelector = createSelector( + availableBonusTypesSelector, + (abs: AvailableBonusTypesState) => pot.isLoading(abs) +); + +export const isAvailableBonusNoneErrorSelector = createSelector( + availableBonusTypesSelector, + (abs: AvailableBonusTypesState) => pot.isNone(abs) && pot.isError(abs) +); + +export const isAvailableBonusErrorSelector = createSelector( + availableBonusTypesSelector, + (abs: AvailableBonusTypesState) => pot.isError(abs) +); + /** * return the bonus type corresponding to the given idBonusType * @param idBonusType diff --git a/ts/features/bonus/bpd/screens/onboarding/BpdCTAStartOnboardingScreen.tsx b/ts/features/bonus/bpd/screens/onboarding/BpdCTAStartOnboardingScreen.tsx index f2aec11b10c..753eb09f995 100644 --- a/ts/features/bonus/bpd/screens/onboarding/BpdCTAStartOnboardingScreen.tsx +++ b/ts/features/bonus/bpd/screens/onboarding/BpdCTAStartOnboardingScreen.tsx @@ -1,15 +1,16 @@ import * as React from "react"; import { Dispatch } from "redux"; import { connect } from "react-redux"; -import * as pot from "italia-ts-commons/lib/pot"; import { bpdOnboardingStart } from "../../store/actions/onboarding"; import { LoadingErrorComponent } from "../../../bonusVacanze/components/loadingErrorScreen/LoadingErrorComponent"; import I18n from "../../../../../i18n"; import { loadAvailableBonuses } from "../../../bonusVacanze/store/actions/bonusVacanze"; import { GlobalState } from "../../../../../store/reducers/types"; -import { availableBonusTypesSelector } from "../../../bonusVacanze/store/reducers/availableBonusesTypes"; +import { + isAvailableBonusErrorSelector, + visibleAvailableBonusSelector +} from "../../../bonusVacanze/store/reducers/availableBonusesTypes"; import { useActionOnFocus } from "../../../../../utils/hooks/useOnFocus"; -import { isStrictSome } from "../../../../../utils/pot"; import BaseScreenComponent from "../../../../../components/screens/BaseScreenComponent"; export type Props = ReturnType & @@ -20,14 +21,12 @@ const loadingCaption = () => I18n.t("global.remoteStates.loading"); * this is a dummy screen reachable only from a message CTA */ const BpdCTAStartOnboardingScreen: React.FC = (props: Props) => { - const hasError = () => pot.isError(props.availableBonus); - // load available bonus when component is focused useActionOnFocus(props.loadAvailableBonus); React.useEffect(() => { // bpdOnboardingStart navigate to ToS screen that needs availableBonus data - if (isStrictSome(props.availableBonus)) { + if (props.availableBonus.length > 0) { props.startBpd(); } }, [props.availableBonus]); @@ -35,7 +34,7 @@ const BpdCTAStartOnboardingScreen: React.FC = (props: Props) => { return ( @@ -51,7 +50,8 @@ const mapDispatchToProps = (dispatch: Dispatch) => ({ }); const mapStateToProps = (globalState: GlobalState) => ({ - availableBonus: availableBonusTypesSelector(globalState) + availableBonus: visibleAvailableBonusSelector(globalState), + hasError: isAvailableBonusErrorSelector(globalState) }); export default connect( diff --git a/ts/features/wallet/component/FeaturedCardCarousel.tsx b/ts/features/wallet/component/FeaturedCardCarousel.tsx index c6fa2cf5018..977266da04e 100644 --- a/ts/features/wallet/component/FeaturedCardCarousel.tsx +++ b/ts/features/wallet/component/FeaturedCardCarousel.tsx @@ -13,7 +13,7 @@ import { IOStyles } from "../../../components/core/variables/IOStyles"; import I18n from "../../../i18n"; import { Dispatch } from "../../../store/actions/types"; import { GlobalState } from "../../../store/reducers/types"; -import { availableBonusTypesSelector } from "../../bonus/bonusVacanze/store/reducers/availableBonusesTypes"; +import { visibleAvailableBonusSelector } from "../../bonus/bonusVacanze/store/reducers/availableBonusesTypes"; import { ID_BPD_TYPE, ID_CGN_TYPE } from "../../bonus/bonusVacanze/utils/bonus"; import { bpdOnboardingStart } from "../../bonus/bpd/store/actions/onboarding"; import { bpdEnabledSelector } from "../../bonus/bpd/store/reducers/details/activation"; @@ -94,8 +94,7 @@ const FeaturedCardCarousel: React.FunctionComponent = (props: Props) => { ); case ID_CGN_TYPE: return ( - !props.cgnActiveBonus && - cgnEnabled && ( + !props.cgnActiveBonus && ( ({ bpdActiveBonus: bpdEnabledSelector(state), // FIXME replace with Selector when the API implementation is completed. cgnActiveBonus: false, - availableBonusesList: pot.getOrElse(availableBonusTypesSelector(state), []) + availableBonusesList: visibleAvailableBonusSelector(state) }); const mapDispatchToProps = (dispatch: Dispatch) => ({ diff --git a/ts/screens/wallet/WalletHomeScreen.tsx b/ts/screens/wallet/WalletHomeScreen.tsx index b5b8af2f63f..ff42cff2ae2 100644 --- a/ts/screens/wallet/WalletHomeScreen.tsx +++ b/ts/screens/wallet/WalletHomeScreen.tsx @@ -38,7 +38,7 @@ import { loadAvailableBonuses } from "../../features/bonus/bonusVacanze/store/actions/bonusVacanze"; import { allBonusActiveSelector } from "../../features/bonus/bonusVacanze/store/reducers/allActive"; -import { availableBonusTypesSelector } from "../../features/bonus/bonusVacanze/store/reducers/availableBonusesTypes"; +import { visibleAvailableBonusSelector } from "../../features/bonus/bonusVacanze/store/reducers/availableBonusesTypes"; import BpdCardsInWalletContainer from "../../features/bonus/bpd/components/walletCardContainer/BpdCardsInWalletComponent"; import { bpdAllData } from "../../features/bonus/bpd/store/actions/details"; import { bpdPeriodsAmountWalletVisibleSelector } from "../../features/bonus/bpd/store/reducers/details/combiner"; @@ -576,11 +576,10 @@ const mapStateToProps = (state: GlobalState) => { .map(si => !isUpdateNeeded(si, "min_app_version_pagopa")) .getOrElse(true); - const potAvailableBonuses = availableBonusTypesSelector(state); return { periodsWithAmount: bpdPeriodsAmountWalletVisibleSelector(state), allActiveBonus: allBonusActiveSelector(state), - availableBonusesList: pot.getOrElse(potAvailableBonuses, []), + availableBonusesList: visibleAvailableBonusSelector(state), potWallets: pagoPaCreditCardWalletV1Selector(state), anyHistoryPayments: paymentsHistorySelector(state).length > 0, anyCreditCardAttempts: creditCardAttemptionsSelector(state).length > 0, From 1200c55e5239252b7e08bd61ba89388f2dc64ce5 Mon Sep 17 00:00:00 2001 From: CrisTofani Date: Thu, 11 Feb 2021 19:23:53 +0100 Subject: [PATCH 2/5] [#176825286] Fix carousel items rendering logic --- .../store/reducers/availableBonusesTypes.ts | 15 ++++++++++----- .../wallet/component/FeaturedCardCarousel.tsx | 11 +++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts b/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts index 6d5c9173860..0e9bd64506a 100644 --- a/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts +++ b/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts @@ -80,19 +80,24 @@ export const visibleAvailableBonusSelector = createSelector( ) ); +// Returns true if information about Available Bonuses list is loading export const isAvailableBonusLoadingSelector = createSelector( availableBonusTypesSelector, (abs: AvailableBonusTypesState) => pot.isLoading(abs) ); -export const isAvailableBonusNoneErrorSelector = createSelector( +// Returns true if information about Available Bonuses list is in error +export const isAvailableBonusErrorSelector = createSelector( availableBonusTypesSelector, - (abs: AvailableBonusTypesState) => pot.isNone(abs) && pot.isError(abs) + (abs: AvailableBonusTypesState): boolean => pot.isError(abs) ); -export const isAvailableBonusErrorSelector = createSelector( - availableBonusTypesSelector, - (abs: AvailableBonusTypesState) => pot.isError(abs) +// Returns true if information about Available Bonuses list +// is in error state and no data is available in list (NoneError type) +export const isAvailableBonusNoneErrorSelector = createSelector( + [availableBonusTypesSelector, isAvailableBonusErrorSelector], + (abs: AvailableBonusTypesState, hasError: boolean): boolean => + hasError && pot.isNone(abs) ); /** diff --git a/ts/features/wallet/component/FeaturedCardCarousel.tsx b/ts/features/wallet/component/FeaturedCardCarousel.tsx index 977266da04e..286e6787c61 100644 --- a/ts/features/wallet/component/FeaturedCardCarousel.tsx +++ b/ts/features/wallet/component/FeaturedCardCarousel.tsx @@ -52,9 +52,12 @@ const FeaturedCardCarousel: React.FunctionComponent = (props: Props) => { }); } - const anyBonusNotActive = - (!pot.getOrElse(props.bpdActiveBonus, false) && bpdEnabled) || - (!props.cgnActiveBonus && cgnEnabled); + const hasBpdActive: boolean | undefined = pot.getOrElse( + props.bpdActiveBonus, + false + ); + const anyBonusNotActive = hasBpdActive === false || !props.cgnActiveBonus; + return anyBonusNotActive ? ( @@ -82,7 +85,7 @@ const FeaturedCardCarousel: React.FunctionComponent = (props: Props) => { switch (b.id_type) { case ID_BPD_TYPE: return ( - !pot.getOrElse(props.bpdActiveBonus, false) && ( + hasBpdActive === false && ( Date: Fri, 12 Feb 2021 10:26:56 +0100 Subject: [PATCH 3/5] [#176825286] add return type --- .../bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts b/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts index 0e9bd64506a..d486f1d7db1 100644 --- a/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts +++ b/ts/features/bonus/bonusVacanze/store/reducers/availableBonusesTypes.ts @@ -83,7 +83,7 @@ export const visibleAvailableBonusSelector = createSelector( // Returns true if information about Available Bonuses list is loading export const isAvailableBonusLoadingSelector = createSelector( availableBonusTypesSelector, - (abs: AvailableBonusTypesState) => pot.isLoading(abs) + (abs: AvailableBonusTypesState): boolean => pot.isLoading(abs) ); // Returns true if information about Available Bonuses list is in error From cf3d311a29544951bf217cd4b2da6382b9a41f39 Mon Sep 17 00:00:00 2001 From: CrisTofani Date: Fri, 12 Feb 2021 11:07:00 +0100 Subject: [PATCH 4/5] [#176825286] Fixes check on BpdActive bonus --- ts/features/wallet/component/FeaturedCardCarousel.tsx | 10 +++++----- ts/utils/pot.ts | 9 ++++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/ts/features/wallet/component/FeaturedCardCarousel.tsx b/ts/features/wallet/component/FeaturedCardCarousel.tsx index 286e6787c61..82457c456a7 100644 --- a/ts/features/wallet/component/FeaturedCardCarousel.tsx +++ b/ts/features/wallet/component/FeaturedCardCarousel.tsx @@ -1,7 +1,6 @@ import { reverse } from "fp-ts/lib/Array"; import { constUndefined } from "fp-ts/lib/function"; import { fromNullable } from "fp-ts/lib/Option"; -import * as pot from "italia-ts-commons/lib/pot"; import { View } from "native-base"; import * as React from "react"; import { ScrollView, StyleSheet } from "react-native"; @@ -20,6 +19,7 @@ import { bpdEnabledSelector } from "../../bonus/bpd/store/reducers/details/activ import { getLocalePrimaryWithFallback } from "../../../utils/locale"; import { cgnActivationStart } from "../../bonus/cgn/store/actions/activation"; import { bpdEnabled, cgnEnabled } from "../../../config"; +import { isStrictSome } from "../../../utils/pot"; import FeaturedCard from "./FeaturedCard"; type Props = ReturnType & @@ -52,10 +52,10 @@ const FeaturedCardCarousel: React.FunctionComponent = (props: Props) => { }); } - const hasBpdActive: boolean | undefined = pot.getOrElse( - props.bpdActiveBonus, - false - ); + const hasBpdActive: boolean | undefined = isStrictSome(props.bpdActiveBonus) + ? props.bpdActiveBonus.value + : undefined; + const anyBonusNotActive = hasBpdActive === false || !props.cgnActiveBonus; return anyBonusNotActive ? ( diff --git a/ts/utils/pot.ts b/ts/utils/pot.ts index 049c96544ed..2128231c130 100644 --- a/ts/utils/pot.ts +++ b/ts/utils/pot.ts @@ -1,4 +1,11 @@ import * as pot from "italia-ts-commons/lib/pot"; +// type alias of pot.Some to make possible type guard, since pot.Some is not exported +interface Some { + readonly kind: "PotSome"; + readonly value: T; +} + // return true if pot is some and not someError and not someLoading -export const isStrictSome = (p: pot.Pot) => p.kind === "PotSome"; +export const isStrictSome = (p: pot.Pot): p is Some => + p.kind === "PotSome"; From 7a8beb030347fceb1bb3443fb162ae223083b325 Mon Sep 17 00:00:00 2001 From: Matteo Boschi Date: Fri, 12 Feb 2021 12:11:31 +0100 Subject: [PATCH 5/5] [#176825286] add tests --- .../__tests__/availableBonusesTypes.test.ts | 81 ++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-) diff --git a/ts/features/bonus/bonusVacanze/store/reducers/__tests__/availableBonusesTypes.test.ts b/ts/features/bonus/bonusVacanze/store/reducers/__tests__/availableBonusesTypes.test.ts index 07cb8dbf200..e2ff2d508fc 100644 --- a/ts/features/bonus/bonusVacanze/store/reducers/__tests__/availableBonusesTypes.test.ts +++ b/ts/features/bonus/bonusVacanze/store/reducers/__tests__/availableBonusesTypes.test.ts @@ -1,5 +1,11 @@ import * as pot from "italia-ts-commons/lib/pot"; -import { visibleAvailableBonusSelector } from "../availableBonusesTypes"; +import { + AvailableBonusTypesState, + isAvailableBonusErrorSelector, + isAvailableBonusLoadingSelector, + isAvailableBonusNoneErrorSelector, + visibleAvailableBonusSelector +} from "../availableBonusesTypes"; import { availableBonuses, contentBonusVacanzeIT @@ -11,6 +17,7 @@ import { ID_BPD_TYPE, ID_CGN_TYPE } from "../../../utils/bonus"; +import { BonusAvailable } from "../../../../../../../definitions/content/BonusAvailable"; const bonusMockContent = { name: "Bonus Vacanze", @@ -22,7 +29,7 @@ const bonusMockContent = { tos_url: "https://io.italia.it/app-content/bonus_vacanze_tos.html" }; -const mockBonus = { +const mockBonus: BonusAvailable = { id_type: 4, it: bonusMockContent, en: bonusMockContent, @@ -124,3 +131,73 @@ describe("availableBonusesTypes with FF enabled", () => { expect(result).toEqual([bonuses[0], bonuses[2]]); }); }); + +const loadingCases: ReadonlyArray<[ + input: AvailableBonusTypesState, + expectedResult: boolean +]> = [ + [pot.noneLoading, true], + [pot.none, false], + [pot.some([mockBonus]), false], + [pot.noneError(new Error()), false], + [pot.someError([mockBonus], new Error()), false] +]; + +describe("isAvailableBonusLoadingSelector selector", () => { + test.each(loadingCases)( + "given %p as argument, returns %p", + (firstArg, expectedResult) => { + const result = isAvailableBonusLoadingSelector.resultFunc(firstArg); + expect(result).toEqual(expectedResult); + } + ); +}); + +const errorCases: ReadonlyArray<[ + input: AvailableBonusTypesState, + expectedResult: boolean +]> = [ + [pot.noneLoading, false], + [pot.none, false], + [pot.some([mockBonus]), false], + [pot.noneError(new Error()), true], + [pot.someError([mockBonus], new Error()), true] +]; +describe("isAvailableBonusErrorSelector selector", () => { + test.each(errorCases)( + "given %p as argument, returns %p", + (firstArg, expectedResult) => { + const result = isAvailableBonusErrorSelector.resultFunc(firstArg); + expect(result).toEqual(expectedResult); + } + ); +}); + +const noneErrorCases: ReadonlyArray<[ + firstInput: AvailableBonusTypesState, + secondInput: boolean, + expectedResult: boolean +]> = [ + [pot.noneLoading, true, true], + [pot.noneLoading, false, false], + [pot.none, true, true], + [pot.none, false, false], + [pot.some([mockBonus]), true, false], + [pot.some([mockBonus]), false, false], + [pot.noneError(new Error()), true, true], + [pot.noneError(new Error()), false, false], + [pot.someError([mockBonus], new Error()), true, false], + [pot.someError([mockBonus], new Error()), false, false] +]; +describe("isAvailableBonusNoneErrorSelector selector", () => { + test.each(noneErrorCases)( + "given %p,%p as argument, returns %p", + (firstArg, secondArg, expectedResult) => { + const result = isAvailableBonusNoneErrorSelector.resultFunc( + firstArg, + secondArg + ); + expect(result).toEqual(expectedResult); + } + ); +});