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 navigating to reports via Search page on staging #2547

Merged
merged 2 commits into from
Apr 23, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Apr 22, 2021

Details

Fixes the broken Search page on staging.

Fixed Issues

Fixes #2538

Tests

QA Steps

  1. Tap on a chat (mobile web)
  2. Go back to the LHN
  3. Tap on another chat
  4. Test the browser back button behavior and verify that we do not get navigated to an empty screen
  5. Open the /search screen and select a new chat
  6. Verify you are able to navigate to that chat by tapping it
  7. Find and locate a group DM chat in the LHN and tap it
  8. Verify that you can also bring up a /r/<reportID>/participants route by tapping on a group chat header avatar

Note: The browser button works unexpectedly if we navigate to a new chat via the "Search" page. Considering that to be not a blocker for now, but will keep looking into it in the future.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron requested a review from a team as a code owner April 22, 2021 23:51
@marcaaron marcaaron self-assigned this Apr 22, 2021
@MelvinBot MelvinBot requested review from NikkiWines and removed request for a team April 22, 2021 23:51
@marcaaron marcaaron changed the title Undo some bad changes Fix navigating to reports via Search page on staging Apr 22, 2021
}

return `/${newUrl}`;
}
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 don't know why I did this to begin with. All the routes start without a slash.

@marcaaron
Copy link
Contributor Author

Leaving out the screenshots because there are no visual changes but I did test on all platforms to verify things work OK. @NikkiWines lmk if there's anything that should be explained better - planning to do a follow up after this since I'm not happy with where things are at but staging is borked. Thanks!!!!

@NikkiWines NikkiWines merged commit 7d1188b into main Apr 23, 2021
@NikkiWines NikkiWines deleted the marcaaron-fixWeirdNavigationIssue branch April 23, 2021 19:56
@OSBotify
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

return {};
}

const pathSegments = route.split('/');
return {
reportID: lodashGet(pathSegments, 1),
isParticipantsRoute: Boolean(lodashGet(pathSegments, 2)),
Copy link
Member

Choose a reason for hiding this comment

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

Coming from issue - app crash when url is invalid

If the url has multiple double slash eg: https://.....https//, pathSegments[2] will be false.
We should have simply checked the length of the array

More details here - #22507 (comment)

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.

Search feature is not working
4 participants