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

[No QA] Remove inline script tag from help site #15515

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

roryabraham
Copy link
Contributor

Details

Removes inline script tag from help site to make it easier to implement a CSP.

Fixed Issues

$ (partial) https://github.com/Expensify/Expensify/issues/264123#issuecomment-1441252931

Tests

  1. Open any HelpDot page.
  2. Verify that the copywrite appears in the footer as it did before.
  • Verify that no errors appear in the JS console

Offline tests

None.

QA Steps

Marking this as No QA as long as HelpDot deploys are broken.

  • Verify that no errors appear in the JS console

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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

image

@roryabraham roryabraham self-assigned this Feb 27, 2023
@roryabraham roryabraham requested a review from a team as a code owner February 27, 2023 19:57
@melvin-bot melvin-bot bot requested review from hayata-suenaga and removed request for a team February 27, 2023 19:58
@MelvinBot
Copy link

@hayata-suenaga 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]

Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

I've got context on the CSP issue, so I figured I'd take a look at this.

@Luke9389
Copy link
Contributor

Luke9389 commented Feb 27, 2023

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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@Luke9389
Copy link
Contributor

I restarted test (job 3). I find that it either takes a super long time or is getting stuck.

Copy link
Contributor

@hayata-suenaga hayata-suenaga left a comment

Choose a reason for hiding this comment

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

Although I don't have much context for this change (except the part that the change is related to making Help Site CSP compliant), the code change LGTM!

@nathanmetcalf nathanmetcalf merged commit 196e6d7 into main Feb 28, 2023
@nathanmetcalf nathanmetcalf deleted the Rory-RemoveInlineHelpDotScript branch February 28, 2023 01:15
@melvin-bot melvin-bot bot added the Emergency label Feb 28, 2023
@MelvinBot
Copy link

@nathanmetcalf looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@nathanmetcalf
Copy link
Contributor

Not an emergency, restarting the test now.

@roryabraham
Copy link
Contributor Author

The tests passed and the job just hanged indefinitely (I think due to a GitHub bug, there's an issue for it somewhere)

@nathanmetcalf
Copy link
Contributor

OK, Thanks Rory :)

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 608.919 ms → 667.608 ms (+58.688 ms, +9.6%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 608.919 ms
Stdev: 22.229 ms (3.7%)
Runs: 570.3098139995709 574.5020350003615 581.1762699997053 587.0197350000963 588.4075519992039 589.8318689996377 591.4144289996475 592.4928380008787 593.6308190003037 595.2668459992856 595.4650879995897 598.1374519998208 600.5575769999996 601.3270669998601 602.1742759998888 604.9610600005835 605.9086109995842 608.0382090006024 608.356241999194 608.9303800007328 610.5546880001202 620.875041000545 621.2580160005018 624.5839849999174 624.6542560001835 626.1386720007285 628.0330400001258 629.1035979995504 638.4680180000141 639.7765309996903 650.1553139993921 673.9136969996616

Current
Mean: 667.608 ms
Stdev: 22.715 ms (3.4%)
Runs: 618.437826000154 619.1520179994404 619.9827469997108 630.8202719995752 647.2211919995025 652.8620609994978 657.8411869993433 660.2219239994884 660.2893890002742 660.3008220000193 661.1217459999025 662.062947999686 663.9869389999658 664.7115479996428 665.4381099995226 668.3068029992282 669.391194999218 669.6024580001831 669.8296719994396 672.6574300006032 675.2571210004389 675.6219069994986 676.1525879995897 678.7512619998306 681.640788000077 690.8051349995658 691.2554519996047 691.8697100002319 696.8223059996963 696.9698490006849 701.1167400004342 712.9444580003619

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 712.243 ms → 721.948 ms (+9.705 ms, +1.4%)
App start runJsBundle 197.290 ms → 197.484 ms (+0.194 ms, ±0.0%)
App start regularAppStart 0.014 ms → 0.014 ms (+0.001 ms, +5.2%)
App start nativeLaunch 20.188 ms → 19.414 ms (-0.774 ms, -3.8%)
Show details
Name Duration
App start TTI Baseline
Mean: 712.243 ms
Stdev: 26.758 ms (3.8%)
Runs: 669.2865209998563 673.8647159999236 674.215389999561 678.9930990003049 683.9152220003307 685.7263150000945 691.0450449995697 694.6770470002666 694.8841739995405 697.9768390003592 701.319466999732 701.7299640001729 702.1285509997979 702.4136300003156 706.8635459998623 708.9644320001826 709.0055999998003 710.6739090001211 717.2937909998 718.1742099998519 721.5699330000207 725.747181000188 725.747728000395 728.9112579999492 732.1575050000101 734.271254000254 744.276828000322 746.8527140002698 751.0658870004117 760.329946000129 785.4561750004068

Current
Mean: 721.948 ms
Stdev: 35.993 ms (5.0%)
Runs: 667.3186520002782 671.5314429998398 682.0938969999552 684.0978920003399 684.3059470001608 689.7951840003952 689.9938669996336 694.5884320000187 694.7185530001298 696.21643400006 696.9711699998006 699.6279990002513 702.7730080001056 710.07012499962 712.7613570000976 717.7064629998058 718.2932280004025 719.9810480000451 721.1128690000623 727.0306200003251 733.9131230004132 737.7097810003906 750.363672000356 750.4556409996003 755.6813420001417 760.5093329995871 762.0663860002533 772.7057119999081 777.8901540003717 785.4676729999483 812.646362000145
App start runJsBundle Baseline
Mean: 197.290 ms
Stdev: 20.771 ms (10.5%)
Runs: 163 164 173 175 175 178 179 181 182 186 187 187 188 190 193 194 196 199 205 207 208 210 212 212 212 213 214 217 218 246 252

Current
Mean: 197.484 ms
Stdev: 20.431 ms (10.3%)
Runs: 170 171 178 179 179 180 180 182 182 183 184 184 186 187 188 192 194 195 196 199 200 211 211 211 214 220 220 222 231 246 247
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.7%)
Runs: 0.012491000816226006 0.012653999961912632 0.012736000120639801 0.01285799965262413 0.012899000197649002 0.012938999570906162 0.013021000660955906 0.013102000579237938 0.013306000269949436 0.01355000026524067 0.013590999878942966 0.013631000183522701 0.013631999492645264 0.013712999410927296 0.01371300034224987 0.013753000646829605 0.013794000260531902 0.0138349998742342 0.0139979999512434 0.014159999787807465 0.014159999787807465 0.014241999946534634 0.014363999478518963 0.014485999941825867 0.014526000246405602 0.01464799977838993 0.014810999855399132 0.015502999536693096 0.015544000081717968

Current
Mean: 0.014 ms
Stdev: 0.001 ms (6.4%)
Runs: 0.013102000579237938 0.013345999643206596 0.013427999801933765 0.013469000346958637 0.01355000026524067 0.01375299971550703 0.01375299971550703 0.013793999329209328 0.013794000260531902 0.013794000260531902 0.013835000805556774 0.013956000097095966 0.014079000800848007 0.014118999242782593 0.014241999946534634 0.0143630001693964 0.014364000409841537 0.01444500032812357 0.01448499970138073 0.014526000246405602 0.014527000486850739 0.014607000164687634 0.014688999392092228 0.014892000705003738 0.015056000091135502 0.015381000004708767 0.01558500062674284 0.015665999613702297 0.01590999960899353 0.016276000067591667 0.01648000068962574 0.016561000607907772
App start nativeLaunch Baseline
Mean: 20.188 ms
Stdev: 1.845 ms (9.1%)
Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 21 21 21 21 21 21 21 22 22 22 23 24 24 24

Current
Mean: 19.414 ms
Stdev: 1.543 ms (7.9%)
Runs: 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 22 23 23

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Feb 28, 2023
@github-actions
Copy link
Contributor

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

🚀 Deployed to staging by https://github.com/nathanmetcalf in version: 1.2.78-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

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
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants