Fix/11810 old dot link without w3 not auto signin #12701

tienifr · 2022-11-14T07:32:44Z

Details

In new dot Expensify, when sending old dot link without ‘www.’ prefix it is redirecting to enter password screen and expectation is automatically signed in to old dot.. This PR will fix that

Fixed Issues

$ #11810
$ #11810 (comment)

Tests

Login to new dot expensify.
open any chat and send links such as

Press the link and verify that you are automatically signed in to old dot when entering all the above URL

Verify that no errors appear in the JS console

Offline tests

QA Steps

Login to new dot expensify.
open any chat and send links such as

Press the link and verify that you are automatically signed in to old dot when entering all the above URL

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Screen.Recording.2022-11-14.at.17.15.00.mp4

Mobile Web - Chrome

old.mp4

Mobile Web - Safari

Can not click URL on Safari (even though in main branch)

Desktop

Screen.Recording.2022-11-14.at.16.58.56.mp4

iOS

Screen.Recording.2022-11-14.at.17.18.29.mp4

Android

Screen.Recording.2022-11-14.at.17.24.24.mp4

melvin-bot · 2022-11-14T07:33:15Z

@Gonals Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/CONFIG.js

src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

src/libs/Url.js

tienifr · 2022-11-15T08:02:10Z

Hi @parasharrajat @MonilBhavsar I just fixed your comments and added unit test. Please help to check again. Thanks

parasharrajat · 2022-11-15T17:47:00Z

I will test it shortly. Can you please look into #12701 (comment)?

parasharrajat · 2022-11-16T15:53:41Z

Bump @tienifr above.

parasharrajat

Add another test.

 it('It should work correctly without https://', () => {
            expect(Url.hasSameOrigin('new.expensify.com/action/1234', 'new.expensify.com/action/123')).toBe(true);
        });

it should also pass.

Let's add tests for all urls in https://github.com/Expensify/expensify-common/blob/dd66d931aeffe18cdcffab8e7d05af32f3ef7ad1/__tests__/ExpensiMark-HTML-test.js#L178 for URL.getURLObject.

you can drop the similar urls.

parasharrajat · 2022-11-16T19:47:17Z

If tests are not passing for these URLs then I feel like URL.getURLObject is not very stable.

It might cause issues in the future and additional maintenance costs to manage it. @MonilBhavsar What do you think? Should we keep it and just fix this issue? Or limit this function to more strict expensify URLs only

MonilBhavsar · 2022-11-17T05:33:59Z

Agree, function should be able to parse all kind of valid URLs. let's add tests for all valid URLs

…ttps://github.com/tienifr/App into fix/11810-old-dot-link-without-w3-not-auto-signin

tienifr · 2022-11-21T07:21:18Z

Hi @MonilBhavsar @parasharrajat sorry about my late. I just added UTs and fixed your comments, please help to check again. Thanks

MonilBhavsar · 2022-11-21T16:23:34Z

src/libs/Url.js

+ */
+function getURLObject(href) {
+    const urlRegex = new RegExp([
+        '^(https?:)//', // protocol


I still feel like we need to update this regex as it doesn't parse URL's without http protocol

can we use the regex from ExpensiMark.js

You wanted to link this - https://github.com/Expensify/expensify-common/blob/dd66d931aeffe18cdcffab8e7d05af32f3ef7ad1/lib/ExpensiMark.js#L5?
If yes, sounds good 👍

Yeah, that one. Thanks.

@parasharrajat @MonilBhavsar Thanks for your review, but I'm afraid that we have no way to import URL_WEBSITE_REGEX from ExpensiMark. Should I duplicate URL_WEBSITE_REGEX and TLD_REGEX or fix on upstream.

I would say fix on upstream. @parasharrajat what do you think?

yeah, feel free to send a PR to expose the data from the e-common repo. https://github.com/Expensify/expensify-common

Create a new File to hold the URL const and import that in ExpensiMark.

tienifr · 2022-12-02T07:52:39Z

Thanks @parasharrajat @MonilBhavsar, I agree with you. Should I split the hostname by dot then ignore www if the result has 3 elements and starts with www? But there're some special cases like: www.example.co.uk. Actually we have several libs to detect the subdomain (pls), but I think adding the new lib is not necessary. Pls let me know your thought.

parasharrajat · 2022-12-02T10:19:02Z

Hmm, these special cases are making it complex. I would say we simply fix this issue and ignore a generic solutions for now.

Let's modify the hasSameOrigin to hasSameExpensifyOrigin and only care about Expensify URLs. in that case, the solution in #12701 (comment) should be fine. cc: @MonilBhavsar

Also, leave a comment specifying the edge case which prevented us from making it generic.

@tienifr Do you still see some cases where this will fail in our app? Please let me know if I am not seeing any.

MonilBhavsar · 2022-12-02T12:19:16Z

Sounds good 👍 Let's just fix this issue with hasSameExpensifyOrigin.

tienifr · 2022-12-02T15:50:53Z

@parasharrajat @MonilBhavsar Thanks for your review, I've changed my PR to update hasSameExpensifyOrigin function and edited some test cases. Pls help review again

parasharrajat · 2022-12-02T16:28:07Z

src/libs/Url.js

+function shouldRemoveW3(hostname) {
+    const parts = hostname.split('.').reverse();
+    const subDomain = parts[2];
+    return subDomain === 'www';
+}


It is good to create a separate function for this but there is not base for the logic used in this. why are we checking parts[2] for subdomain? URL can be very versatile. That's why I suggest you just move this logic inside hasSameExpensifyOrigin so that it is clear that subDomain === 'www' is applicable to Expensify domains.

We have www.new.expensify.com => hostname.split('.').reverse() = [com,expensify,new,www] that why I use parts[2] (new) for subdomain. I'm also afraid of duplicating this logic inside hasSameExpensifyOrigin so I created a separate function.

Can we use shouldRemoveW3FromExpensifyUrl?

Yeah, I didn't notice there are two calls to it. Yeah that will work.

@parasharrajat I've changed my PR. Pls help check, thanks

parasharrajat · 2022-12-02T18:35:01Z

Screenshots

🔲 iOS / native

screen-2022-12-03_00.52.03.mp4

🔲 iOS / Safari

🔲 MacOS / Desktop

screen-2022-12-03_00.40.41.mp4

🔲 MacOS / Chrome

screen-2022-12-02_23.58.25.mp4

🔲 Android / Chrome

screen-2022-12-03_00.05.22.mp4

🔲 Android / native

screen-2022-12-03_00.27.56.mp4

parasharrajat

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @MonilBhavsar

🎀 👀 🎀 C+ reviewed

MonilBhavsar

Also, please add tests to cover broader case. You can add more variations of URL's.

MonilBhavsar · 2022-12-05T13:29:06Z

src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.js

+    const internalNewExpensifyPath = (Url.hasSameExpensifyOrigin(attrHref, CONST.NEW_EXPENSIFY_URL) || Url.hasSameExpensifyOrigin(attrHref, CONST.STAGING_NEW_EXPENSIFY_URL)) && attrPath;
+    const internalExpensifyPath = Url.hasSameExpensifyOrigin(attrHref, CONFIG.EXPENSIFY.EXPENSIFY_URL)
+                                    && !attrPath.startsWith(CONFIG.EXPENSIFY.CONCIERGE_URL_PATHNAME)
+                                    && attrPath;


Check for attrPath before
attrPath && !attrPath.startsWith(CONFIG.EXPENSIFY.CONCIERGE_URL_PATHNAME)

@MonilBhavsar Sorry I don't understand your mean. If we use check attrPath before attrPath && !attrPath.startsWith(CONFIG.EXPENSIFY.CONCIERGE_URL_PATHNAME) => internalExpensifyPath will be boolean

Ah ok, got it

MonilBhavsar · 2022-12-05T13:45:39Z

src/libs/Url.js

+}
+
+/**
+ * Determine if we should remove w3 from hostname


Lets update the comment to answer - Why we want to remove sub domain

MonilBhavsar · 2022-12-05T13:48:37Z

src/libs/Url.js

+ * @param {String} hostname
+ * @returns {Boolean}
+ */
+function shouldRemoveW3FromExpensifyUrl(hostname) {


Ah, also this function is not working as expected, locally. I mean for internal engineers as our local URL has subdomain and can be accessed with both www and now www - expensify.com.dev

@parasharrajat This is exactly what we mentioned before #12701 (comment) and I think we should find the generic solution right?

I'm so appreciated if we had the regex for this case before. If not I'll find by myself. Thanks

I am really confused and not sure how can we fix this problem once and for all. Should we keep a list of URLs that support www and adjust this function?

@MonilBhavsar I think we need internal support here.

I feel like this is an exceptional case and we can add a condition to support this.

@MonilBhavsar You mean if hostname === 'www.expensify.com.dev' we will remove www. first then keep the logic to remove www as before right?

Correct!
Also please leave a comment, why are we doing like this. Something like -

Since expensify.com.dev is accessible with and without www subdomain.

parasharrajat · 2022-12-08T18:20:57Z

cc: @tienifr This is very close to merging. Let's wrap this up.

tienifr · 2022-12-13T03:00:33Z

@parasharrajat @MonilBhavsar Thanks for your help. I've updated my PR to add more tests with local url and update logic to remove w3 from local expensify url

parasharrajat · 2022-12-13T09:11:22Z

@MonilBhavsar All yours..

MonilBhavsar

Also, please add tests to cover broader case. You can add more variations of URL's.

@tienifr please update tests and QA to cover different URL variations and also concierge path case

tienifr · 2022-12-14T03:09:18Z

@MonilBhavsar Please help to check again. Thanks

MonilBhavsar · 2022-12-14T15:54:54Z

Thanks! updated one link - https://new.expensify.com/inbox because there is no inbox in newdot
Also, one thing - If you make an update to PR after long time, please merge main so it is up to date. We're pushing lots of code daily so 😄
Thanks for working on this!!

MonilBhavsar · 2022-12-14T15:55:46Z

The PR author checklist is failing, could you please check again

parasharrajat · 2022-12-14T17:18:45Z

You may have to get the latest PR Author checklist from https://github.com/Expensify/App/blob/main/.github/PULL_REQUEST_TEMPLATE.md and replace PR description with that.

tienifr · 2022-12-15T03:37:06Z

Hi @MonilBhavsar , Please help to check again. Thanks cc @parasharrajat

github-actions · 2022-12-15T12:25:36Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	796.929 ms → 820.327 ms (+23.398 ms, +2.9%)
runJsBundle	184.100 ms → 189.133 ms (+5.033 ms, +2.7%)
regularAppStart	0.014 ms → 0.014 ms (+0.000 ms, +3.4%)
nativeLaunch	11.250 ms → 9.613 ms (-1.637 ms, -14.6%)

Show details

Name	Duration
TTI	Baseline Mean: 796.929 ms Stdev: 29.639 ms (3.7%) Runs: 743.3840520000085 748.942968999967 756.0604469999671 765.8303149999119 768.1067699999548 768.8739839999471 771.546296000015 773.230979999993 773.2596320000011 774.1965020000935 781.2092800000682 790.1560490000993 794.7497360000852 796.3601140000392 797.9662109999917 798.1714860000648 798.5504419999197 800.3707010000944 802.3326870000456 808.042572000064 810.8698519999161 811.5492700000759 812.8470699999016 813.9500609999523 815.7102020001039 826.0884740001056 842.4422689999919 849.1501619999763 853.8666850000154 860.0424049999565 Current Mean: 820.327 ms Stdev: 36.486 ms (4.4%) Runs: 757.6809169999324 762.6218799999915 769.7184480000287 770.9631819999777 780.5091659999453 781.0679999999702 785.2642159999814 793.3821600000374 794.8100399998948 798.2814319999889 798.320582000073 814.1613940000534 814.2513639999088 814.6918860001024 816.2841439999174 817.2938679999206 817.9813530000392 822.5716279998887 823.0588899999857 829.4813260000665 839.2234249999747 839.3141149999574 849.8082449999638 851.5643309999723 852.6075589999091 854.2694109999575 859.3167659998871 860.6292409999296 867.801899000071 884.5755849999841 908.6204019999132
runJsBundle	Baseline Mean: 184.100 ms Stdev: 15.376 ms (8.4%) Runs: 162 162 167 168 168 170 171 171 171 173 175 176 177 178 185 186 186 187 188 189 190 191 191 192 194 200 207 210 218 220 Current Mean: 189.133 ms Stdev: 14.170 ms (7.5%) Runs: 167 174 175 175 178 178 179 179 180 180 181 181 182 182 183 186 189 191 191 193 193 194 195 195 198 202 208 215 221 229
regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (6.5%) Runs: 0.012491999892517924 0.012574000051245093 0.012735999887809157 0.012735999887809157 0.012818000046536326 0.013061999808996916 0.013101999880746007 0.01310200011357665 0.013141999952495098 0.013143000192940235 0.013223999878391623 0.013224000111222267 0.013224000111222267 0.013346000108867884 0.013347000116482377 0.013386999955400825 0.013428000034764409 0.0134680001065135 0.013508999953046441 0.013753999955952168 0.014038000022992492 0.014119999948889017 0.014201000100001693 0.014404000015929341 0.014567000092938542 0.014649000018835068 0.014973999932408333 0.015422000084072351 0.016398000065237284 Current Mean: 0.014 ms Stdev: 0.001 ms (6.9%) Runs: 0.012248000130057335 0.012899000197649002 0.013021000195294619 0.013061000034213066 0.013223999878391623 0.013224000111222267 0.0133050000295043 0.013386999955400825 0.013428000034764409 0.013428000034764409 0.013508999953046441 0.013712000101804733 0.013713000109419227 0.013875000178813934 0.013916000025346875 0.01395700010471046 0.014038000022992492 0.014038000022992492 0.014079000102356076 0.014159999787807465 0.014160000020638108 0.014282000018283725 0.01436399994418025 0.014484999934211373 0.015137000009417534 0.015625 0.015666000079363585 0.01570700015872717 0.015746999997645617 0.01582799991592765 0.016032000072300434
nativeLaunch	Baseline Mean: 11.250 ms Stdev: 2.411 ms (21.4%) Runs: 8 8 8 9 9 9 9 9 9 10 10 10 10 10 10 10 10 11 11 12 12 13 13 13 13 13 14 15 15 15 16 16 Current Mean: 9.613 ms Stdev: 1.452 ms (15.1%) Runs: 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 9 10 10 10 11 11 11 11 11 11 12 12 14

OSBotify · 2022-12-16T03:12:13Z

🚀 Deployed to production by @chiragsalian in version: 1.2.40-3 🚀

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

tienifr added 6 commits November 8, 2022 19:27

fix/11810 old dot link without w3 not auto signin

a69f2e8

Merge branch 'main' of https://github.com/tienifr/App

ac27993

fix: issue

e0a7d2e

fix: issue

42cf9d5

Merge branch 'main' of https://github.com/tienifr/App

5f3c495

fix: issue

cde06f9

tienifr requested a review from a team as a code owner November 14, 2022 07:32

tienifr marked this pull request as draft November 14, 2022 07:32

melvin-bot bot requested review from Gonals and removed request for a team November 14, 2022 07:33

Merge branch 'main' of https://github.com/tienifr/App

d404ec6

tienifr marked this pull request as ready for review November 14, 2022 10:44

MonilBhavsar requested review from MonilBhavsar and parasharrajat November 14, 2022 12:58

parasharrajat suggested changes Nov 14, 2022

View reviewed changes

tienifr added 2 commits November 15, 2022 14:30

fix: comment

59bbf98

fix: issue

dbebaa1

parasharrajat suggested changes Nov 16, 2022

View reviewed changes

fix: issue

6afcc8d

tienifr added 3 commits November 18, 2022 17:20

Merge branch 'fix/11810-old-dot-link-without-w3-not-auto-signin' of h…

e3c41ea

…ttps://github.com/tienifr/App into fix/11810-old-dot-link-without-w3-not-auto-signin

fix: issue

7a128f2

fix: issue

d675c79

MonilBhavsar reviewed Nov 21, 2022

View reviewed changes

fix: change logic to check same origin

f154441

parasharrajat reviewed Dec 2, 2022

View reviewed changes

fix: change function name

79f505a

parasharrajat previously approved these changes Dec 2, 2022

View reviewed changes

MonilBhavsar reviewed Dec 5, 2022

View reviewed changes

fix: add new logic to remove w3 from local domain

f10df84

tienifr dismissed parasharrajat’s stale review via f10df84 December 13, 2022 02:55

parasharrajat approved these changes Dec 13, 2022

View reviewed changes

trjExpensify mentioned this pull request Dec 13, 2022

[HOLD for payment 2022-12-22][$500] Bug: Old dot link without ‘www.’ prefix ( expensify.com/inbox) does not auto signin to old dot reported by @sobitneupane #11810

Closed

MonilBhavsar approved these changes Dec 13, 2022

View reviewed changes

merge: main

be66a92

MonilBhavsar merged commit 1dd41bb into Expensify:main Dec 15, 2022

OSBotify mentioned this pull request Dec 16, 2022

Deploy Checklist: New Expensify 2022-12-16 #13654

Closed

25 tasks

Fix/11810 old dot link without w3 not auto signin #12701

Fix/11810 old dot link without w3 not auto signin #12701

Conversation

tienifr commented Nov 14, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot bot commented Nov 14, 2022

tienifr commented Nov 15, 2022

parasharrajat commented Nov 15, 2022

parasharrajat commented Nov 16, 2022

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Nov 16, 2022 • edited Loading

MonilBhavsar commented Nov 17, 2022

tienifr commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

tienifr commented Dec 2, 2022

parasharrajat commented Dec 2, 2022 • edited Loading

MonilBhavsar commented Dec 2, 2022

tienifr commented Dec 2, 2022

Choose a reason for hiding this comment

tienifr Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tienifr Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Dec 2, 2022 • edited Loading

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

MonilBhavsar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Dec 8, 2022

tienifr commented Dec 13, 2022

parasharrajat commented Dec 13, 2022

MonilBhavsar left a comment

Choose a reason for hiding this comment

tienifr commented Dec 14, 2022

MonilBhavsar commented Dec 14, 2022

MonilBhavsar commented Dec 14, 2022

parasharrajat commented Dec 14, 2022

tienifr commented Dec 15, 2022

tienifr commented Nov 14, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

parasharrajat commented Nov 16, 2022 •

edited

Loading

parasharrajat Nov 22, 2022 •

edited

Loading

parasharrajat commented Dec 2, 2022 •

edited

Loading

tienifr Dec 2, 2022 •

edited

Loading

tienifr Dec 2, 2022 •

edited

Loading

parasharrajat commented Dec 2, 2022 •

edited

Loading