[HOLD for payment 2023-12-08] [$1000] Web - App does not display working report on back from 'something is wrong page'

kbecciv commented 10 months ago

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

Action Performed:

Open the app
Open any report, to trigger something is wrong page, send any attachment, click on attachment and copy URL, send URL, delete the attachment and click on URL
Click on browser back, observe that it changes URL but it still displays something is wrong page
Reload and observe that app now displays proper report

Expected Result:

App should display chat report on back from 'something is wrong' page if report don't have any issue

Actual Result:

App still displays 'something is wrong' page on browser back click even though app changes URL to a working report

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.41-2 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/caf339c1-633f-4bf8-b4b7-db3968a9dba3

https://github.com/Expensify/App/assets/93399543/8a738523-b893-429d-ac72-e37547cd53ba

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689531247187909

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f4efb0d0e28187e6
Upwork Job ID: 1682055964789444608
Last Price Increase: 2023-11-20
Automatic offers:

0xmiroslav | Reviewer | 27774673
WikusKriek | Contributor | 27774676
dhanashree-sawant | Reporter | 27774677

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

DanutGavrus commented 10 months ago

Proposal

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

App does not display working report on back from 'something is wrong page'

What is the root cause of that problem?

We throw an error if we don't find the attachment: https://github.com/Expensify/App/blob/ab3d0e876e311ed1ce3cd859ad6dd77a5d8e3cdf/src/components/AttachmentCarousel/index.js#L192-L195
We never clear that error, so pressing on the back button, still shows the Error page.

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

To fix 1st problem:

Instead of throwing an error, we could show the FullPageNotFoundView, and optionally, we could add a `Log` and a new text in our languages to show a custom error.

if (page === -1) {
    Log.warn('Attachment not found'); // Optional
    return {
        attachmentNotFound: true,
    }
}

<FullPageNotFoundView
  shouldShow={this.state.attachmentNotFound}
  subtitleKey='notFound.attachmentYouLookingForCannotBeFound' // Optional
  >
  <View

Add to en.js file(Optional, we'll also need the Spanish translation):

notFound: {
    attachmentYouLookingForCannotBeFound: 'The attachment you are looking for cannot be found.',

To fix 2nd problem:

Inside BaseErrorBoundary component, add the componentDidMount method which is not yet present. In it, we'll add a listener which clears the existing error when user presses on the back button(if it exists):

componentDidMount() {
    window.onpopstate = () => {
        if (this.state.hasError) {
            this.clearError();
        }
    }
}

Adding the listener in the componentDidMount method will not flash the page when user presses on the back button.

michaelhaxhiu commented 10 months ago

This is a very edge case bug, but it is indeed "inconsistent" from the behavior we'd expect in the app. I think we should try to fix it (and hopefully it's not a big undertaking to do so).

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

rk111 commented 10 months ago

Hi, Assign to me I will work in this

kameshwarnayak commented 10 months ago

Proposal

Edited to address 2 issues as per this comment https://github.com/Expensify/App/issues/23068#issuecomment-1692141049

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

Bug 1: Infinite loading shows when you open not available/inaccessible attachments (Point 5 in the above comment) Bug 2: App redirects to not accessible chat (Point 7 in the above comment)

What is the root cause of that problem?

Bug 1: Infinite loading shows when you open not available/inaccessible attachments (Point 5 in the above comment)

We are not handling onError/onLoadEnd in the ImageView. isLoading is set to true in onLoadStart={imageLoadingStart}. However isLoading is reset to false in onLoad={imageLoad} which doesn't get called since the image is not loaded. This causes infinite loading error.

https://github.com/Expensify/App/blob/7eb96d8a8f8ceeef7460e05f0d428d19b05ca6b3/src/components/ImageView/index.js#L257-L264

Bug 2: App redirects to not accessible chat (Point 7 in the above comment)

Root cause of this bug is that when the AttachmentModal is closed, we call Navigation.dismissModal.

In Navigation.dismissModal, if the targetReportID is not same as the one on top of the stack, we replace the URL with the URL of the targetReportID

https://github.com/Expensify/App/blob/c2dab6ea63e0802888e32de3cc36fa15bfb706b8/src/pages/home/report/ReportAttachments.js#L33

https://github.com/Expensify/App/blob/c2dab6ea63e0802888e32de3cc36fa15bfb706b8/src/libs/Navigation/Navigation.js#L167-L172

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

To solve Bug 1 we need to write an handler for onError/onLoadEnd for Image component.

Define a state variable error const [error, setError] = useState(false);

Define function onError to set error and remove loading

const onError = (e) =>{
    setError(true);
    setIsLoading(false);
}

Set onError handler in the Image component and If error is true show a BlockingView

{error === true ? (
<BlockingView
    icon={Illustrations.ToddBehindCloud}
    iconWidth={variables.modalTopIconWidth}
    iconHeight={variables.modalTopIconHeight}
    title={translate('notFound.notHere')} 
/>
) : (
    <Image
        source={{uri: url}}
        isAuthTokenRequired={isAuthTokenRequired}
        // Hide image until finished loading to prevent showing preview with wrong dimensions.
        style={isLoading ? undefined : [styles.w100, styles.h100]}
        // When Image dimensions are lower than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions.
        // Both `center` and `contain` keeps the image centered on both x and y axis.
        resizeMode={zoomScale > 1 ? Image.resizeMode.center : Image.resizeMode.contain}
        onLoadStart={imageLoadingStart}
        onLoad={imageLoad}
        onError={onError}
    />
)}

Result :

https://github.com/Expensify/App/assets/2232432/15b63998-8deb-48e8-a512-0944f6b741b8

To solve Bug 2 we can check for error and if there is any error in loading the image, open the report in the top of the Stack. We can achieve this by following the below steps

Pass error param to Navigation.dismissModal

If error is true, open the latest report

        if (!error && targetReportID && targetReportID !== getTopmostReportId(rootState)) {

In ReportAttachment.js define a state variable error and pass the value to Navigation.dismissModal

const [error, setError] = useState(false);

return (
    <AttachmentModal
        ...
        onError={setError}
        onModalHide={() => Navigation.dismissModal(reportID, error)}
        ...
    />

onError handler is passed to the children components and is called when there is an error in loading an image

Result :

https://github.com/Expensify/App/assets/2232432/02bd8a0d-3f41-4d49-b270-1dadd64b320b

What alternative solutions did you explore? (Optional)

None

rk111 commented 9 months ago

Proposal

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

The problem we are trying to solve is that the app does not display the working report when the user navigates back from the "Something is wrong" page.

What is the root cause of that problem?

The root cause of this problem is that in the AttachmentCarousel component, when the app doesn't find the expected attachment, it throws an error. This error prevents the app from displaying the working report, leaving the user stuck on the "Something is wrong" page.

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

To solve the problem, instead of throwing an error, we should handle the scenario where the attachment is not found in a more graceful manner. We can make the following changes:

In the AttachmentCarousel component, when the attachment is not found, set a state flag (e.g., "attachmentNotFound") to true, indicating that the attachment is not available.

Rather than throwing an error, we should return from the function to allow the app to continue executing without interruption.

Show the FullPageNotFoundView component with a custom error message when the "attachmentNotFound" flag is true.

Optional Changes:

Log a warning (using Log.warn) when the attachment is not found, which can be helpful for debugging purposes.

Add a new text key in the language files (e.g., en.js and possibly in Spanish translation as well) to display a custom error message when the attachment is not found.

// Inside AttachmentCarousel class

constructor(props) {
    super(props);
    this.state = {
        attachmentNotFound: false,
    };
}

// Inside the function where the attachment is not found

const page = _.findIndex(attachments, (a) => a.source === this.props.source);
if (page === -1) {
    Log.warn('Attachment not found'); // Optional
    this.setState({ attachmentNotFound: true });
    return;
}

// Render method

render() {
    // ...
    return (
        <>
            {this.state.attachmentNotFound && (
                <FullPageNotFoundView
                    shouldShow={true}
                    subtitleKey='notFound.attachmentYouLookingForCannotBeFound' // Optional, provide the language key
                />
            )}
            <View
                // Render the AttachmentCarousel content here
            />
        </>
    );
}

greg-schroeder commented 9 months ago

For C+ (@0xmiroslav) - can you confirm if any solutions in this issue may apply to https://github.com/Expensify/App/issues/23374?

melvin-bot[bot] commented 9 months ago

@michaelhaxhiu, @0xmiroslav Huh... This is 4 days overdue. Who can take care of this?

michaelhaxhiu commented 9 months ago

DM'ed miki to make sure he's aware of this one. We can re-assign to another C+ if you are busy too!

0xmiroslav commented 9 months ago

For C+ (@0xmiroslav) - can you confirm if any solutions in this issue may apply to #23374?

@greg-schroeder I followed repro step in #23374. First time, it shows infinite loading and from next time, it crashes which is this GH. So I suggest to put #23374 on hold for this.

0xmiroslav commented 9 months ago

Page not found view looks good to me but it's new design pattern to show it in preview modal, not in full screen.

@shawnborton do you think it's fine? (Here, back arrow button should be removed of course)

melvin-bot[bot] commented 9 months ago

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

michaelhaxhiu commented 9 months ago

cc @shawnborton do you agree with this design approach? I think it's fairly simple and a natural extension of what we already do.

shawnborton commented 9 months ago

I think the modal approach with the "Not found" view makes sense, but yes, we should remove that back arrow.

kameshwarnayak commented 9 months ago

I think the bug is more on the issue of back from "Something is wrong page" than a "Not found". I have forced an error which triggers "Something is wrong page". This is not a "Not found" issue. But clicking back still remains in the same page. Seen in the video below.

https://github.com/Expensify/App/assets/2232432/d4f98b98-3cbf-40ef-a5ae-b8f0943b6316

The root cause is that the error state is not cleared on back. https://github.com/Expensify/App/issues/23068#issuecomment-1646582365 this proposal handles that issue. Seen in the video below with the exact usecase as above

https://github.com/Expensify/App/assets/2232432/e7f30f0c-965d-484e-81c4-cbc3ad3f5773

melvin-bot[bot] commented 9 months ago

@michaelhaxhiu @0xmiroslav 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!

DanutGavrus commented 9 months ago

If we'll add the The attachment you are looking for cannot be found. custom message, could we also get the correct translated alternative in Spanish? Thanks!

melvin-bot[bot] commented 9 months ago

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

michaelhaxhiu commented 9 months ago

There are some existing proposals and I want to confirm if those are viable. If not @0xmiroslav can you let me know and I'll double the price to get more activity here?

0xmiroslav commented 9 months ago

Still want to see more proposals which also fix https://github.com/Expensify/App/issues/23068#issuecomment-1660007287

kameshwarnayak commented 9 months ago

Still want to see more proposals which also fix #23068 (comment)

https://github.com/Expensify/App/issues/23068#issuecomment-1646582365 - This fixes the above issue and example mentioned in the bug. However it is not considering the modal view for the images which is a design improvement for the specific use case. Should I change the proposal to add design change too?

DanutGavrus commented 9 months ago

@greg-schroeder @0xmiroslav If this issue is about displaying the working report on back from "something is wrong page", generated by any error, and not actually fixing only the reported error, should we reopen https://github.com/Expensify/App/issues/23374? Asking because after the above clarification, these 2 seem to be different Issues now. Thanks!

Edit: If so, it seems that we have 1 Proposal for clearing any error on back, and 1 for fixing the error found here and in 23374. (If they are good enough). Doubling the price, as suggested, will generate the same cost eventually, and may require more time.

melvin-bot[bot] commented 9 months ago

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

michaelhaxhiu commented 9 months ago

Note: I'm preparing to go OOO for ~2 weeks and need a BZ buddy to watch over this in the meantime. 🙏

Next step: let's wait 1-2 business days for @0xmiroslav to weigh in on the latest comments about potentially expanding scope of this bug fix. If we disagree, I think we should double price to get this moving and assigned.

melvin-bot[bot] commented 9 months ago

@michaelhaxhiu, @sonialiap, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

sonialiap commented 9 months ago

@0xmiroslav any thoughts on expanding the scope of this bug fix?

melvin-bot[bot] commented 9 months ago

@michaelhaxhiu @sonialiap @0xmiroslav this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

0xmiroslav commented 9 months ago

It makes sense to fix infinite loading as well here as it reaches same result of showing not found page. So agree with doubling this and pick up proposal which fixes both #23068 and #23374

DanutGavrus commented 9 months ago

Proposal Updated

EDIT: Following the reported steps from here on latest staging seems to not be reproducible. I've thrown a random error when going to the Settings Page in order to reproduce this with slightly different steps. In the InitialSettingsPage component, I've added throw new Error('This is a test error.'); before the return statement.

Ahmed-Abdella commented 9 months ago

@0xmiroslav https://github.com/Expensify/App/issues/23374#issuecomment-1671651180 please check this.

kameshwarnayak commented 9 months ago

@Ahmed-Abdella - This (https://github.com/Expensify/App/issues/23374#issuecomment-1671651180) solves the infinite spinning. Checked the branch. The mentioned PR can be appended to add a error message.

The second issue is solved by this proposal - https://github.com/Expensify/App/issues/23068#issuecomment-1646582365

melvin-bot[bot] commented 9 months ago

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

0xmiroslav commented 9 months ago

reviewing latest proposals

melvin-bot[bot] commented 9 months ago

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

sonialiap commented 9 months ago

@0xmiroslav any thoughts on the proposals?

melvin-bot[bot] commented 9 months ago

@michaelhaxhiu, @sonialiap, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 9 months ago

@michaelhaxhiu, @sonialiap, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 8 months ago

@michaelhaxhiu, @sonialiap, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 8 months ago

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

sonialiap commented 8 months ago

@0xmiroslav bump on reviewing proposals. If you do not have bandwidth for this issue, let me know and I can reassign

0xmiroslav commented 8 months ago

I am not able to reproduce this anymore

0xmiroslav commented 8 months ago

@dhanashree-sawant can you please confirm that this is fixed?

0xmiroslav commented 8 months ago

Instead, I found another weird bug:

After closing attachment modal, goes to not found page

Repro step:

Send any attachment on any private chat
Copy attachment link and paste into public room
On another device, open that public room deep link
Click attachment link
Infinite loading shows (BUG1)
Close modal
App redirects to not accessible chat (BUG2)

https://github.com/Expensify/App/assets/97473779/d000e646-8ab2-4bd4-b012-0c6e0d6b7368

kameshwarnayak commented 8 months ago

@0xmiroslav Please correct me if I am wrong.

We are mixing two different issues here. I believe the original issue here is that if one clicks back from 'something is wrong page', the redirect doesn't happen. Attachment was taken as an example to create that scenario. I have forced the 'something is wrong page' in this comment https://github.com/Expensify/App/issues/23068#issuecomment-1660007287 without using an attachment and we still face the issue.

The root cause is that we are not clearing the error in "BaseErrorBoundary.js" file. https://github.com/Expensify/App/issues/23068#issuecomment-1646582365 states the root cause and the solution to this exact issue.

There are multiple bugs that are created for the infinite spinner (https://github.com/Expensify/App/issues/23374, https://github.com/Expensify/App/issues/23899). Since Attachment was used as an example to create the 'something is wrong page' crash we are getting confused on the actual bug which is "If we go back from 'something is wrong page', the app doesn't refresh". This happens even when we don't have attachments.

I think we should handle both separately. Please correct if my understanding is wrong.

DanutGavrus commented 8 months ago

@0xmiroslav

I am not able to reproduce this anymore

Yes, this commit added a BlockingView on 08 Aug in order to prevent the error from the reproduction steps to happen.

But, starting from the Issue title "App does not display working report on back from 'something is wrong page'" you may reproduce the same problem reported here with these steps https://github.com/Expensify/App/issues/23068#issuecomment-1670349768

To fix that, from my Proposal we don't need 1st fix anymore as it was fixed by the previously mentioned PR, but we may proceed with implementing 2nd fix in order to "display working report on back from 'something is wrong page'".

0xmiroslav commented 8 months ago

I think 7. App redirects to not accessible chat (BUG2) from my comment is what you're talking about on 2nd fix.

Expensify / App