-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show eReceipts for distance requests #27204
Conversation
This feels pretty good to me, cc @Expensify/design The only major thing I am seeing is, should we vertically center the receipt in the available area when you view it? We might want to coordinate with @georgia too on how this code overlaps with eReceipts for cards. |
|
||
/** The id of the transaction related to the attachment */ | ||
transactionID: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this being passed in? (Trying to understand patterns for ECard transactions eReceipts).
I'm having trouble finding where the URL is coming from, do you save it to the receipt key in the transaction object in the BE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AttachmentCarousel renders items here
App/src/components/Attachments/AttachmentCarousel/index.js
Lines 145 to 148 in 04e3eb1
const renderItem = useCallback( | |
({item}) => ( | |
<CarouselItem | |
item={item} |
renderItem is passed data from attachments
App/src/components/Attachments/AttachmentCarousel/index.js
Lines 204 to 206 in 04e3eb1
data={attachments} | |
CellRendererComponent={AttachmentCarouselCellRenderer} | |
renderItem={renderItem} |
attachments are set up here
const attachmentsFromReport = extractAttachmentsFromReport(report, reportActions); |
Here's where the transactionID is set
App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js
Line 62 in 61920ff
transactionID, |
I'm having trouble finding where the URL is coming from, do you save it to the receipt key in the transaction object in the BE?
Yeah it's from transaction.filename here
App/src/components/DistanceEReceipt.js
Line 34 in 8dedc7a
const {thumbnail} = TransactionUtils.hasReceipt(transaction) ? ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename) : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This was super helpful
Reviewing the PR and the previous conversations. Thanks for resolving the conflicts @neil-marcellini. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change looks fine except for one minor object I added.
Reviewer Checklist
Screenshots/VideosMobile Web - Chromemweb-chrome-distance-card.movMobile Web - Safarimweb-safari-distance-card.movDesktopdesktop-distance-carc.moviOSios-distance-card.movAndroidandroid-distance-card.mov |
@neil-marcellini All done from my side. 🎀 👀 🎀 C+ reviewed. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/grgia in version: 1.3.84-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.3.85-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
<PendingMapView /> | ||
) : ( | ||
<ThumbnailImage | ||
previewSourceURL={thumbnailSource} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, we had a bug here - App crashes when tapping on the receipt thumbnail while receipt is generating
On native, the pending attachment uses a static image, and it has the source: 1 (that's a standard RN, see this), which is not a string so it breaks the Str.isPDF() and Str.isImage() checks.
More details: #28814
eReceiptAmount: { | ||
...headlineFont, | ||
fontSize: variables.fontSizeXXXLarge, | ||
lineHeight: variables.lineHeightXXXLarge, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line caused an edge case regression in #29616; when device font size is set to the minimum, the amount in the e-receipt is cut off.
Details
When a distance request receipt image is clicked from the transaction report the attachment modal is opened. For distance requests an eReceipt is displayed instead of the full size image of the route. The amount, merchant, waypoints list, and created date are shown.
For now, the download button will only download the route image. In the future maybe we can download the entire eReceipt view.
Fixed Issues
$ #27050
$ #28289
$ #28949
PROPOSAL: N/A
Tests
Distance request with 2 waypoints
Distance request with 12 waypoints
Also run the "action performed" from the 2nd and 3rd linked issues.
Offline tests
Run the same tests while offline and verify you see TBD for some values
Go back online
Verify that the value fill in
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
androidChrome.mov
androidChromeFilled.mov
Mobile Web - Safari
Safari.mp4
Desktop
desktop1080.mov
iOS
https://drive.google.com/file/d/1-9bGeQcr-sFNX_8Q5wII57MUXeT3Jzg-/view?usp=sharing
Android
https://drive.google.com/file/d/10utdOdYCBTRV_tkUkHA6cB_tJxuyt7Q6/view?usp=sharing