[ON HOLD #30561] [$500] Attachment - Hmm its not here page displayed on login with receipt attachment URL #28568

lanitochka17 · 2023-10-01T20:51:42Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Action Performed:

Open the app
Click on plus and raise manual request money request with any amount and any user
Open the request money page by clicking twice on IOU message
edit URL and add '/edit/receipt' in it
Upload any image and after upload, open it
Copy the URL
Logout and visit the copied URL
Login and observe that it displays 'Hmm its not here' page

Expected Result:

App should display receipt image on login with receipt attachment

Actual Result:

App displays 'Hmm its not here' on login with receipt attachment

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.75-8

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

IOS.safari.hmm.its.not.here.login.attachment.URL.mov

windows.chrome.hmm.its.not.here.on.login.with.attachment.URL.mp4

android.chrome.hmm.its.not.here.on.login.with.attachment.URL.mp4

mac.chrome.hmm.its.not.here.on.login.with.attachment.URL.mov

Recording.121.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696066001795669

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019998a84689b09070
Upwork Job ID: 1708585513587392512
Last Price Increase: 2023-10-22

melvin-bot · 2023-10-01T20:51:47Z

Job added to Upwork: https://www.upwork.com/jobs/~019998a84689b09070

melvin-bot · 2023-10-01T20:51:47Z

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

melvin-bot · 2023-10-01T20:51:49Z

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-10-01T20:51:52Z

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

abzokhattab · 2023-10-01T22:45:52Z

Proposal

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

An error shown while logging in with the attachment url, instead of showing the attached document

What is the root cause of that problem?

The report and the report action elements are empty in the beginning causing the error to show up

App/src/pages/home/report/ReportAttachments.js

Line 24 in 4ebeea0

const report = ReportUtils.getReport(reportID);

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

we should remove the previous line and use useOnyx to subscribe to date changes in the report and the reportActions in the ReportAttachments component and pass it to the AttachmentModal page .. since the ReportUtils.getReport(reportID) returns empty report in the beginning, thats why we need to subscribe to data changes.

const propTypes = {
    /** Navigation route context info provided by react navigation */
    route: PropTypes.shape({
        /** Route specific parameters used on this screen */
        params: PropTypes.shape({
            /** The report ID which the attachment is associated with */
            reportID: PropTypes.string.isRequired,
            /** The uri encoded source of the attachment */
            source: PropTypes.string.isRequired,
        }).isRequired,
    }).isRequired,
    report: reportPropTypes,
    reportActions: PropTypes.shape(reportActionPropTypes),
};

const defaultProps = {
    report: {},
    reportActions: {},
};

function ReportAttachments(props) {
    const reportID = _.get(props, ['route', 'params', 'reportID']);
    const source = decodeURI(_.get(props, ['route', 'params', 'source']));
    return (
        <AttachmentModal
            allowDownload
            defaultOpen
            report={props.report}
            reportActions={props.reportActions}
            source={source}
            onModalHide={() => Navigation.dismissModal(reportID)}
            onCarouselAttachmentChange={(attachment) => {
                const route = ROUTES.REPORT_ATTACHMENTS.getRoute(reportID, attachment.source);
                Navigation.navigate(route);
            }}
        />
    );
}

ReportAttachments.propTypes = propTypes;
ReportAttachments.defaultProps = defaultProps;
ReportAttachments.displayName = 'ReportAttachments';

export default withOnyx({
    report: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
    },
    reportActions: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${route.params.reportID}`,
        canEvict: false,
    },
})(ReportAttachments);

where in the AttachementModel we should add the reportActions to the propTypes:

    reportActions: PropTypes.shape(reportActionPropTypes),

and also in the defaultProps:

    reportActions: {},

Now we need to update the AttachmentCarousel condition to show only if the report and the report actions are not empty in order to prevent the Hmm its not here error .

so we need to change this:

App/src/components/AttachmentModal.js

Lines 381 to 389 in fbd04cf

    
           {!_.isEmpty(props.report) ? ( 
        
               <AttachmentCarousel 
        
                   report={props.report} 
        
                   onNavigate={onNavigate} 
        
                   source={props.source} 
        
                   onClose={closeModal} 
        
                   onToggleKeyboard={updateConfirmButtonVisibility} 
        
                   setDownloadButtonVisibility={setDownloadButtonVisibility} 
        
               />

To:

       {!_.isEmpty(props.report) && !_.isEmpty(props.reportActions) ? (
                        <AttachmentCarousel
                            report={props.report}
                            onNavigate={onNavigate}
                            reportActions={props.reportActions}
                            source={props.source}
                            onClose={closeModal}
                            onToggleKeyboard={updateConfirmButtonVisibility}
                            setDownloadButtonVisibility={setDownloadButtonVisibility}
                        />

POC

Screen.Recording.2023-10-02.at.1.41.48.AM.mov

dukenv0307 · 2023-10-02T03:48:44Z

Proposal

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

App displays 'Hmm its not here' on login with receipt attachment

What is the root cause of that problem?

When the page loads for the first time, the report and reportActions are not available yet. So the page here is -1, thus it shows the not found view.

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

Since the ReportAttachments don't need the full report object (only the reportID is needed), we should modify it to pass through only the reportID to AttachmentModal rather than full report object. This will minimize component rerendering due to report/reportActions change
In AttachmentModal, connect to report via Onyx (from the reportID passed as prop)
In AttachmentCarousel, connect to report via Onyx (from the reportID passed as prop)
In AttachmentCarousel, if the report and report actions are still loading (use the same condition as in here), we'll show loading indicator.

const isLoadingReport = isLoadingReportData && (_.isEmpty(report) || !report.reportID);
const isLoadingReportAction = _.isEmpty(reportActions) || reportMetadata.isLoadingReportActions;

if (isLoadingReport || isLoadingReportAction) {
    return <FullscreenLoadingIndicator />;
}

This will make sure it renders correctly if we go to offline mode right after logging in.

(Optional) If we want to show something else instead of loading during offline mode, like You need to be online to load this report attachment (when the report and report actions are not loaded yet), we can add a condition to show that as well.

What alternative solutions did you explore? (Optional)

We can modify the withReportAndReportActionOrNotFound a bit and wrap it around AttachmentCarousel to achieve the same above, but this will require more changes that can impact other components.

CortneyOfstad · 2023-10-04T12:15:06Z

@sobitneupane thoughts on the proposals above?

CortneyOfstad · 2023-10-05T19:15:25Z

@sobitneupane bump ^^^

melvin-bot · 2023-10-08T16:00:09Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot · 2023-10-09T11:27:58Z

@CortneyOfstad, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

sobitneupane · 2023-10-09T11:40:39Z

Sorry for the delay.

I will try to review the proposals before EOD tomorrow (NPT).

sobitneupane · 2023-10-10T08:53:59Z

@abzokhattab Thanks for your proposal.

Now we need to update the AttachmentCarousel condition to show only if the report and the report actions are not empty in order to prevent the Hmm its not here error .

We might want to show "Hmm its not here error" page if user don't have access to the attachment. I don't think our goal is to completely prevent Not found page.

sobitneupane · 2023-10-10T08:56:11Z

@dukenv0307 Thanks for your proposal.

Since the ReportAttachments and AttachmentModal don't need the full report object (only the reportID is needed), we should modify them to pass through only the reportID rather than full report object.

We are using report.parentReportActionID in AttachmentModal.

This will make sure it renders correctly if we go to offline mode right after logging in.

How are we handling attachments in offline mode as of now if they are not already loaded?

dukenv0307 · 2023-10-10T15:41:40Z

We are using report.parentReportActionID in AttachmentModal.

Yeah it's a new code update, we'll need to connect to the report as well in AttachmentModal then.

How are we handling attachments in offline mode as of now if they are not already loaded?

@sobitneupane currently it will show loading indicator when offline if the attachment is not yet ready. When we go online the attachment will show.

sobitneupane · 2023-10-12T08:06:49Z

Yeah it's a new code update, we'll need to connect to the report as well in AttachmentModal then.

@dukenv0307 Can you please update your proposal to reflect the update?

currently it will show loading indicator when offline if the attachment is not yet ready. When we go online the attachment will show.

In that case, we can have the same behavior here as well.

dukenv0307 · 2023-10-13T18:14:28Z

@dukenv0307 Can you please update your proposal to reflect the update?

Proposal updated, thank you for the feedback

In that case, we can have the same behavior here as well.

@sobitneupane Yes my proposal already follows that behavior.

melvin-bot · 2023-10-15T11:49:58Z

@CortneyOfstad @sobitneupane this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot · 2023-10-15T16:00:12Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

mountiny · 2023-12-11T11:36:12Z

PR not on hold anymore @dukenv0307 @situchan lets get this over the finish line, thanks!

situchan · 2023-12-11T11:42:21Z

@mountiny please check #30561 (comment). Should we prioritize this over #31105?

mountiny · 2023-12-11T12:01:47Z

Thanks, I am bumping the PR this is on hold for

CortneyOfstad · 2023-12-22T14:15:58Z

Any update @mountiny? Thanks!

mountiny · 2023-12-26T15:40:28Z

PR is still on hold

melvin-bot · 2024-01-18T17:17:51Z

This issue has not been updated in over 15 days. @CortneyOfstad, @mountiny, @situchan, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

mountiny · 2024-01-18T18:23:23Z

@situchan @dukenv0307 is this still on hold?

dukenv0307 · 2024-01-23T04:35:10Z

@mountiny Yes, still on hold.

CortneyOfstad · 2024-02-15T18:26:01Z

I just realized the title was never updated for the on-hold PR (#30561 (comment)), so updated to better reflect that.

I did a bit of a deep dive and found that the issue was on hold because of another PR (#28999) which was merged about 3 weeks ago. @situchan @dukenv0307 I bumped you both on your PR. Let me know where we are at with that 👍

situchan · 2024-02-15T18:35:59Z

Updated on PR

CortneyOfstad · 2024-02-22T18:38:01Z

@situchan checking the PR, it shows that @dukenv0307 mentioned an issue last week affecting main. Do either of you have an update on the status of the PR? Do you think that this is something that can be resolved by EOW? Thanks!

situchan · 2024-02-22T18:42:23Z

Seems like we're currently blocked on unrelated bug

CortneyOfstad · 2024-02-28T14:40:04Z

@situchan bumped you on the on-hold issue, as it looks like the review is waiting for you. Can you please provide an update there by EOD?

CortneyOfstad · 2024-03-11T16:21:56Z

PR has some movement in the last few hours, so looking good there! Will continue to keep an eye on it 👍

mountiny · 2024-03-11T22:17:23Z

@dukenv0307 @situchan what should we do here, the issue is on hold for the PR which you can now close @dukenv0307

dukenv0307 · 2024-03-12T03:41:11Z

@mountiny We're discussing this in the PR.

CortneyOfstad · 2024-03-13T14:39:10Z

Discussed with @mountiny and due to the nature of the original PR we decided a partial payment of 75% is appropriate here. Normally, no payment would be made, as another PR was utilized. However, due to the time and effort put into the original PR, we both felt that payment was appropriate.

@situchan @dukenv0307 — I sent you both offers in Upwork for the adjusted contract. Please let me know once you accept and i can get those paid ASAP today. Thank you!

CortneyOfstad · 2024-03-14T15:27:06Z

@dukenv0307 It is showing that it is still waiting for you to accept the offer in Upwork — can you let me know once you accept and I can get that paid to you ASAP. Thanks!

dhanashree-sawant · 2024-03-14T15:31:46Z

Hello @CortneyOfstad @mountiny, I tried but couldn't understand whether the issue was worked on somewhere or not so can you let me know if it is eligible for reporting bonus or not?

dukenv0307 · 2024-03-14T15:56:43Z

@CortneyOfstad I've accepted the offer, thank you!

CortneyOfstad · 2024-03-14T18:37:49Z

@dhanashree-sawant Sorry about that! I've sent you a request in Upwork — please let me know once you accept and I will get that paid ASAP. Thanks!

dhanashree-sawant · 2024-03-14T18:40:38Z

That's okay, thanks I have accepted the offer.

CortneyOfstad · 2024-03-14T18:45:14Z

Payment Summary

@dhanashree-sawant (reporter) – paid $50 via Upwork
@dukenv0307 (contributor) — paid $375 via Upwork
@situchan (C+) — paid $375 via Upwork

lanitochka17 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 Oct 1, 2023

melvin-bot bot changed the title ~~Attachment - Hmm its not here page displayed on login with receipt attachment URL~~ [$500] Attachment - Hmm its not here page displayed on login with receipt attachment URL Oct 1, 2023

melvin-bot bot assigned CortneyOfstad Oct 1, 2023

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

melvin-bot bot assigned sobitneupane Oct 1, 2023

melvin-bot bot added the Overdue label Oct 4, 2023

melvin-bot bot removed the Overdue label Oct 4, 2023

melvin-bot bot added the Overdue label Oct 8, 2023

melvin-bot bot removed the Overdue label Oct 9, 2023

melvin-bot bot added the Overdue label Oct 15, 2023

melvin-bot bot added the Monthly KSv2 label Dec 11, 2023

mountiny added Weekly KSv2 and removed Monthly KSv2 labels Dec 11, 2023

melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jan 18, 2024

CortneyOfstad changed the title ~~[$500] Attachment - Hmm its not here page displayed on login with receipt attachment URL~~ [ON HOLD #30561] [$500] Attachment - Hmm its not here page displayed on login with receipt attachment URL Feb 15, 2024

CortneyOfstad closed this as completed Mar 14, 2024

[ON HOLD #30561] [$500] Attachment - Hmm its not here page displayed on login with receipt attachment URL #28568

[ON HOLD #30561] [$500] Attachment - Hmm its not here page displayed on login with receipt attachment URL #28568

Comments

lanitochka17 commented Oct 1, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Oct 1, 2023

melvin-bot bot commented Oct 1, 2023

melvin-bot bot commented Oct 1, 2023 • edited by CortneyOfstad Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Oct 1, 2023

abzokhattab commented Oct 1, 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?

POC

dukenv0307 commented Oct 2, 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)

CortneyOfstad commented Oct 4, 2023

CortneyOfstad commented Oct 5, 2023

melvin-bot bot commented Oct 8, 2023

melvin-bot bot commented Oct 9, 2023

sobitneupane commented Oct 9, 2023 • edited Loading

sobitneupane commented Oct 10, 2023

sobitneupane commented Oct 10, 2023

dukenv0307 commented Oct 10, 2023

sobitneupane commented Oct 12, 2023

dukenv0307 commented Oct 13, 2023

melvin-bot bot commented Oct 15, 2023

melvin-bot bot commented Oct 15, 2023

mountiny commented Dec 11, 2023

situchan commented Dec 11, 2023

mountiny commented Dec 11, 2023

CortneyOfstad commented Dec 22, 2023

mountiny commented Dec 26, 2023

melvin-bot bot commented Jan 18, 2024

mountiny commented Jan 18, 2024

dukenv0307 commented Jan 23, 2024

CortneyOfstad commented Feb 15, 2024

situchan commented Feb 15, 2024

CortneyOfstad commented Feb 22, 2024

situchan commented Feb 22, 2024

CortneyOfstad commented Feb 28, 2024

CortneyOfstad commented Mar 11, 2024

mountiny commented Mar 11, 2024

dukenv0307 commented Mar 12, 2024

CortneyOfstad commented Mar 13, 2024

CortneyOfstad commented Mar 14, 2024

dhanashree-sawant commented Mar 14, 2024

dukenv0307 commented Mar 14, 2024

CortneyOfstad commented Mar 14, 2024

dhanashree-sawant commented Mar 14, 2024

CortneyOfstad commented Mar 14, 2024

Payment Summary

lanitochka17 commented Oct 1, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Oct 1, 2023 •

edited by CortneyOfstad

Loading

abzokhattab commented Oct 1, 2023 •

edited

Loading

dukenv0307 commented Oct 2, 2023 •

edited

Loading

sobitneupane commented Oct 9, 2023 •

edited

Loading