-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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: urls for user details #4291
Conversation
src/ROUTES.js
Outdated
|
||
/** | ||
* React Navigation is failing to parse urls with dot in the path segment. | ||
* This method enable pasring of urls with dot in path Segment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: switch 'enable' with 'enables'.
switch 'pasring' with 'parsing'.
src/ROUTES.js
Outdated
* @param {String} path - Path for the config | ||
* @returns {Object} | ||
*/ | ||
getEmailRouteLinkingConfig: path => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts @chiragsalian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron Do you have any thoughts on this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @marcaaron What do you think about it? Sorry for the ping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there context on why we using @dot@
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm familiar with the url parsing features of react navigation so that part seems fine to me.
src/ROUTES.js
Outdated
* @param {String} path - Path for the config | ||
* @returns {Object} | ||
*/ | ||
getEmailRouteLinkingConfig: path => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there context on why we using @dot@
?
src/ROUTES.js
Outdated
* @param {String} path - Path for the config | ||
* @returns {Object} | ||
*/ | ||
getEmailRouteLinkingConfig: path => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm familiar with the url parsing features of react navigation so that part seems fine to me.
src/ROUTES.js
Outdated
path, | ||
parse: { | ||
login: l => (l | ||
? decodeURIComponent(l).replace('@dot@', '.').replace(encodeURIComponent('@dot@'), '@dot@') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the second .replace()
achieve here? Didn't we replace with .
already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I saw somewhere that @ can be used in quotes. I believe I overthink it. I 'll remove the extra escaping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron Updated the PR. Removed the Extra escaping stuff.
src/ROUTES.js
Outdated
: undefined), | ||
}, | ||
stringify: { | ||
login: l => (l ? l.replace('@dot@', encodeURIComponent('@dot@')).replace('.', '@dot@') : undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I have answered it above. First replace will escape the existing @dot@
from email and then we can use @dot@
to convert .
.
@parasharrajat Is there some more context on the |
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. |
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 |
Yeah, we can do this we Query param. This was the original Idea. But Route will not be strictly matched with the Query param. We can definitely use query params. Let me know? Thanks. |
We could maybe bring this up in Slack? I feel others may have opinions to share. |
Ok @marcaaron after discussion https://expensify.slack.com/archives/C01GTK53T8Q/p1627942434082700. I have used query params. |
@marcaaron This is ready. Please let me know if there is something that needs to be changed. |
Thanks for the bump! Can we fix up the description so it reflects the final decision that was made? |
Done. Thanks for mentioning that. |
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? |
@chiragsalian Yes I retested it but I didn't test the URL with Plus. I have fixed this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM and works great 👍
@marcaaron all yours.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging in version: 1.0.82-8🚀
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
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:
:login
to query param?:login=xxx
which fixes the issue. and routes are parsed successfully.e.g.
https://staging.expensify.cash/details/:login
tohttps://staging.expensify.cash/details?login=:login
Fixed Issues
$ Fixes #3924
Tests | QA Steps
Tested On
Screenshots
Web
fix-url.mp4
Mobile Web
Desktop
iOS
Android