Skip to content

Improve website validation in adding bank account process#14364

Merged
stitesExpensify merged 3 commits into
mainfrom
monil-updateURLRegex
Jan 18, 2023
Merged

Improve website validation in adding bank account process#14364
stitesExpensify merged 3 commits into
mainfrom
monil-updateURLRegex

Conversation

@MonilBhavsar

@MonilBhavsar MonilBhavsar commented Jan 17, 2023

Copy link
Copy Markdown
Contributor

Details

Fixed Issues

$ #14259
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

  1. Go to Workspace > Connect Bank Account > Connect Manually
  2. Complete Step 1
  3. On Company information Step 2
  4. In the company website input, verify the hint (e.g. https://www.expensify.com) and also for spanish language
  5. Enter a set of valid and invalid websites and confirm it works

Offline tests

Same as tests

QA Steps

  1. Go to Workspace > Connect Bank Account > Connect Manually
  2. Complete Step 1
  3. On Company information Step 2
  4. In the company website input, verify the hint (e.g. https://www.expensify.com) and also for spanish language
  5. Enter a set of valid and invalid websites and confirm it works

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (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 is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • 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
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-01-18.at.10.52.16.AM.mov
Screenshot 2023-01-18 at 10 40 00 AM
Mobile Web - Chrome
Mobile Web - Safari
Desktop Screenshot 2023-01-18 at 11 23 27 AM
iOS
Android

@MonilBhavsar MonilBhavsar self-assigned this Jan 17, 2023
@MonilBhavsar MonilBhavsar marked this pull request as ready for review January 18, 2023 05:24
@MonilBhavsar MonilBhavsar requested a review from a team as a code owner January 18, 2023 05:24
@melvin-bot melvin-bot Bot requested review from sobitneupane and stitesExpensify and removed request for a team January 18, 2023 05:24
@melvin-bot

melvin-bot Bot commented Jan 18, 2023

Copy link
Copy Markdown

@sobitneupane @stitesExpensify One of you needs to 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]

@sobitneupane

sobitneupane commented Jan 18, 2023

Copy link
Copy Markdown
Contributor

@MonilBhavsar The recent change will block the user to enter websites without application layer protocol(like: 'https://'). Is this an intended behavior? I think we should allow users to enter websites even without application layer protocol(like: 'www.expensify.com')

@MonilBhavsar

Copy link
Copy Markdown
Contributor Author

The recent change will block the user to enter websites without application layer protocol(like: 'https://'). Is this an intended behavior?

Yes, right now, if you enter website without protocol, frontend will allow, but backend will deny it.
So, we also make frontend validate it

@sobitneupane sobitneupane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • 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 is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-01-18.at.16.01.55.mov
Mobile Web - Chrome
Screen.Recording.2023-01-18.at.16.11.02.mov
Mobile Web - Safari
Screen.Recording.2023-01-18.at.16.17.13.mov
Desktop
Screen.Recording.2023-01-18.at.16.07.20.mov
iOS
Screen.Recording.2023-01-18.at.16.13.19.mov
Android
Screen.Recording.2023-01-18.at.16.21.08.mov

@stitesExpensify stitesExpensify merged commit 7b2fd32 into main Jan 18, 2023
@stitesExpensify stitesExpensify deleted the monil-updateURLRegex branch January 18, 2023 16:15
@OSBotify

Copy link
Copy Markdown
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.

@github-actions

Copy link
Copy Markdown
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 681.973 ms → 715.245 ms (+33.272 ms, +4.9%)
Open Search Page TTI 597.002 ms → 604.898 ms (+7.895 ms, +1.3%)
App start runJsBundle 189.594 ms → 196.484 ms (+6.890 ms, +3.6%)
App start nativeLaunch 19.571 ms → 19.724 ms (+0.153 ms, +0.8%)
App start regularAppStart 0.015 ms → 0.015 ms (+0.001 ms, +3.8%)
Show details
Name Duration
App start TTI Baseline
Mean: 681.973 ms
Stdev: 36.156 ms (5.3%)
Runs: 618.0319469999522 621.2434219997376 629.0555169992149 629.899095999077 643.3693090006709 648.5559330005199 653.0989360008389 655.9252630006522 657.9003459997475 661.3925379998982 661.7162539996207 665.8310250006616 667.7230019997805 669.992872999981 677.5890800002962 678.9329320006073 683.1512129995972 685.7328830007464 685.893836999312 689.7863679993898 690.0109989997 690.3367279991508 699.7827989999205 705.7673380002379 711.1182279996574 715.7189819999039 722.0932839997113 731.3557459991425 733.3642570003867 739.3579629994929 741.8971510007977 757.5015539992601

Current
Mean: 715.245 ms
Stdev: 29.825 ms (4.2%)
Runs: 665.1668699998409 675.3566859997809 678.5142230000347 679.9290309995413 680.5312610007823 680.5913200005889 682.0709530003369 694.3565849997103 697.5652409996837 701.8458450008184 703.8227210007608 704.7843490000814 705.4617779999971 709.462833000347 710.2998210005462 711.6086270008236 712.0659560002387 714.221115000546 714.7370379995555 717.339958999306 724.7708870004863 728.233164999634 729.9938440006226 731.5885160006583 731.8103979993612 750.6350820008665 759.2119239997119 761.1648120004684 766.4984729997814 768.6851410008967 780.2611929997802
Open Search Page TTI Baseline
Mean: 597.002 ms
Stdev: 16.119 ms (2.7%)
Runs: 561.8668210003525 566.363119000569 573.1638190001249 576.7109779994935 576.8114020004869 580.8430989999324 581.1993819996715 583.315755000338 584.2467860002071 585.1778159998357 587.2240000013262 588.8547370005399 589.1353769991547 594.4246429987252 596.0261639989913 600.0256760008633 600.8675539996475 601.6469729989767 602.7890629991889 603.3952240012586 605.8012700006366 606.4460039995611 608.2496750000864 609.1646730005741 609.8085939995944 610.2737639993429 612.1331380009651 612.5888679996133 614.7994389999658 616.3194180000573 616.8657229989767 617.4315600004047 627.1101890001446

Current
Mean: 604.898 ms
Stdev: 20.219 ms (3.3%)
Runs: 565.437541000545 571.4085699990392 580.8084319997579 581.4132890012115 582.2670089993626 588.5758460015059 589.5248619988561 591.8506670016795 592.1957200001925 593.816324999556 593.925008000806 598.8462729994208 599.4495860002935 599.5580650009215 603.2492270004004 604.6506349984556 604.7397060003132 605.4776210002601 607.2097169999033 608.8308920003474 611.4860839992762 612.569499000907 614.1612139996141 615.4748129993677 615.6243900004774 618.6402179989964 626.6084390003234 635.6208100002259 641.487549001351 647.9172769989818 649.0052490010858
App start runJsBundle Baseline
Mean: 189.594 ms
Stdev: 19.181 ms (10.1%)
Runs: 159 160 160 164 164 167 171 174 174 177 180 182 182 183 188 191 191 191 194 195 198 199 199 206 207 208 209 210 214 222 223 225

Current
Mean: 196.484 ms
Stdev: 18.224 ms (9.3%)
Runs: 175 175 176 177 178 180 181 182 183 183 183 186 187 188 192 192 195 196 198 200 202 205 207 209 211 213 216 218 219 230 254
App start nativeLaunch Baseline
Mean: 19.571 ms
Stdev: 1.425 ms (7.3%)
Runs: 17 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 21 22 23

Current
Mean: 19.724 ms
Stdev: 1.310 ms (6.6%)
Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 21 21 22 23
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (7.0%)
Runs: 0.012897999957203865 0.013387000188231468 0.01342800073325634 0.013590000569820404 0.013591000810265541 0.013671999797224998 0.013793999329209328 0.013875000178813934 0.013875000178813934 0.013916000723838806 0.014159999787807465 0.014240998774766922 0.014241000637412071 0.014525998383760452 0.01452699862420559 0.014567999169230461 0.01464799977838993 0.014689000323414803 0.014689000323414803 0.014810999855399132 0.014932999387383461 0.014933999627828598 0.015014998614788055 0.015095999464392662 0.015177000313997269 0.015828000381588936 0.015949999913573265 0.016804998740553856 0.017781000584363937

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.8%)
Runs: 0.013670999556779861 0.013915998861193657 0.0139979999512434 0.01407800056040287 0.014119001105427742 0.014241000637412071 0.014241999015212059 0.014444999396800995 0.014526000246405602 0.014607999473810196 0.01477000117301941 0.014973999932408333 0.015015000477433205 0.01505500078201294 0.015095999464392662 0.015095999464392662 0.015137000009417534 0.015257999300956726 0.01534000039100647 0.015340998768806458 0.015461999922990799 0.015583999454975128 0.015625 0.0157880000770092 0.01586899906396866 0.01586899906396866 0.01590999960899353 0.016439000144600868 0.016764000058174133 0.017578000202775

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @stitesExpensify in version: 1.2.56-0 🚀

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

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.56-0 🚀

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

@sobitneupane

Copy link
Copy Markdown
Contributor

@MonilBhavsar Payment is due for C+ review. #14259 is locked so could not comment in the issue.

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.

4 participants