[Awaiting Payment 11th Jan] [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview #31105

izarutskaya · 2023-11-09T10:25:40Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.3.97-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @

Action Performed:

Create a request with a receipt via Request Money > Scan` (if you don't have one already)
Navigate to the IOU report
Add an attachment to the IOU report via + > Add attachment
Click on the uploaded attachment
Tap the < arrow in the attachment carousal
Observe the "Hmm it's not here" page

Expected Result:

The uploaded receipt can be seen in the attachment carousal modal

Actual Result:

The uploaded receipt can not be seen and "Hmm... it's not here" appears after clicking the < navigation arrow from an attachment preview

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6269678_1699519179592.2023-11-09_09-37-40.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a232fe720233dddb
Upwork Job ID: 1722561095734079488
Last Price Increase: 2023-11-09
Automatic offers:

dukenv0307 | Contributor | 27703336
shubham1206agra | Contributor | 28051420

Issue Owner

Current Issue Owner: @trjExpensify

The text was updated successfully, but these errors were encountered:

melvin-bot · 2023-11-09T10:25:45Z

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

melvin-bot · 2023-11-09T10:25:45Z

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

melvin-bot · 2023-11-09T10:25:47Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-11-09T10:25:51Z

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

dukenv0307 · 2023-11-09T10:43:39Z

Proposal

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

"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview

What is the root cause of that problem?

We get parent report action here to compare transactionID. But the report here is IOU report that makes parent report action is report preview action and then this check is wrong and not found page appears.

App/src/components/Attachments/AttachmentCarousel/index.js

Line 45 in 9862a3e

const action = ReportActionsUtils.getParentReportAction(report);

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

We should check here if the report is IOU or Expense report we shouldn't add this action to attachemnt list

App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

Line 49 in 9862a3e

    
           const transactionID = lodashGet(action, ['originalMessage', 'IOUTransactionID']);

What alternative solutions did you explore? (Optional)

We can remove the receipt image from carousal

Update this case to return if the action is money request action

if (!ReportActionsUtils.shouldReportActionBeVisible(action, key) || ReportActionsUtils.isMoneyRequestAction(action)) {
    return;
}

App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

Line 43 in d6cef83

if (!ReportActionsUtils.shouldReportActionBeVisible(action, key)) {

2. remove the compare transactionID on compareImage function

const compareImage = useCallback(
    (attachment) => {
        return attachment.source === source;
    },
    [source, report, isReceipt],
);

App/src/components/Attachments/AttachmentCarousel/index.js

Line 42 in d6cef83

const compareImage = useCallback(

In AttachmentModal, add an extra prop like isReceiptImage and replace isAttachmentReceipt state with this prop
Update this condition here to not open carouse for receipt image

!_.isEmpty(props.report) && !props.isReceiptImage

App/src/components/AttachmentModal.js

Line 445 in 5e7022d

{!_.isEmpty(props.report) ? (

In MoneyRequestView, we will pass full source of receipt into ReportActionItemImage.

App/src/components/ReportActionItem/MoneyRequestView.js

Line 170 in 5e7022d

<ReportActionItemImage

In ReportActionItemImage component, we will add an AttachmentModal, and then when we click on the receipt image we will open this modal instead of navigating to report attachment page.

<AttachmentModal
    headerTitle={'Receipt'}
    source={source}
    isAuthTokenRequired
    report={report}
    isReceiptImage
    allowToDownload
>
    {({show}) => (
        <PressableWithoutFocus
            style={[styles.noOutline, styles.w100, styles.h100]}
            onPress={show}
            role={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
            accessibilityLabel={translate('accessibilityHints.viewAttachment')}
        >
            {receiptImageComponent}
        </PressableWithoutFocus>
    )}
    
</AttachmentModal>

Result

Screen.Recording.2023-11-14.at.08.11.58.mov

trjExpensify · 2023-11-09T10:56:56Z

We get parent report action here to compare transactionID. But the report here is IOU report that makes this check is wrong and not found page appears.

I think the proposal lacks a little specificity as to the root cause. Why does this only happen when the receipt is still scanning though? For example here I'm in a transaction thread on an iou report and the carousel works just fine:

ql9FfgDys6.mp4

esh-g · 2023-11-09T11:00:09Z

Proposal

Please re-state the problem we are trying to solve

"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview

What is the root cause of this problem?

Parent report action transaction ID checking is done wrong here:

App/src/components/Attachments/AttachmentCarousel/index.js

Lines 42 to 52 in 9862a3e

    
           const compareImage = useCallback( 
        
               (attachment) => { 
        
                   if (attachment.isReceipt && isReceipt) { 
        
                       const action = ReportActionsUtils.getParentReportAction(report); 
        
                       const transactionID = _.get(action, ['originalMessage', 'IOUTransactionID']); 
        
                       return attachment.transactionID === transactionID; 
        
                   } 
        
                   return attachment.source === source; 
        
               }, 
        
               [source, report, isReceipt], 
        
           );

Because originalMessage.IOUTransactionID doesn't exist on the parent report action.

What changes should be made to fix this?

We can skip the separate checking for receipt and use the same way that other attachments are checked with their source attribute because the source attribute always exists
Change the compareImage function to the following:

const compareImage = useCallback(
        (attachment) => {
            return attachment.source === source;
        },
        [source],
    );

What alternative did you explore?

We can remove the receipts from the attachments altogether by the following:

Remove the code to extract receipts as attachments here:

App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

Lines 46 to 67 in 1615049

    
           // We're handling receipts differently here because receipt images are not 
        
           // part of the report action message, the images are constructed client-side 
        
           if (ReportActionsUtils.isMoneyRequestAction(action)) { 
        
               const transactionID = lodashGet(action, ['originalMessage', 'IOUTransactionID']); 
        
               if (!transactionID) { 
        
                   return; 
        
               } 
        
               const transaction = TransactionUtils.getTransaction(transactionID); 
        
               if (TransactionUtils.hasReceipt(transaction)) { 
        
                   const {image} = ReceiptUtils.getThumbnailAndImageURIs(transaction); 
        
                   const isLocalFile = typeof image === 'string' && (image.startsWith('blob:') || image.startsWith('file:')); 
        
                   attachments.unshift({ 
        
                       source: tryResolveUrlFromApiRoot(image), 
        
                       isAuthTokenRequired: !isLocalFile, 
        
                       file: {name: transaction.filename}, 
        
                       isReceipt: true, 
        
                       transactionID, 
        
                   }); 
        
                   return; 
        
               } 
        
           }

Change the compareImage function as mentioned in the change above (maybe remove the function use an inline function for it)
We can also remove the isReceipt state from the component and all the invocations where it is used.

dukenv0307 · 2023-11-09T11:01:25Z

@trjExpensify After the scan is complete we also can reproduce this issue and this issue happens on IOU report not in transaction report.

dukenv0307 · 2023-11-09T11:04:40Z

Screen.Recording.2023-11-09.at.18.03.54.mov

trjExpensify · 2023-11-09T11:13:30Z

Alright, these steps in the OP can be simplified. I've updated them:

Create a request with a receipt via Request Money > Scan` (if you don't have one already)
Navigate to the IOU report
Add an attachment to the IOU report via + > Add attachment
Click on the uploaded attachment
Tap the < arrow in the attachment carousal
Observe the "Hmm it's not here" page

trjExpensify · 2023-11-09T11:14:10Z

So @dukenv0307 why does this work fine in the transaction thread but not the IOU report?

dukenv0307 · 2023-11-09T11:16:14Z

@trjExpensify The current check now is used for transantion report so it works fine. parent action of transaction report will be a iou action that contain transactionID in originalMessage.

DylanDylann · 2023-11-10T07:58:06Z

Proposal

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

Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview

What is the root cause of that problem?

App/src/components/Attachments/AttachmentCarousel/index.js

Lines 55 to 63 in d4a97bc

    
           const attachmentsFromReport = extractAttachmentsFromReport(report, reportActions); 
        
           const initialPage = _.findIndex(attachmentsFromReport, compareImage); 
        
           // Dismiss the modal when deleting an attachment during its display in preview. 
        
           if (initialPage === -1 && _.find(attachments, compareImage)) { 
        
               Navigation.dismissModal(); 
        
           } else { 
        
               setPage(initialPage);

The logic of compareImage function is incorrect, It cause in line 63 we setPage(-1) --> Not Found Page.

Let see the logic of compareImage function

App/src/components/Attachments/AttachmentCarousel/index.js

Lines 42 to 49 in d4a97bc

    
           const compareImage = useCallback( 
        
               (attachment) => { 
        
                   if (attachment.isReceipt && isReceipt) { 
        
                       const action = ReportActionsUtils.getParentReportAction(report); 
        
                       const transactionID = _.get(action, ['originalMessage', 'IOUTransactionID']); 
        
                       return attachment.transactionID === transactionID; 
        
                   } 
        
                   return attachment.source === source;

The logic to get the current transactionID is incorrect. It is only correct in the Transaction Thread

With IOU/Expense Report

App/src/components/Attachments/AttachmentCarousel/index.js

Lines 44 to 46 in d4a97bc

    
           if (attachment.isReceipt && isReceipt) { 
        
               const action = ReportActionsUtils.getParentReportAction(report); 
        
               const transactionID = _.get(action, ['originalMessage', 'IOUTransactionID']);

This logic is incorrect.

It explains why this bug only happens in IOU report (not in Transaction Thread)

Note that: If we get transactionID based on report and reportAction props, for each report type, we need a separate logic to get transactionID

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

We should create new state called currentTransactionID
In here

App/src/components/Attachments/AttachmentCarousel/index.js

Line 95 in d4a97bc

setIsReceipt(entry.item.isReceipt);

we will call setCurrentTransactionID(entry.item.transactionID) and use currentTransactionID instead of this logic

App/src/components/Attachments/AttachmentCarousel/index.js

Lines 44 to 46 in d4a97bc

    
           if (attachment.isReceipt && isReceipt) { 
        
               const action = ReportActionsUtils.getParentReportAction(report); 
        
               const transactionID = _.get(action, ['originalMessage', 'IOUTransactionID']);

Result

Screen.Recording.2023-11-10.at.14.56.31.mov

mananjadhav · 2023-11-10T22:26:46Z

Checked the proposals, and @dukenv0307's proposal looks good.

🎀 👀 🎀 C+ reviewed.

melvin-bot · 2023-11-10T22:26:53Z

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

DylanDylann · 2023-11-11T06:49:04Z

@dukenv0307 In the yout main solution

We should check here if the report is IOU or Expense report we shouldn't add this action to attachment list

You mean that with the IOU report, in AttachmentCarousel we will not scroll to the receipt image, we only scroll attachment

cc @mananjadhav

dukenv0307 · 2023-11-11T06:58:36Z

@DylanDylann Yes because if we don't send any attachment we don't have the way to open reciept in IOU report.

If we want to show it in IOU report, I have a solution that is we will add an contextMenu action like view reciept which will show if the report action is reciept request and when click on this action, we will navigate to report attachment. And in attachment carous, we can replace isReciept state by transactionID and use this state to compare with attachment.transactionID

DylanDylann · 2023-11-11T07:00:25Z

In case, user send an attachment to IOU report. What happen?

dukenv0307 · 2023-11-11T07:05:19Z

@DylanDylann With my main solution, when we click on any attachment, reciept image doesn't show in the list.

DylanDylann · 2023-11-11T07:14:07Z

@dukenv0307 Thanks for your explanation, I think that we should display the receipt in the scroll list in the IOU report like we did in transaction thread
@mananjadhav I think we should confirm the expected before moving forward
@trjExpensify In the IOU report, when user click to attachment, the scroll list will display to allow user move to next/previous image. Should we display the receipt image of request in this list? Could you help to confirm the expected in this case?
cc @shawnborton Ping you here to get some opinion from UX perspective

trjExpensify · 2023-11-13T13:55:07Z

@trjExpensify In the IOU report, when user click to attachment, the scroll list will display to allow user move to next/previous image. Should we display the receipt image of request in this list? Could you help to confirm the expected in this case?

I don't see a reason to not allow that. We do in the transaction thread for example as illustrated here.

trjExpensify · 2023-12-21T01:35:15Z

So the second PR to fix the regression from the first PR is this one: #31105 (comment)

That has a couple of days to still go, but @situchan you're saying there's now a new regression from that 2nd PR: #31105 (comment)

If I have that correct, I think we need to remove the payment hold to await Melv adding that again on the deploy of it?

situchan · 2023-12-21T02:23:48Z

yes correct. @dukenv0307 should take responsible for fixing it.
And then I don't suggest to apply regression penalty given the efforts they made to fix this issue so far (just my suggestion)

melvin-bot · 2023-12-29T11:09:45Z

@mananjadhav, @trjExpensify, @mountiny, @shubham1206agra, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot · 2024-01-01T17:16:57Z

@mananjadhav, @trjExpensify, @mountiny, @shubham1206agra, @dukenv0307 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot · 2024-01-03T17:07:56Z

@mananjadhav, @trjExpensify, @mountiny, @shubham1206agra, @dukenv0307 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

mountiny · 2024-01-04T15:02:52Z

@situchan @dukenv0307 What are the next steps here please?

dukenv0307 · 2024-01-04T15:04:27Z

The regression is merged, waiting it deploy to production to complete the payment here.

mountiny · 2024-01-04T15:06:11Z

Thanks

melvin-bot · 2024-01-05T17:05:46Z

@mananjadhav, @trjExpensify, @mountiny, @shubham1206agra, @dukenv0307 10 days overdue. I'm getting more depressed than Marvin.

shubham1206agra · 2024-01-08T12:37:49Z

@trjExpensify Can you add the correct labels again for payment? It is due on 11th January.

trjExpensify · 2024-01-08T15:53:29Z

Sure, done!

trjExpensify · 2024-01-12T19:08:04Z

Okay, so confirming payments as follows:

$250 to @mananjadhav for the C+ review of the first PR
$250 to @shubham1206agra for the C+ review of the second PR
$250 to @dukenv0307 for the fix

Payment has been reduced accordingly due to the regression. Sound right?

mananjadhav · 2024-01-12T19:10:22Z

fyi, there were more than one regressions. I had helped with the proposal reviews and the review of the first PR that caused the first regression. Subsequently during my OOO it was assigned to @shubham1206agra.

Would like to request for partial compensation for the initial effort.

shubham1206agra · 2024-01-12T19:11:11Z

yes correct. @dukenv0307 should take responsible for fixing it.
And then I don't suggest to apply regression penalty given the efforts they made to fix this issue so far (just my suggestion)

@trjExpensify didn't you agree to not to apply regression reduction of payment here?

situchan · 2024-01-12T19:12:31Z

I imagine split compensation 1k into 3 people 😄

trjExpensify · 2024-01-12T19:27:05Z

Ah, I missed Manan reviewing the first PR.

@trjExpensify didn't you agree to not to apply regression reduction of payment here?

I interpreted @situchan's suggestion as not penalising for the second regression with the follow-up PR, which would result in $0 technically, which I agreed with.

mananjadhav · 2024-01-19T04:30:02Z

Ah, I missed Manan reviewing the first PR.

@trjExpensify Can you accommodate me ;)

trjExpensify · 2024-01-19T12:32:10Z

Yep! I've updated the payment summary here to add that. Go ahead and request.

Drawing a line in the sand, @dukenv0307 I've paid you $250. @shubham1206agra I've sent the offer for the same. Once accepted, I'll settle and close this out.

shubham1206agra · 2024-01-19T12:51:47Z

@trjExpensify Accepted

trjExpensify · 2024-01-19T13:16:38Z

Settled up!

JmillsExpensify · 2024-01-22T15:35:12Z

$250 payment approved for @mananjadhav based on this comment.

izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 9, 2023

melvin-bot bot changed the title ~~Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview~~ [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview Nov 9, 2023

melvin-bot bot assigned trjExpensify Nov 9, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 9, 2023

melvin-bot bot assigned mananjadhav Nov 9, 2023

melvin-bot bot assigned mountiny Nov 10, 2023

melvin-bot bot added the Overdue label Nov 13, 2023

dukenv0307 mentioned this issue Dec 21, 2023

Fix download file issue #33403

Merged

50 tasks

trjExpensify changed the title ~~[HOLD for payment 2023-12-15] [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview~~ [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview Dec 21, 2023

trjExpensify added Reviewing Has a PR in review and removed Awaiting Payment Auto-added when associated PR is deployed to production labels Dec 21, 2023

trjExpensify added the Awaiting Payment Auto-added when associated PR is deployed to production label Jan 8, 2024

trjExpensify changed the title ~~[$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview~~ [Awaiting Payment 11th Jan] [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview Jan 8, 2024

melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Jan 14, 2024

trjExpensify closed this as completed Jan 19, 2024

[Awaiting Payment 11th Jan] [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview #31105

[Awaiting Payment 11th Jan] [$500] Chat-"Hmm... it's not here" appears after clicking Previous arrow from an attachment preview #31105

Comments

izarutskaya commented Nov 9, 2023 • edited by mountiny Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Nov 9, 2023

melvin-bot bot commented Nov 9, 2023

melvin-bot bot commented Nov 9, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Nov 9, 2023

dukenv0307 commented Nov 9, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Result

trjExpensify commented Nov 9, 2023

esh-g commented Nov 9, 2023 • edited Loading

Proposal

Please re-state the problem we are trying to solve

What is the root cause of this problem?

What changes should be made to fix this?

What alternative did you explore?

dukenv0307 commented Nov 9, 2023

dukenv0307 commented Nov 9, 2023

trjExpensify commented Nov 9, 2023

trjExpensify commented Nov 9, 2023

dukenv0307 commented Nov 9, 2023

DylanDylann commented Nov 10, 2023

Proposal

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

What is the root cause of that problem?

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

Result

mananjadhav commented Nov 10, 2023

melvin-bot bot commented Nov 10, 2023

DylanDylann commented Nov 11, 2023 • edited Loading

dukenv0307 commented Nov 11, 2023 • edited Loading

DylanDylann commented Nov 11, 2023

dukenv0307 commented Nov 11, 2023

DylanDylann commented Nov 11, 2023

trjExpensify commented Nov 13, 2023

trjExpensify commented Dec 21, 2023

situchan commented Dec 21, 2023

melvin-bot bot commented Dec 29, 2023

melvin-bot bot commented Jan 1, 2024

melvin-bot bot commented Jan 3, 2024

mountiny commented Jan 4, 2024

dukenv0307 commented Jan 4, 2024

mountiny commented Jan 4, 2024

melvin-bot bot commented Jan 5, 2024

shubham1206agra commented Jan 8, 2024

trjExpensify commented Jan 8, 2024

trjExpensify commented Jan 12, 2024 • edited Loading

mananjadhav commented Jan 12, 2024 • edited Loading

shubham1206agra commented Jan 12, 2024

situchan commented Jan 12, 2024

trjExpensify commented Jan 12, 2024

mananjadhav commented Jan 19, 2024

trjExpensify commented Jan 19, 2024

shubham1206agra commented Jan 19, 2024

trjExpensify commented Jan 19, 2024

JmillsExpensify commented Jan 22, 2024

izarutskaya commented Nov 9, 2023 •

edited by mountiny

Loading

dukenv0307 commented Nov 9, 2023 •

edited

Loading

esh-g commented Nov 9, 2023 •

edited

Loading

DylanDylann commented Nov 11, 2023 •

edited

Loading

dukenv0307 commented Nov 11, 2023 •

edited

Loading

trjExpensify commented Jan 12, 2024 •

edited

Loading

mananjadhav commented Jan 12, 2024 •

edited

Loading