search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed luacmartins closed 1 year ago

luacmartins commented 1 year ago

Problem

Coming from Firebase Crashlytics, there are several non-fatal exceptions for Text strings must be rendered within a <Text> component. in v1.2.91. We should fix these.

Why is this important

Guarantees App stability

Solution

Find a way to reproduce the exception and prevent text nodes from being rendered outside of a <Text> component. I suspect that the exception is coming from this line, although I couldn't reproduce the issue.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ef3651f57da6d5c9
  • Upwork Job ID: 1641170623381450752
  • Last Price Increase: 2023-03-29
MelvinBot commented 1 year ago

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01ef3651f57da6d5c9

MelvinBot commented 1 year ago

Current assignee @miljakljajic is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

MelvinBot commented 1 year ago

Current assignee @luacmartins is eligible for the External assigner, not assigning anyone new.

allroundexperts commented 1 year ago

@luacmartins Is it possible to share the Crashlytics logs?

luacmartins commented 1 year ago

Stack trace:

Non-fatal Exception: io.invertase.firebase.crashlytics.JavaScriptError: Text strings must be rendered within a <Text> component.
       at .completeWork(address at index.android.bundle:1:182980)
       at .completeUnitOfWork(address at index.android.bundle:1:197038)
       at .performUnitOfWork(address at index.android.bundle:1:196882)
       at .workLoopSync(address at index.android.bundle:1:196703)
       at .renderRootSync(address at index.android.bundle:1:196587)
       at .performSyncWorkOnRoot(address at index.android.bundle:1:194006)
       at .flushSyncCallbacks(address at index.android.bundle:1:158416)
       at .batchedUpdatesImpl(address at index.android.bundle:1:210783)
       at .batchedUpdates(address at index.android.bundle:1:151205)
       at ._receiveRootNodeIDEvent(address at index.android.bundle:1:151478)
       at .receiveEvent(address at index.android.bundle:1:205242)
       at .apply((native):0:0)
       at .__callFunction(address at index.android.bundle:1:133673)
       at .anonymous(address at index.android.bundle:1:132077)
       at .__guard(address at index.android.bundle:1:133030)
       at .callFunctionReturnFlushedQueue(address at index.android.bundle:1:132035)

Logs:

errorInfo: {"componentStack":"\n    in RCTView\n    in Unknown\n    in ImageView\n    in Unknown\n    in AttachmentView\n    in Unknown\n    in RCTView\n    in Unknown\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in Carousel\n    in RCTView\n    in Unknown\n    in AttachmentCarousel\n    in withOnyx(AttachmentCarousel)\n    in Unknown\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in KeyboardAvoidingView\n    in KeyboardAvoidingView\n    in RCTView\n    in Unknown\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in withAnimatable(View)\n    in RCTView\n    in Unknown\n    in VirtualizedListContextResetter\n    in RCTModalHostView\n    in Modal\n    in ReactNativeModal\n    in BaseModal\n    in Unknown\n    in Modal\n    in Unknown\n    in AttachmentModal\n    in Unknown\n    in Unknown\n    in ImageRenderer\n    in MemoizedTNodeRenderer\n    in TNodeChildrenRenderer\n    in RCTView\n    in Unknown\n    in MemoizedTNodeRenderer\n    in TNodeChildrenRenderer\n    in RCTView\n    in Unknown\n    in MemoizedTNodeRenderer\n    in TNodeChildrenRenderer\n    in RCTView\n    in Unknown\n    in MemoizedTNodeRenderer\n    in Unknown\n    in RenderTTree\n    in SourceLoaderInline\n    in RawSourceLoader\n    in RenderHtmlSource\n    in RenderHTML\n    in ReportActionItemFragment\n    in Unknown\n    in Unknown\n    in RCTView\n    in Unknown\n    in ReportActionItemMessage\n    in Unknown\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in ReportActionItemGrouped\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in OfflineWithFeedback\n    in Unknown\n    in Unknown\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in Hoverable\n    in RCTView\n    in Unknown\n    in Pressable\n    in PressableWithSecondaryInteraction\n    in Unknown\n    in ReportActionItem\n    in Unknown\n    in Unknown\n    in Unknown\n    in Unknown\n    in Unknown\n    in RCTView\n    in Unknown\n    in InvertedCell\n    in VirtualizedListCellContextProvider\n    in CellRenderer\n    in RCTView\n    in Unknown\n    in RCTScrollView\n    in ScrollView\n    in ScrollView\n    in VirtualizedListContextProvider\n    in VirtualizedList\n    in FlatList\n    in BaseInvertedFlatList\n    in Unknown\n    in Unknown\n    in RCTView\n    in Unknown\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in ReportActionsList\n    in Unknown\n    in Unknown\n    in Unknown\n    in Unknown\n    in withDrawerState(withWindowDimensions(withLocalize(withPersonalDetails(withNetwork(ReportActionsList)))))\n    in Unknown\n    in ReportActionsView\n    in Unknown\n    in Unknown\n    in Unknown\n    in RCTView\n    in Unknown\n    in FullPageNotFoundView\n    in Unknown\n    in Suspender\n    in Suspense\n    in Freeze\n    in RCTView\n    in Unknown\n    in KeyboardAvoidingView\n    in RCTView\n    in Unknown\n    in SafeAreaConsumer\n    in ScreenWrapper\n    in Unknown\n    in withOnyx(withNetwork(ScreenWrapper))\n    in Unknown\n    in Unknown\n    in Unknown\n    in withNavigation(withWindowDimensions(withKeyboardState(Component)))\n    in Unknown\n    in ReportScreen\n    in withOnyx(ReportScreen)\n    in Unknown\n    in Unknown\n    in withDrawerState(withNetwork(Component))\n    in Unknown\n    in Unknown\n    in Unknown\n    in WithViewportOffsetTop(withLocalize(withWindowDimensions(Component)))\n    in Unknown\n    in StaticContainer\n    in EnsureSingleNavigator\n    in SceneView\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in Background\n    in Screen\n    in RNSScreen\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in Suspender\n    in Suspense\n    in Freeze\n    in DelayedFreeze\n    in InnerScreen\n    in Screen\n    in MaybeScreen\n    in RNSScreenContainer\n    in ScreenContainer\n    in MaybeScreenContainer\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in AnimatedComponent(View)\n    in Unknown\n    in RCTView\n    in Unknown\n    in AnimatedComponent(View)\n    in Unknown\n    in Wrap\n    in AnimatedComponent(Wrap)\n    in Unknown\n    in GestureDetector\n    in Drawer\n    in DrawerViewBase\n    in RNGestureHandlerRootView\n    in GestureHandlerRootView\n    in RCTView\n    in Unknown\n    in SafeAreaProviderCompat\n    in DrawerView\n    in PreventRemoveProvider\n    in NavigationContent\n    in Unknown\n    in DrawerNavigator\n    in BaseDrawerNavigator\n    in Unknown\n    in MainDrawerNavigator\n    in withOnyx(MainDrawerNavigator)\n    in Unknown\n    in StaticContainer\n    in EnsureSingleNavigator\n    in SceneView\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in CardSheet\n    in RCTView\n    in Unknown\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in PanGestureHandler\n    in PanGestureHandler\n    in RCTView\n    in Unknown\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in RCTView\n    in Unknown\n    in Card\n    in CardContainer\n    in RNSScreen\n    in AnimatedComponent\n    in AnimatedComponentWrapper\n    in Suspender\n    in Suspense\n    in Freeze\n    in DelayedFreeze\n    in InnerScreen\n    in Screen\n    in MaybeScreen\n    in RNSScreenContainer\n    in ScreenContainer\n    in MaybeScreenContainer\n    in RCTView\n    in Unknown\n    in Background\n    in CardStack\n    in RCTView\n    in Unknown\n    in SafeAreaProviderCompat\n    in RNGestureHandlerRootView\n    in GestureHandlerRootView\n    in StackView\n    in CustomRootStackNavigator\n    in AuthScreens\n    in withOnyx(AuthScreens)\n    in Unknown\n    in Unknown\n    in AppNavigator\n    in EnsureSingleNavigator\n    in BaseNavigationContainer\n    in ThemeProvider\n    in NavigationContainerInner\n    in NavigationRoot\n    in DeeplinkWrapper\n    in Expensify\n    in withOnyx(Expensify)\n    in Unknown\n    in Unknown\n    in BaseErrorBoundary\n    in KeyboardStateProvider\n    in WindowDimensionsProvider\n    in RenderersPropsProvider\n    in ListStyleSpecsProvider\n    in SharedPropsProvider\n    in RenderRegistryProvider\n    in RenderHTMLConfigProvider\n    in TRenderEngineProvider\n    in BaseHTMLEngineProvider\n    in HTMLEngineProvider\n    in LocaleContextProvider\n    in withOnyx(LocaleContextProvider)\n    in Unknown\n    in WithCurrentUserPersonalDetails(Component)\n    in Unknown\n    in withOnyx(Component)\n    in Unknown\n    in _default\n    in PortalProviderComponent\n    in RNCSafeAreaProvider\n    in SafeAreaProvider\n    in BetasProvider\n    in withOnyx(BetasProvider)\n    in Unknown\n    in Private_blockedFromConciergeProvider\n    in withOnyx(Private_blockedFromConciergeProvider)\n    in Unknown\n    in CurrentDateProvider\n    in withOnyx(CurrentDateProvider)\n    in Unknown\n    in ReportActionsDrafts_Provider\n    in withOnyx(ReportActionsDrafts_Provider)\n    in Unknown\n    in PersonalDetailsProvider\n    in withOnyx(PersonalDetailsProvider)\n    in Unknown\n    in NetworkProvider\n    in withOnyx(NetworkProvider)\n    in Unknown\n    in ComposeProviders\n    in OnyxProvider\n    in ComposeProviders\n    in RNGestureHandlerRootView\n    in GestureHandlerRootView\n    in App\n    in RCTView\n    in Unknown\n    in RCTView\n    in Unknown\n    in AppContainer"}
MelvinBot commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

situchan commented 1 year ago

I believe this crash came from attachment modal which was a recent regression and now fixed. https://github.com/Expensify/App/issues/16636

alexxxwork commented 1 year ago

Found this https://stackoverflow.com/questions/52368342/invariant-violation-text-strings-must-be-rendered-within-a-text-component

So it seems we should look for all string && { code } blocks and fix them

Change ALL OF YOUR LOGICAL CONDITIONAL RENDERS into ternary renditions.
alexxxwork commented 1 year ago

@luacmartins if these errors are new and should've appeared after recent changes in staging here are potential problems:

https://github.com/Expensify/App/blob/8a1510373598807a4ea6c5049f2a3121dfbd9215/src/components/Picker/Picker.js#L179-L196

and https://github.com/Expensify/App/blob/8a1510373598807a4ea6c5049f2a3121dfbd9215/src/components/Picker/Picker.js#L258-L263

we shouldn't use this.props.label && and this.props.hintText &&

these are coming from this PR https://github.com/Expensify/App/pull/16449

alexxxwork commented 1 year ago

I believe this crash came from attachment modal which was a recent regression and now fixed. #16636

@situchan @luacmartins could be because of comments in return function https://github.com/margelo/expensify-app-fork/blob/7b8f419e51d4309c30fe348535d58f3df2fde7cb/src/components/KeyboardAvoidingView/index.android.js#L17 and https://github.com/margelo/expensify-app-fork/blob/7b8f419e51d4309c30fe348535d58f3df2fde7cb/src/components/KeyboardAvoidingView/index.android.js#L23

situchan commented 1 year ago

@alexxxwork nope, this happened on iOS as well. image

luacmartins commented 1 year ago

It seems like the exceptions haven't happened on v1.2.92-0, which means it could be related to this deploy blocker. That being said, I do think that we should update all instances of the string && component pattern.

Please post a full proposal that addresses all instances of this pattern in App.

praveen-ix commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Text strings must be rendered within a component.

What is the root cause of that problem?

The possible root causes of the issue are as follows: -

  1. Use of Logical AND (&&) operator for the condition rendering inside the View element.
  2. Functional Component returns string only.
  3. Use of the string outside the Text element.
  4. Use of Terminate (;) operator inside the View element.

What changes do you think we should make in order to solve the problem?

The possible solutions to the issue are as follows: -

  1. Use the Ternary Operator instead of the Logical AND (&&) operator for condition view rendering.
  2. Functional Component should always return React Native elements.
  3. Always use string within the Text element.
  4. Do not use the Terminate (;) operator and if-else condition with curly braces inside the View element.

What alternative solutions did you explore? (Optional)

NA

alexxxwork commented 1 year ago

Please post a full proposal that addresses all instances of this pattern in App.

@luacmartins I believe these are all cases now

Proposal

Please re-state the problem that we are trying to solve in this issue.

We get Text strings must be rendered within a <Text> component error

What is the root cause of that problem?

We need to search for pattern string && JSX to eliminate such cases

What changes do you think we should make in order to solve the problem?

we need to fix this (replace with Boolean(*), add to PropTypes):

List of files to fix https://github.com/Expensify/App/blob/8a1510373598807a4ea6c5049f2a3121dfbd9215/src/components/Picker/Picker.js#L179-L196 https://github.com/Expensify/App/blob/8a1510373598807a4ea6c5049f2a3121dfbd9215/src/components/Picker/Picker.js#L258-L263 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/AttachmentModal.js#L271-L305 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/MenuItem.js#L93 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/MenuItem.js#L159 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/OptionRow.js#L274 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/RadioButtonWithLabel.js#L70-L75 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/Section.js#L49-L59 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/ImageView/index.native.js#L178 (this could be the cause of the crashalytics crash) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/ReportActionItem/IOUPreview.js#L234 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/ScreenWrapper/index.js#L123 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/TextInput/BaseTextInput.js#L247 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/TextInput/BaseTextInput.js#L319 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/TextInput/BaseTextInput.js#L331 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/components/ValidateCode/ExpiredValidateCodeModal.js#L95-L103 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/DetailsPage.js#L152 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/ReportSettingsPage.js#L219 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/ReportSettingsPage.js#L229 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/EnablePayments/TermsPage/LongTermsForm.js#L68-L82 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/home/ReportScreen.js#L267 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/home/report/ReportActionCompose.js#L721 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/home/report/ReportActionCompose.js#L900 (not in state in constructor) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/home/report/ReportActionItemFragment.js#L132 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/ReimbursementAccount/ACHContractStep.js#L221 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/ReimbursementAccount/BankAccountStep.js#L115 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/ReimbursementAccount/BankAccountStep.js#L133 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/ReimbursementAccount/EnableStep.js#L110 (not in proptypes) https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/settings/InitialSettingsPage.js#L303 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/settings/Profile/Contacts/LoginField.js#L128 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/signin/PasswordForm.js#L218 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/signin/SignInPage.js#L119 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js#L248 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/pages/workspace/card/WorkspaceCardVBANoECardView.js#L60 https://github.com/Expensify/App/blob/239874465fa29de0cd7ca24274834d3a9897ba28/src/stories/Composer.stories.js#L67
luacmartins commented 1 year ago

@alexxxwork your proposal looks good.

MelvinBot commented 1 year ago

📣 @alexxxwork You have been assigned to this job by @luacmartins! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

alexxxwork commented 1 year ago

@luacmartins @mananjadhav The PR is ready for review

allroundexperts commented 1 year ago

@luacmartins @alexxxwork Do we have a lint rule that errors on this pattern? If not, how about creating one?

luacmartins commented 1 year ago

@allroundexperts AFAIK we don't have a lint rule, but it'd be good to add one.

allroundexperts commented 1 year ago

@allroundexperts AFAIK we don't have a lint rule, but it'd be good to add one.

@luacmartins Should I post it in Slack channel or create a PR directly?

alexxxwork commented 1 year ago

@luacmartins @alexxxwork Do we have a lint rule that errors on this pattern? If not, how about creating one?

@allroundexperts the problem is we frequently have boolean props and sometimes they are in separate files with PropTypes. Also there's plenty of props not in proptypes without any description (but we have a lint rule on missing props) and there are cases with an additional variable calculated, so it could be compilcated to write an isBoolean function for our lint rule. I'd say that true solution would be typescript 😃

MelvinBot commented 1 year ago

@mananjadhav, @alexxxwork, @luacmartins, @miljakljajic Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.99-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-04-20. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

luacmartins commented 1 year ago

@mananjadhav would you mind completing the the checklist above please? I think there were a lot of PRs that introduced this.

mananjadhav commented 1 year ago

I'll do that by tomorrow.

I think there were a lot of PRs that introduced this.

Agreed, I am going to link as many as I can for the change.

luacmartins commented 1 year ago

PR is in prod, just missing the checklist and payment

alexxxwork commented 1 year ago

@mananjadhav Just to keep it moving:

src/components/AttachmentModal.js https://github.com/Expensify/App/pull/685

src/components/ImageView/index.native.js https://github.com/Expensify/App/pull/15472

src/components/MenuItem.js https://github.com/Expensify/App/pull/10333

src/components/OptionRow.js https://github.com/Expensify/App/pull/13319

src/components/Picker/BasePicker.js https://github.com/Expensify/App/pull/3414 (src/components/ExpensiPicker/index.js) https://github.com/Expensify/App/pull/16449

src/components/RadioButtonWithLabel.js https://github.com/Expensify/App/pull/8009

src/components/ReportActionItem/IOUPreview.js https://github.com/Expensify/App/pull/13329

src/components/ScreenWrapper/propTypes.js https://github.com/Expensify/App/pull/10140

src/components/Section.js https://github.com/Expensify/App/pull/6234 (src/pages/workspace/WorkspaceSection.js)

src/components/TextInput/BaseTextInput.js https://github.com/Expensify/App/pull/6501

src/components/TextInput/baseTextInputPropTypes.js https://github.com/Expensify/App/pull/6234

src/components/ValidateCode/ExpiredValidateCodeModal.js https://github.com/Expensify/App/pull/15505

src/pages/DetailsPage.js https://github.com/Expensify/App/pull/4154

src/pages/EnablePayments/TermsPage/LongTermsForm.js https://github.com/Expensify/App/pull/3879

src/pages/ReimbursementAccount/ACHContractStep.js https://github.com/Expensify/App/pull/13501

src/pages/ReimbursementAccount/BankAccountStep.js https://github.com/Expensify/App/pull/13236 https://github.com/Expensify/App/pull/9423

src/pages/ReimbursementAccount/EnableStep.js https://github.com/Expensify/App/pull/6319

src/pages/ReportSettingsPage.js https://github.com/Expensify/App/pull/7028

src/pages/home/ReportScreen.js https://github.com/Expensify/App/pull/10452

src/pages/home/report/ReportActionCompose.js https://github.com/Expensify/App/pull/14686 (PR deletes proptype)

src/pages/home/report/reportActionFragmentPropTypes.js src/pages/home/report/ReportActionItemFragment.js https://github.com/Expensify/App/pull/2320

src/pages/settings/InitialSettingsPage.js https://github.com/Expensify/App/pull/9560

src/pages/settings/userPropTypes.js https://github.com/Expensify/App/pull/6319

src/pages/signin/PasswordForm.js https://github.com/Expensify/App/pull/14627

src/pages/signin/SignInPage.js https://github.com/Expensify/App/pull/1025

src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js https://github.com/Expensify/App/pull/14627

src/pages/workspace/card/WorkspaceCardVBANoECardView.js https://github.com/Expensify/App/pull/6319

.storybook/preview.js https://github.com/Expensify/App/pull/7984

luacmartins commented 1 year ago

@mananjadhav when you get a chance, can you complete the BZ checklist please?

mananjadhav commented 1 year ago

@luacmartins apologies for the delay here, I was out sick. I verified some of the offending PRs tagged by @alexxxwork. They seem to be relevant. How do you propose commenting on the all the PRs? or should we just post a thread on #expensify-open-source ?

luacmartins commented 1 year ago

I think that we can leave a comment on each PR saying that it introduced that pattern and to watch out for it.

alexxxwork commented 1 year ago

I think that we can leave a comment on each PR saying that it introduced that pattern and to watch out for it.

@luacmartins I need to mention that I tried to identify the first PR to introduce the pattern. Some of them are pretty old ones. Also there's a number of PRs that refactored or moved the code later without changing these parts.

luacmartins commented 1 year ago

@alexxxwork thanks. I don't think we need to worry about those here. There were clearly many that introduced it and we should just add something to the checklist to prevent future PRs from introducing this issue.

miljakljajic commented 1 year ago

Contract extended to @mananjadhav - @alexxxwork, can you apply to the job in Upwork? I'm not finding your surname when I search. https://www.upwork.com/jobs/~01ef3651f57da6d5c9

alexxxwork commented 1 year ago

@miljakljajic I've already appied for this job. My upwork profile is https://upwork.com/freelancers/~01558071651a8abfb5

miljakljajic commented 1 year ago

Offer extended @alexxxwork

miljakljajic commented 1 year ago

Alex and Manan have been paid. Is it possible to introduce a regression test that would have caught this bug? I see that multiple PRs contributed to it.

Specifically I'm wondering what you mean here @luacmartins:

we should just add something to the checklist to prevent future PRs from introducing this issue.

luacmartins commented 1 year ago

We should add a new check to the PR review checklist. Maybe something like:

mananjadhav commented 1 year ago

Ohh Yes that's a good idea. @luacmartins do you want me to raise a PR? Do we need to document this in the Style guide as well?

luacmartins commented 1 year ago

PR created https://github.com/Expensify/App/pull/18069

aldo-expensify commented 1 year ago

Ohh Yes that's a good idea. @luacmartins do you want me to raise a PR? Do we need to document this in the Style guide as well?

There is this related thing in the guideline: https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#important-note

but it is talking about it in the context of ternaries

mananjadhav commented 1 year ago

I think the check should then suffice.

mananjadhav commented 1 year ago

@luacmartins @aldo-expensify @miljakljajic I've posted a comment on all the PRs so that the contributors are aware about this in the future.

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.7-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-05-05. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: