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 URL routing for IOU #3888

Merged
merged 2 commits into from
Jul 7, 2021
Merged

Fix URL routing for IOU #3888

merged 2 commits into from
Jul 7, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jul 6, 2021

cc @Julesssss

Details

Fixed Issues

$ #3886

Tests

QA Steps

  1. Navigate to e.cash
  2. Click on FAB menu
  3. Click on Split bill or request money
  4. Verify no %3AreportID is shown in the URL
  5. Verify there are no other regressions

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Jul 6, 2021
@marcaaron marcaaron marked this pull request as ready for review July 6, 2021 16:06
@marcaaron marcaaron requested a review from a team as a code owner July 6, 2021 16:06
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team July 6, 2021 16:06
Comment on lines +34 to +50
IOU_REQUEST,
IOU_BILL,
IOU_SEND,
IOU_REQUEST_WITH_REPORT_ID: `${IOU_REQUEST}/:reportID?`,
IOU_BILL_WITH_REPORTID: `${IOU_BILL}/:reportID?`,
IOU_SEND_WITH_REPORTID: `${IOU_SEND}/:reportID?`,
getIouRequestRoute: reportID => `${IOU_REQUEST}/${reportID}`,
getIouSplitRoute: reportID => `${IOU_BILL}/${reportID}`,
getIOUSendRoute: reportID => `${IOU_SEND}/${reportID}`,
IOU_BILL_CURRENCY: `${IOU_BILL}/:reportID/currency`,
IOU_REQUEST_CURRENCY: `${IOU_REQUEST}/:reportID/currency`,
IOU_SEND_CURRENCY: `${IOU_SEND}/:reportID/currency`,
getIouRequestCurrencyRoute: reportID => `${IOU_REQUEST}/${reportID}/currency`,
getIouBillCurrencyRoute: reportID => `${IOU_BILL}/${reportID}/currency`,
getIouSendCurrencyRoute: reportID => `${IOU_SEND}/${reportID}/currency`,
IOU_DETAILS,
IOU_DETAILS_WITH_IOU_REPORT_ID: `${IOU_DETAILS}/:chatReportID/:iouReportID/`,
Copy link
Contributor

@jasperhuangg jasperhuangg Jul 7, 2021

Choose a reason for hiding this comment

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

NAB: Have we defined a pattern for how we want to organize these routes? It seems like they're grouped by the pages they're related to. I personally think it's more clear if the getter for a route is placed right after the route itself.

This might be a good opportunity to define a clearer way to organize routes since this list is only going to get longer and longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think it's more clear if the getter for a route is placed right after the route itself.

I tried to reorganize things in a way that made sense to me, but definitely prioritizing the bug fix over the engineering efficiency of having things in a certain order. 😄

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

looks good and tests well!

@marcaaron marcaaron merged commit fad829f into main Jul 7, 2021
@marcaaron marcaaron deleted the marcaaron-iouTransitions branch July 7, 2021 17:34
@OSBotify
Copy link
Contributor

OSBotify commented Jul 7, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@roryabraham
Copy link
Contributor

Unfortunately the deploy comments are not working right now. This was deployed to staging yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants