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

Do not append & to internal Expenisfy URLs that have a # symbol. #9450

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

johnmlee101
Copy link
Contributor

@johnmlee101 johnmlee101 commented Jun 14, 2022

Details

Some internal links use hash params which don't expect ? in the URL. Instead they should be continued via & so this will fix the issue of some internal links appending data like the token to data fields rather than as a new param.

Fixed Issues

$ #9416

Tests

IF TESTING ON STAGING REPLACE www WITH staging IN THE URL

  1. Send a message with the following contents:
    https://www.expensify.com/api.php
    https://www.expensify.com/api.php?test=1
    https://www.expensify.com/api.php#123
    https://www.expensify.com/_devportal/tools/logSearch/#sort=asc&size=10000&query=blob%3A%22Expensiworks_CaptureAgentActivity%22%20AND%20blob%3A%22isDoneWorking%3A%20'true'%22%20AND%20email%3A%22monte%40expensify.com%22%20AND%20timestamp%3A%5B2022-05-25T12%3A00%20TO%202022-05-25T14%3A00%5D

  2. See that clicking each will result in the following in the end-point URL

https://www.expensify.com/api.php

AuthToken is appended with ?authToken=

https://www.expensify.com/api.php?test=1

AuthToken is appended with &authToken=

https://www.expensify.com/api.php#123

AuthToken is appended with &authToken=

https://www.expensify.com/_devportal/tools/logSearch/#sort=asc&size=10000&query=blob%3A%22Expensiworks_CaptureAgentActivity%22%20AND%20blob%3A%22isDoneWorking%3A%20'true'%22%20AND%20email%3A%22monte%40expensify.com%22%20AND%20timestamp%3A%5B2022-05-25T12%3A00%20TO%202022-05-25T14%3A00%5D

AuthToken is appended with &authToken= as is not part of the log request

  • Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-06-14.at.10.47.11.AM.mov

Mobile Web

Desktop

iOS

Android

@johnmlee101 johnmlee101 self-assigned this Jun 14, 2022
@johnmlee101 johnmlee101 marked this pull request as ready for review June 14, 2022 14:51
@johnmlee101 johnmlee101 requested a review from a team as a code owner June 14, 2022 14:51
@melvin-bot melvin-bot bot requested review from aldo-expensify and removed request for a team June 14, 2022 14:52
@johnmlee101
Copy link
Contributor Author

Bump!

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Sorry for the slowness @johnmlee101

This solutions considers # and ? equivalents when appending the new param authToken. I understand the reasoning for the logsearch url because it weirdly uses # as a start for paramenters, but this is not the normal thing.

This could break if there was an olddot url using # to make you scroll to a specific node, but maybe we never do that? if we never do that, this fix should be fine.

@johnmlee101
Copy link
Contributor Author

This could break if there was an olddot url using # to make you scroll to a specific node, but maybe we never do that? if we never do that, this fix should be fine.

Yeah I scanned for any URLs that link to olddot baked into Expensify/App already and couldn't find any URLs that used # in that way, so I thought this was safe enough to implement.

@aldo-expensify aldo-expensify merged commit 3546820 into main Jun 20, 2022
@aldo-expensify aldo-expensify deleted the john-hash-url-internal branch June 20, 2022 19:27
@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

🚀 Deployed to staging by @aldo-expensify in version: 1.1.78-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @sketchydroide in version: 1.1.78-8 🚀

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

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.

3 participants