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.

Sign up for GitHub

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

Jump to bottom

Fix /r/undefined issue #1790

Merged
merged 4 commits into from
Mar 17, 2021
Merged

Fix /r/undefined issue #1790

merged 4 commits into from
Mar 17, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 16, 2021 •
edited
Loading

Details

When navigating to the site root in Expensify.cash we are redirecting to /r/undefined which looks pretty goofy. This PR adds a default parameter to the report route so the URL will never show undefined.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/157273

Tests

Web/Desktop

  1. Navigate to e.cash and sign out if you are signed in to create a fresh session
  2. Sign in
  3. Verify you are brought to /r instead of /r/undefined and that a report is visible in the main content area

mWeb

  1. Navigate to e.cash and sign out if you are signed in to create a fresh session
  2. Sign in
  3. Verify you see the LHN and not the most recent chat and that the url in the browser just says /r for now

iOS/Android

  1. Navigate to e.cash and sign out if you are signed in to create a fresh session
  2. Sign in
  3. Verify we are not navigated to any chats
  4. Swipe right and verify a chat exists

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Mar 16, 2021
@marcaaron marcaaron marked this pull request as ready for review March 16, 2021 17:43
@marcaaron marcaaron requested a review from a team as a code owner March 16, 2021 17:43
@botify botify requested review from NikkiWines and removed request for a team March 16, 2021 17:43
NikkiWines
NikkiWines requested changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines 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! One minor comment

src/libs/actions/Report.js Outdated
}

const firstReportID = _.first(reportIDs);
const currentReportID = firstReportID ? String(firstReportID) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be '' instead of 0 to maintain consistency with our reportID types?

Also got this error when testing on a new account:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I'll fix this!

@marcaaron
Copy link
Contributor Author

Updated

NikkiWines
NikkiWines previously approved these changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a comment •
edited
Loading

Choose a reason for hiding this comment

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

:shipit: (oh except conflicts)

NikkiWines
NikkiWines approved these changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

:shipit: :shipit:

@NikkiWines NikkiWines merged commit 87c1540 into master Mar 17, 2021
@NikkiWines NikkiWines deleted the marcaaron-fixRundefined branch March 17, 2021 17:49
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NikkiWines NikkiWines NikkiWines approved these changes

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marcaaron @NikkiWines