Skip to content
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

fix: replace new report with preexisting #24320

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -2534,6 +2534,17 @@ const CONST = {
DISTANCE: 'distance',
},
STATUS_TEXT_MAX_LENGTH: 100,
NAVIGATION: {
TYPE: {
FORCED_UP: 'FORCED_UP',
UP: 'UP',
},
ACTION_TYPE: {
REPLACE: 'REPLACE',
PUSH: 'PUSH',
NAVIGATE: 'NAVIGATE',
},
},
};

export default CONST;
3 changes: 2 additions & 1 deletion src/libs/Navigation/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import NAVIGATORS from '../../NAVIGATORS';
import originalGetTopmostReportId from './getTopmostReportId';
import getStateFromPath from './getStateFromPath';
import SCREENS from '../../SCREENS';
import CONST from '../../CONST';

let resolveNavigationIsReadyPromise;
const navigationIsReadyPromise = new Promise((resolve) => {
Expand Down Expand Up @@ -127,7 +128,7 @@ function goBack(fallbackRoute = ROUTES.HOME, shouldEnforceFallback = false, shou
}

if (shouldEnforceFallback || (isFirstRouteInNavigator && fallbackRoute)) {
navigate(fallbackRoute, 'UP');
navigate(fallbackRoute, CONST.NAVIGATION.TYPE.UP);
return;
}

Expand Down
19 changes: 12 additions & 7 deletions src/libs/Navigation/linkTo.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import NAVIGATORS from '../../NAVIGATORS';
import linkingConfig from './linkingConfig';
import getTopmostReportId from './getTopmostReportId';
import getStateFromPath from './getStateFromPath';
import CONST from '../../CONST';

/**
* Motivation for this function is described in NAVIGATION.md
Expand Down Expand Up @@ -59,19 +60,23 @@ export default function linkTo(navigation, path, type) {
const action = getActionFromState(state, linkingConfig.config);

// If action type is different than NAVIGATE we can't change it to the PUSH safely
if (action.type === 'NAVIGATE') {
// If this action is navigating to the report screen and the top most navigator is different from the one we want to navigate - PUSH
if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
action.type = 'PUSH';
if (action.type === CONST.NAVIGATION.ACTION_TYPE.NAVIGATE) {
// In case if type is 'FORCED_UP' we replace current screen with the provided. This means the current screen no longer exists in the stack
if (type === CONST.NAVIGATION.TYPE.FORCED_UP) {
action.type = CONST.NAVIGATION.ACTION_TYPE.REPLACE;

// If this action is navigating to the report screen and the top most navigator is different from the one we want to navigate - PUSH the new screen to the top of the stack
} else if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
action.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;

// If the type is UP, we deeplinked into one of the RHP flows and we want to replace the current screen with the previous one in the flow
// and at the same time we want the back button to go to the page we were before the deeplink
} else if (type === 'UP') {
action.type = 'REPLACE';
} else if (type === CONST.NAVIGATION.TYPE.UP) {
action.type = CONST.NAVIGATION.ACTION_TYPE.REPLACE;

// If this action is navigating to the RightModalNavigator and the last route on the root navigator is not RightModalNavigator then push
} else if (action.payload.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR && _.last(root.getState().routes).name !== NAVIGATORS.RIGHT_MODAL_NAVIGATOR) {
action.type = 'PUSH';
action.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;
}
}

Expand Down
16 changes: 15 additions & 1 deletion src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,20 @@ function handleReportChanged(report) {
return;
}

// It is possible that we optimistically created a DM/group-DM for a set of users for which a report already exists.
// In this case, the API will let us know by returning a preexistingReportID.
// We should clear out the optimistically created report and re-route the user to the preexisting report.
Copy link
Contributor

@youssef-lr youssef-lr May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this happen: We should clear out the optimistically created report and re-route the user to the preexisting report.? I've made use of the preexistingReportID to navigate the user to an existing report after creating a money request, but the stale optimistic report does not get removed for me. I had to add Onyx.merge(${ONYXKEYS.COLLECTION.REPORT}${report?.reportID}, null); after the navigation line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good question. I'm not seeing it either. Maybe we forgot to do it?

if (report && report.reportID && report.preexistingReportID) {
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, null);

// Only re-route them if they are still looking at the optimistically created report
if (Navigation.getActiveRoute().includes(`/r/${report.reportID}`)) {
// Pass 'FORCED_UP' type to replace new report on second login with proper one in the Navigation
Navigation.navigate(ROUTES.getReportRoute(report.preexistingReportID), CONST.NAVIGATION.TYPE.FORCED_UP);
}
return;
}

if (report && report.reportID) {
allReports[report.reportID] = report;

Expand Down Expand Up @@ -1720,7 +1734,7 @@ function openReportFromDeepLink(url, isAuthenticated) {
InteractionManager.runAfterInteractions(() => {
SidebarUtils.isSidebarLoadedReady().then(() => {
if (reportID) {
Navigation.navigate(ROUTES.getReportRoute(reportID), 'UP');
Navigation.navigate(ROUTES.getReportRoute(reportID), CONST.NAVIGATION.TYPE.UP);
}
if (route === ROUTES.CONCIERGE) {
navigateToConciergeChat();
Expand Down
Loading