Jules iou show paid preview#3404
Conversation
| shouldShowViewDetailsLink={Boolean(action.originalMessage.IOUReportID)} | ||
| onViewDetailsPressed={launchDetailsModal} | ||
| /> | ||
| {isMostRecentIOUReportAction |
There was a problem hiding this comment.
I don't get why this is not needed anymore, wouldn't this mean that we would show the preview for a paid IOU? I thought we did not want that?
There was a problem hiding this comment.
Yeah exactly. That logic had to move to within the Preview, as once the iouReport is paid we cannot guarantee it will be stored locally -- and the iouReport will be null
| <View style={[styles.flex1, styles.justifyContentBetween]}> | ||
| <ScrollView contentContainerStyle={styles.iouDetailsContainer}> | ||
| {(this.props.iouReport.hasOutstandingIOU) && ( | ||
| {(this.props.iouReport.hasOutstandingIOU || isIOUPaid) && ( |
There was a problem hiding this comment.
Yeah, good spot. I've removed that condition. Perhaps all the shouldDisplay logic should be handled by the component itself, but I don't think that change is necessary as part of this PR given that it's blocking N5.
Let me know if you disagree.
| // Usually the parent determines whether the IOU Preview is displayed. But as the iouReport total cannot be known | ||
| // until it is stored locally, we need to make this check within the Component after retrieving it. This allows us | ||
| // to handle the loading UI from within this Component instead of needing to declare it within each parent, which | ||
| // would duplicate and complicate the logic |
There was a problem hiding this comment.
Tbh, I don't really follow what logic would be complicated by this. It seems like the logic is already kind of weird and confusing and this comment doesn't explain much about why it was set up that way.
e.g. this component perhaps should not be responsible for fetching the iou at all...
this comment made me look into how we are doing it and... I think it's a pretty bad practice to be making an API call like this when the component renders... (instead of e.g. using a lifecycle method)
Not sure if anything needs to be done right now and I won't block on it (since the issue seems important to address), but feels like a code smell to me.
There was a problem hiding this comment.
Yeah, when making these changes it became apparent that the parent should perhaps be handling the loading state. Also, only one use of the comp has any requirement for displaying the preview now, so it might be possible to further simplify the shouldDisplay logic.
I didn't want this to turn into a large refactor and risk longer delays, so I've created an issue to refactor this component that I'll make external.
Beamanator
left a comment
There was a problem hiding this comment.
Tests succeeded, thanks for the detailed steps!
Code looks good too, looks like some logic will be improved further in the future, but gotta get this in asap 👍
|
🚀 Deployed to staging in version: 1.0.64-4🚀
|
|
🚀 Deployed to production in version: 1.0.65-0🚀
|
Details
Fixes regression where the IOUPreview would not display after a report was paid. It also fixes an issue where the 'Pay' button was displayed incorrectly after it had already been paid
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/166443
Tests
Ensure you are testing against the latest Auth/Web-E changes
QA Steps
Run above tests
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android