Fix: urls for user details by parasharrajat · Pull Request #4291 · Expensify/App

parasharrajat · 2021-07-29T00:08:29Z

Details

This issue was caused due to the reason that Url Path had . which is not being parsed in React-navigation. I have also reported the issue to React-navigation but nobody is even bothering to reply so we have to fix it with a patch in our app. I will submit a new PR as soon as I got some leads on that issue which is very unlikely.

To fix, PR does the following:

Covert the route param :login to query param ?:login=xxx which fixes the issue. and routes are parsed successfully.
e.g. https://staging.expensify.cash/details/:login to https://staging.expensify.cash/details?login=:login

Fixed Issues

$ Fixes #3924

Tests | QA Steps

Open Any chat in E.cash on web.
Click user avatar to see its details.
refresh the page, you should land on the same page and app should not crash.

Tested On

Screenshots

Web

fix-url.mp4

Mobile Web

Desktop

iOS

Android

chiragsalian · 2021-07-29T01:13:41Z

src/ROUTES.js

+
+    /**
+     * React Navigation is failing to parse urls with dot in the path segment.
+     * This method enable pasring of urls with dot in path Segment


NAB: switch 'enable' with 'enables'.
switch 'pasring' with 'parsing'.

chiragsalian · 2021-07-29T01:16:05Z

src/ROUTES.js

+     * @param {String} path - Path for the config
+     * @returns {Object}
+     */
+    getEmailRouteLinkingConfig: path => ({


In the PR details you explain why you moved away from the initial solution but i don't quite understand the current solution. Can you add it to your PR or explain it to me.

@chiragsalian We can customize the default param parsing by using the parse and stringify option for LinkingConfig.

In stringify, I replace all the dots with @dot@ safely. we first encode the existing @dot@ in the URL and then do the dot conversion. Which will allow @dot@ to be part of the email.

In parse, we revert to the original email by first converting @dot@ back to dot and then replace the escaped version of @dot@ back to @dot@.

This is a helper method as we need the same functionality for two of the routes and it can be used in the future for similar Routes that contain email as param.

Any thoughts @chiragsalian.

Ah nice, i had no idea. I just read more about this here too. Well it makes sense to me and i think its fine, but i'll loop in @marcaaron too for additional thoughts.

@marcaaron Do you have any thoughts on this issue?

Hey @marcaaron What do you think about it? Sorry for the ping.

Is there context on why we using @dot@?

I'm familiar with the url parsing features of react navigation so that part seems fine to me.

marcaaron · 2021-08-02T17:17:06Z

src/ROUTES.js

+     * @param {String} path - Path for the config
+     * @returns {Object}
+     */
+    getEmailRouteLinkingConfig: path => ({


Is there context on why we using @dot@?

marcaaron · 2021-08-02T17:17:30Z

src/ROUTES.js

+     * @param {String} path - Path for the config
+     * @returns {Object}
+     */
+    getEmailRouteLinkingConfig: path => ({


I'm familiar with the url parsing features of react navigation so that part seems fine to me.

marcaaron · 2021-08-02T17:17:58Z

src/ROUTES.js

+        path,
+        parse: {
+            login: l => (l
+                ? decodeURIComponent(l).replace('@dot@', '.').replace(encodeURIComponent('@dot@'), '@dot@')


What does the second .replace() achieve here? Didn't we replace with . already?

I added this for safe parsing. In case the email already contains @dot@ e.g. rajat"@dot@"parashar@gmail.com. Please let me know if I have forgotten to encounter any special cases.

@dot@ I just took it based on the meaning. dot for . and @ is a special URL character that allows us to safely convert emails that can contain @dot@ as part of the string.
@ will be escaped and thus we can use it.

In case the email already contains @dot@ e.g. rajat"@dot@"parashar@gmail.com. Please let me know if I have forgotten to encounter any special cases.

Sorry, not following. Why would a valid email have more than one @ let alone an @dot@ in it?

Ok. I saw somewhere that @ can be used in quotes. I believe I overthink it. I 'll remove the extra escaping.

@marcaaron Updated the PR. Removed the Extra escaping stuff.

marcaaron · 2021-08-02T17:20:31Z

src/ROUTES.js

+                : undefined),
+        },
+        stringify: {
+            login: l => (l ? l.replace('@dot@', encodeURIComponent('@dot@')).replace('.', '@dot@') : undefined),


The logic here is also confusing...

l .replace('@dot@', encodeURIComponent('@dot@')) .replace('.', '@dot@')

We are replacing the @dot@ with %40dot%40

Then we replace all . with @dot@...

Why?

Maybe, I have answered it above. First replace will escape the existing @dot@ from email and then we can use @dot@ to convert ..

marcaaron · 2021-08-02T17:23:11Z

@parasharrajat Is there some more context on the @dot@ addition? URLs are going to look pretty weird after we do this no? Just wondering if we checked with anyone whether the weird looking urls are what we want.

parasharrajat · 2021-08-02T17:33:57Z

I checked out on google, Using emails as URL is not a recommended thing as emails are considered private data. So if we use them in URL then it is kind of public. But we are not using any userIds so It's kind of good that the URL does not look like containing emails and bots can't read emails from it.
Thoughts 🤔 ?

marcaaron · 2021-08-02T19:29:11Z

I checked out on google, Using emails as URL is not a recommended thing as emails are considered private data. So if we use them in URL then it is kind of public. But we are not using any userIds so It's kind of good that the URL does not look like containing emails and bots can't read emails from it.

The emails are easily accessible. What do we achieve by obscuring them in the URL only?
The app is used to converse with people you know and work with not random people. Maybe that will change in the future, but it's not a problem today.

we are not using any userIds so It's kind of good that the URL does not look like containing emails and bots can't read emails from it

I'm not sure this is a problem we need to solve right now, but could also not be understanding which bots we should be worried about.

To clarify, my concern is that the url looks weird and I don't see any discussion about how we landed on @dot@ or other alternatives. Is there another way to do this? e.g. query params vs. path?

parasharrajat · 2021-08-02T20:33:42Z

Yeah, we can do this we Query param. This was the original Idea. But Route will not be strictly matched with the Query param.
for example, /details is equal to /details?user=xxxx;

We can definitely use query params. Let me know? Thanks.

marcaaron · 2021-08-02T22:01:33Z

We could maybe bring this up in Slack? I feel others may have opinions to share.

parasharrajat · 2021-08-02T22:52:11Z

Ok @marcaaron after discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1627942434082700. I have used query params.

parasharrajat · 2021-08-03T18:54:57Z

@marcaaron This is ready. Please let me know if there is something that needs to be changed.

marcaaron · 2021-08-03T18:58:15Z

Thanks for the bump! Can we fix up the description so it reflects the final decision that was made?

parasharrajat · 2021-08-03T19:03:30Z

Done. Thanks for mentioning that.

chiragsalian · 2021-08-04T01:48:52Z

Did you retest? The moment i click on an avatar i get an error,

Okay looks like normal emails work. I got the failure with the email chirag+15@expensify.com which is a pretty normal test email to have. Would you be able to address this?

parasharrajat · 2021-08-04T06:57:20Z

@chiragsalian Yes I retested it but I didn't test the URL with Plus. I have fixed this issue.
I think escaping is not happening for query params so I have added it myself and tested it.

chiragsalian

Nice, LGTM and works great 👍
@marcaaron all yours.

OSBotify · 2021-08-04T17:45:32Z

✋ 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 · 2021-08-06T03:17:10Z

🚀 Deployed to staging in version: 1.0.82-8🚀

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

OSBotify · 2021-08-11T22:17:36Z

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

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

fix: urls for user details

840d6ae

parasharrajat requested a review from a team as a code owner July 29, 2021 00:08

MelvinBot requested review from chiragsalian and removed request for a team July 29, 2021 00:08

chiragsalian reviewed Jul 29, 2021

View reviewed changes

marcaaron requested changes Aug 2, 2021

View reviewed changes

remove unnecessary escaping

c49304c

use query params for login routes

9c871a5

marcaaron assigned parasharrajat Aug 3, 2021

esacpe chars in param

4474fea

chiragsalian approved these changes Aug 4, 2021

View reviewed changes

marcaaron approved these changes Aug 4, 2021

View reviewed changes

marcaaron merged commit 8c06c02 into Expensify:main Aug 4, 2021

This was referenced Aug 5, 2021

Deploy Checklist: Expensify.cash 2021-07-30 #4333

Closed

Deploy Checklist: Expensify.cash 2021-08-06 #4447

Closed

parasharrajat deleted the userdetails branch November 20, 2023 13:07

Conversation

parasharrajat commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Aug 2, 2021

Uh oh!

parasharrajat commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Aug 2, 2021

Uh oh!

parasharrajat commented Aug 2, 2021

Uh oh!

marcaaron commented Aug 2, 2021

Uh oh!

parasharrajat commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parasharrajat commented Aug 3, 2021

Uh oh!

marcaaron commented Aug 3, 2021

Uh oh!

parasharrajat commented Aug 3, 2021

Uh oh!

chiragsalian commented Aug 4, 2021

Uh oh!

parasharrajat commented Aug 4, 2021

Uh oh!

chiragsalian left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Aug 4, 2021

parasharrajat commented Jul 29, 2021 •

edited

Loading

parasharrajat Jul 29, 2021 •

edited

Loading

parasharrajat commented Aug 2, 2021 •

edited

Loading

parasharrajat commented Aug 2, 2021 •

edited

Loading