Open dukenv0307 opened 6 months ago
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]
@dukenv0307 Any update on the above?
Update today.
@abdulrahuman5196 I responded in the review thread.
@dukenv0307 The changes doesn't seem to work properly in my testing.
https://github.com/Expensify/App/assets/46707890/ec47301f-1f73-48a9-b302-1101a01327e2
https://github.com/Expensify/App/assets/46707890/98805514-d05b-4ab4-846d-f68578c4eec1
@abdulrahuman5196 I believe the bug mentioned here is not related to my code change. The current behavior of the app is signing in via deeplink does not take the user to the correct report in the URL -> this causes OpenReport API not to get called, leading to infinite loading
Please check the video attached below for more details.
https://github.com/Expensify/App/assets/129500732/b1a37f8f-3246-46fe-a7d7-9611fdf7d9b7
I think we'd better put this on hold to find the PR or issue that caused this new bug
Will take a review today
@abdulrahuman5196 Friendly bump.
@dukenv0307 I doubt if the route is returned wrongly. I just wrapped the EditRequestPage
with WithReportOrNotFound
and printed the received reportID it had proper reportID. I think there could be something else happening here.
Code change I Made.
diff --git a/src/ROUTES.ts b/src/ROUTES.ts
index 49067d1c7b..f292a636b8 100644
--- a/src/ROUTES.ts
+++ b/src/ROUTES.ts
@@ -156,8 +156,8 @@ const ROUTES = {
getRoute: (reportID: string) => `r/${reportID}` as const,
},
EDIT_REQUEST: {
- route: 'r/:threadReportID/edit/:field',
- getRoute: (threadReportID: string, field: ValueOf<typeof CONST.EDIT_REQUEST_FIELD>) => `r/${threadReportID}/edit/${field}` as const,
+ route: 'r/:reportID/edit/:field',
+ getRoute: (reportID: string, field: ValueOf<typeof CONST.EDIT_REQUEST_FIELD>) => `r/${reportID}/edit/${field}` as const,
},
EDIT_CURRENCY_REQUEST: {
route: 'r/:threadReportID/edit/currency',
diff --git a/src/pages/EditRequestPage.js b/src/pages/EditRequestPage.js
index e41f30779f..855f39be4b 100644
--- a/src/pages/EditRequestPage.js
+++ b/src/pages/EditRequestPage.js
@@ -29,6 +29,7 @@ import EditRequestReceiptPage from './EditRequestReceiptPage';
import EditRequestTagPage from './EditRequestTagPage';
import reportActionPropTypes from './home/report/reportActionPropTypes';
import reportPropTypes from './reportPropTypes';
+import withReportOrNotFound from './home/report/withReportOrNotFound';
const propTypes = {
/** Route from navigation */
@@ -285,6 +286,7 @@ EditRequestPage.displayName = 'EditRequestPage';
EditRequestPage.propTypes = propTypes;
EditRequestPage.defaultProps = defaultProps;
export default compose(
+ withReportOrNotFound(),
withOnyx({
report: {
key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.threadReportID}`,
diff --git a/src/pages/home/report/withReportOrNotFound.tsx b/src/pages/home/report/withReportOrNotFound.tsx
index cf2c0d5aca..5c10e6cb81 100644
--- a/src/pages/home/report/withReportOrNotFound.tsx
+++ b/src/pages/home/report/withReportOrNotFound.tsx
@@ -73,7 +73,10 @@ export default function (
return withOnyx<TProps & RefAttributes<TRef>, OnyxProps>({
report: {
- key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID}`,
+ key: ({route}) => {
+ console.log("Abdul Test" + JSON.stringify(route))
+ return `${ONYXKEYS.COLLECTION.REPORT}${route.params.reportID || route.params.threadReportID}`
+ },
},
isLoadingReportData: {
Video of testing:
https://github.com/Expensify/App/assets/46707890/34e7097c-bd45-41d2-9d27-5e1baa6ee3ac
@abdulrahuman5196 Updated. I fixed this issue in the past here https://github.com/Expensify/App/issues/28641 but it was removed when we migrate ReportScreenIDSetter
to Typescript.
@abdulrahuman5196 Any update here.
Will check tomorrow. @dukenv0307 Meanwhile could you kindly fix the merge conflicts.
@abdulrahuman5196 resolved conflict.
@abdulrahuman5196 I think we should hold this until EditRequestPage
is removed.
@abdulrahuman5196 I think we should hold this until
EditRequestPage
is removed.
Oh. This PR got missed.
Why should we wait on? If so could you share the links and reasoning?
Because we're refactor money request flow. And after that, EditRequestPage will be removed.
@abdulrahuman5196 What do you think about my comment above.
@dukenv0307 Could you provide references of the refractor? Any GH for reference to confirm?
@abdulrahuman5196 It's here https://github.com/Expensify/App/issues/29107
@abdulrahuman5196 Updated the PR to add a new solution after refactor by adding the logic to show the loading page in WithWritableReportOrNotFound
.
@abdulrahuman5196 Friendly bump.
Checking now
@abdulrahuman5196 Any update here.
Checking now
Checking again
Whenever I pick this issue I am not getting OTPs :sleepy: due to site issues I think. https://expensify.slack.com/archives/C01GTK53T8Q/p1715879150080079
I can get OTP to login.
Details
Login via deep link should open the modal in Request money
Fixed Issues
$ https://github.com/Expensify/App/issues/29115 PROPOSAL: https://github.com/Expensify/App/issues/29115#issuecomment-1754801666
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
https://github.com/Expensify/App/assets/129500732/4d106dc6-624b-4154-a9c1-9855a57267d2iOS: Native
https://github.com/Expensify/App/assets/129500732/ed475b9f-23e2-4420-a308-8a0eaa1d60b1iOS: mWeb Safari
https://github.com/Expensify/App/assets/129500732/e60bcff8-67bc-4460-bc55-c57c39991f3bMacOS: Chrome / Safari
https://github.com/Expensify/App/assets/129500732/c0e8c2da-e2ec-4376-9d46-d1900e94f4c3MacOS: Desktop
https://github.com/Expensify/App/assets/129500732/5f51cea0-a8d4-48ec-b9f8-e116da65b5e7