Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.12k stars 2.61k forks source link

[$250] Distance - "Error loading messages" appears after dismissing invalid distance error #42518

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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!


Version Number: 1.4.75-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/41481

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat that has no unsettled request
  3. Go offline
  4. Submit a distance expense with invalid addresses
  5. Go online
  6. Click on the distance preview
  7. Dismiss the error message
  8. Click on the header subtitle to return to the main chat
  9. Scroll down

Expected Result:

There will be no "There was an error loading more messages." in the main chat

Actual Result:

"There was an error loading more messages." error shows up in the main chat after dismissing the error

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1b49ecdf-a7d1-40ef-a05d-be02ebd93b22

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013c3c05166994ac09
  • Upwork Job ID: 1795874267241521152
  • Last Price Increase: 2024-06-12
Issue OwnerCurrent Issue Owner: @ishpaul777
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 1 month ago

@JmillsExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 month ago

We think that this bug might be related to #wave-collect - Release 1

jainilparikh commented 1 month ago

Proposal

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

Error message "There was an error loading more messages" is visible even when there is no error in the chat flow (The error is in the MoneyRequestView).

What is the root cause of that problem?

The error message "There was an error loading more messages" in the chat view is controlled by the flag: hasLoadingNewerReportActionsError which is a part of ReportMetadata. When we close the error modal (The modal that shows Route exceeded... ) this flag is not flipped.

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

Create a new function in ReportActions.tsx as such:

function callOnxyMerge(reportID: string) {
    Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`, {
        isLoadingInitialReportActions: false,
        hasLoadingNewerReportActionsError: false,
    });
}

and call that when the error modal closes, here:

https://github.com/Expensify/App/blob/main/src/components/ReportActionItem/MoneyRequestView.tsx#L356

This will flip the hasLoadingNewerReportActionsError flag to false, hence, when the user navigates back to the main chat view, the error message There was an error loading more messages won't be visible.

melvin-bot[bot] commented 1 month ago

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify commented 1 month ago

Opening up to the community since we have a proposal.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~013c3c05166994ac09

melvin-bot[bot] commented 1 month ago

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

ishpaul777 commented 1 month ago

Thanks for your proposal @jainilparikh, but to be able to evaluate the proposal I might need more details and a detailed Root cause Analysis, Would you mind adding more details to your proposal

melvin-bot[bot] commented 1 month ago

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

ishpaul777 commented 1 month ago

Awaiting proposals

goldenbear101 commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue. Error message "There was an error loading more messages" is visible even when there is no error in the chat flow (The error is in the MoneyRequestView).

What is the root cause of that problem? I think there is a bug where the error message from the MoneyRequestView propagates to the chat view, causing the "Error loading messages" error to display after dismissing an invalid distance error.

What changes do you think we should make in order to solve the problem? The suggested fix involves making sure that when the error modal in MoneyRequestView is dismissed, the state indicating an error in loading messages is reset.

The state hasLoadingNewerReportActionsError should be reset to false in Onyx when the error modal is dismissed in MoneyRequestView.

Steps to fix: 1, I will check how the state hasLoadingNewerReportActionsError is managed in MoneyRequestView and ChatView. 2, Modify MoneyRequestView to reset the error state by ensuring that dismissing the error modal triggers an update to reset hasLoadingNewerReportActionsError in Onyx. 3, Update the MoneyRequestView component, find where the error modal is dismissed and add code to reset the hasLoadingNewerReportActionsError state in Onyx. 4, Make sure the Onyx key hasLoadingNewerReportActionsError is correctly defined and can be updated from the MoneyRequestView.

Implementation: 1, Finding the dismiss error modal code in MoneyRequestView:

// Assuming the error modal dismissal is handled in a function like this:
dismissErrorModal() {
    this.setState({ showErrorModal: false });

    // Add code to reset the error state in Onyx
    Onyx.merge(ONYXKEYS.REPORT_HAS_LOADING_NEWER_REPORT_ACTIONS_ERROR, false);
}

2, Ensure Onyx Key is Defined:

Verify that ONYXKEYS.REPORT_HAS_LOADING_NEWER_REPORT_ACTIONS_ERROR is properly defined in your Onyx keys:

// ONYXKEYS.js
const ONYXKEYS = {
    ...
    REPORT_HAS_LOADING_NEWER_REPORT_ACTIONS_ERROR: 'hasLoadingNewerReportActionsError',
    ...
};
export default ONYXKEYS;

3, Handle State Reset in Onyx:

Make sure the state is properly updated in Onyx when merging:

Onyx.merge(ONYXKEYS.REPORT_HAS_LOADING_NEWER_REPORT_ACTIONS_ERROR, false); What alternative solutions did you explore? (Optional)

This approach is to make sure that the state indicating an error in loading messages is properly reset, to avoid the propagation of the error to the chat view.

ishpaul777 commented 1 month ago

@goldenbear101 Thanks for your interest in this issue, I suggest you please take a look at our contribution guildelines and few issues where contributors have posted proposals and get yourself familiarized with the codebase and try writing a detailed proposal on where & what to change and for a proposal to be evaluated it's important to put accurate and detailed RCA.

melvin-bot[bot] commented 1 month ago

@JmillsExpensify @ishpaul777 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!

ishpaul777 commented 1 month ago

we are looking for proposals

jainilparikh commented 4 weeks ago

Updated: https://github.com/Expensify/App/issues/42518#issuecomment-2131373176

CC: @ishpaul777 for review

NJ-2020 commented 3 weeks ago

Proposal

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

"Error loading messages" appears after dismissing an invalid distance error

What is the root cause of that problem?

The backend will show an error because of the invalid address so it won't create the report including the report ID in the backend, but we still keep the ONYX data so when we click the subheader link, we pass the parent report ID and thread report id (which doesn't exist because backend error) to getNewerActions function https://github.com/Expensify/App/blob/66cd70716217b91c927d5267f7672b1d080ac17f/src/pages/home/report/ReportActionsView.tsx#L225

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

We can check if the report ID exists in the backend, and then we can invoke the getNewerActions

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 3 weeks ago

@JmillsExpensify, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

NJ-2020 commented 3 weeks ago

Proposal

Updated

ishpaul777 commented 3 weeks ago

I have asked another c+ volunteer for this issue https://expensify.slack.com/archives/C02NK2DQWUX/p1718127011310639

allroundexperts commented 3 weeks ago

Jumping in today as promised on channel. It seems like that we need to fix this on the backend. Backend seems to be returning hasLoadingNewerReportActionsError as true.

NJ-2020 commented 3 weeks ago

I also think that the issue is in the backend. So, does it mean that there is no point in fixing the frontend side as per my proposal?

ishpaul777 commented 3 weeks ago

Thanks for taking this @allroundexperts!

@JmillsExpensify Please assign @allroundexperts

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify commented 2 weeks ago

Assigned!

melvin-bot[bot] commented 2 weeks ago

@JmillsExpensify @allroundexperts this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 1 week ago

@JmillsExpensify, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify commented 1 week ago

Awaiting PR

melvin-bot[bot] commented 6 days ago

@JmillsExpensify, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify commented 4 days ago

Same