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] Add KSv2.md file #14342

Merged
merged 32 commits into from
Jan 30, 2023
Merged

[No QA] Add KSv2.md file #14342

merged 32 commits into from
Jan 30, 2023

Conversation

marcochavezf
Copy link
Contributor

cc @mallenexpensify @michaelhaxhiu @tgolen

Details

This PR adds the KSv2 file that explains the priority labels and a link to the k2 repo.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/255342

Tests

N/A

  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

N/A

  • 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

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

@marcochavezf marcochavezf requested a review from a team as a code owner January 16, 2023 23:14
@marcochavezf marcochavezf self-assigned this Jan 16, 2023
@melvin-bot melvin-bot bot requested review from mananjadhav and removed request for a team January 16, 2023 23:14
@melvin-bot
Copy link

melvin-bot bot commented Jan 16, 2023

@mananjadhav @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]

@melvin-bot melvin-bot bot requested a review from stitesExpensify January 16, 2023 23:14
@stitesExpensify
Copy link
Contributor

Unassigning @mananjadhav since this is just a documentation change

@stitesExpensify stitesExpensify removed the request for review from mananjadhav January 16, 2023 23:17
@marcochavezf
Copy link
Contributor Author

Posting here to get more feedback about the content.

@stitesExpensify
Copy link
Contributor

stitesExpensify commented Jan 16, 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.
  • 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

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

It's great that this is going public! A few more ideas of things we couldddd mention in this doc:

  1. Is the ksv2 repo open for bug reports? (paid or unpaid?)
    • If yes, where should people report these bugs?
  2. Maybe you can explain the different sections of the k2 dashboard?
  3. Had one more comment I'll throw in slack :D

contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
@marcochavezf
Copy link
Contributor Author

marcochavezf commented Jan 17, 2023

  • Is the ksv2 repo open for bug reports? (paid or unpaid?)

Yup, I think that was mentioned in the doc, but the idea atm is to allow contributors to report bugs in the #expensify-bugs channel and follow the process that we have for NewDot. We can update the bug reports for k2 as follow-up.

  • Maybe you can explain the different sections of the k2 dashboard?

Oh good idea, I think we can begin with what we have in this SO:

To help surface the issues that need the most #urgency, we've built a custom extension to use in GitHub. It looks like this:

In the dashboard, you can see all your issues broken down from urgent to least urgent. Issues will also change color depending on other factors - eg, if they have "HOLD" in the title or if they have the Overdue, Planning, or Waiting for copy labels applied.

Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Adding a little bit of color and explanation. The rest looks great!

Not sure if you want to mention the parts about the pull request sections on the dashboard. Also, it's more than a dashboard, but the extension also provides quite a few other things (like the quick label buttons and it enables the "reviewer checklist" button).

contributingGuides/KSv2.md Show resolved Hide resolved
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Looking pretty great! One more suggested change for now

contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Show resolved Hide resolved
marcochavezf and others added 3 commits January 21, 2023 09:59
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
@marcochavezf
Copy link
Contributor Author

Ready for review again 👍🏽

Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Looking great!

contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
marcochavezf and others added 8 commits January 29, 2023 21:49
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
Co-authored-by: Tim Golen <tgolen@gmail.com>
@marcochavezf
Copy link
Contributor Author

Cool, updated. Thanks for the suggestions!

@marcochavezf marcochavezf requested a review from tgolen January 30, 2023 02:52
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Getting so fantastic!! Just a few more suggestions from me

contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
contributingGuides/KSv2.md Outdated Show resolved Hide resolved
tgolen
tgolen previously approved these changes Jan 30, 2023
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I think this is in a really good spot right now. I would say, let's take the latest of Alex's suggestions and then merge this. It doesn't need to be perfect and we can always update it more later, but it's holding up rolling out the extension publicly.

Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
marcochavezf and others added 3 commits January 30, 2023 10:34
Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
Co-authored-by: Alex Beaman <alexbeaman@expensify.com>
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this @marcochavezf 👍

@tgolen tgolen merged commit 606e367 into main Jan 30, 2023
@tgolen tgolen deleted the marco-addKSv2file branch January 30, 2023 16:32
@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.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 712.677 ms → 729.115 ms (+16.438 ms, +2.3%)
App start runJsBundle 202.906 ms → 204.375 ms (+1.469 ms, +0.7%)
App start nativeLaunch 20.031 ms → 20.097 ms (+0.066 ms, ±0.0%)
App start regularAppStart 0.017 ms → 0.015 ms (-0.002 ms, -11.0%)
Open Search Page TTI 630.137 ms → 624.067 ms (-6.070 ms, -1.0%)
Show details
Name Duration
App start TTI Baseline
Mean: 712.677 ms
Stdev: 35.604 ms (5.0%)
Runs: 622.267061999999 652.7592080000322 657.1670880001038 674.2811330000404 678.3991499999538 680.702251000097 689.5300429998897 693.3142909999005 693.5393739999272 693.8700510000344 695.536603000015 698.3497899998911 699.731351000024 701.8227480000351 705.5882329999004 708.1499109999277 709.2504020000342 717.0667210000101 719.958399000112 721.7068099998869 724.4184689999092 731.1642650000285 734.6616380000487 740.1547109999228 741.6226180000231 745.1982529999223 745.706441000104 753.4002720001154 757.6287360000424 764.8843970000744 772.8441469999962 780.9919459999073

Current
Mean: 729.115 ms
Stdev: 30.063 ms (4.1%)
Runs: 676.0887420000508 678.2052609999664 679.6099620000459 681.7276069999207 697.1177880000323 699.3238200000487 706.8269330000039 709.6025409998838 711.815239999909 711.9892269999254 713.0797780000139 714.0120069999248 718.8014680000488 726.7577649999876 727.1485460000113 727.4060619999655 727.8666789999697 730.113156999927 730.9155089999549 733.7872160000261 737.7510460000485 747.637185000116 748.880126999924 749.8313460000791 751.9665169999935 754.610098999925 760.5018090000376 761.1309579999652 770.3436519999523 770.4554149999749 788.0311519999523 788.3531309999526
App start runJsBundle Baseline
Mean: 202.906 ms
Stdev: 25.313 ms (12.5%)
Runs: 166 168 170 179 181 181 183 184 184 187 187 190 191 193 195 195 196 205 206 208 208 209 210 214 218 218 220 221 246 251 263 266

Current
Mean: 204.375 ms
Stdev: 18.985 ms (9.3%)
Runs: 164 175 176 177 177 177 188 188 190 191 194 200 203 204 205 209 209 211 213 213 214 215 215 218 219 221 224 225 230 230 232 233
App start nativeLaunch Baseline
Mean: 20.031 ms
Stdev: 1.741 ms (8.7%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 21 21 21 21 21 22 22 22 22 22 23 23 24

Current
Mean: 20.097 ms
Stdev: 1.614 ms (8.0%)
Runs: 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 22 22 22 22 23 25
App start regularAppStart Baseline
Mean: 0.017 ms
Stdev: 0.001 ms (5.3%)
Runs: 0.015379999997094274 0.015381000004708767 0.015421000076457858 0.015910000074654818 0.016195000149309635 0.016315999906510115 0.01631600013934076 0.016316999914124608 0.0163569999858737 0.016478999983519316 0.016600999981164932 0.016641999827697873 0.016723999986425042 0.016764000058174133 0.016804999904707074 0.01684599998407066 0.01684599998407066 0.0168869998306036 0.01692699990235269 0.017048999899998307 0.017089999979361892 0.017170999897643924 0.017212000209838152 0.017496000044047832 0.017659000121057034 0.017699999967589974 0.01774100004695356 0.017985000042244792 0.018025999888777733 0.018635999877005816 0.01948999986052513

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.5%)
Runs: 0.013183000031858683 0.013712999876588583 0.013752999948337674 0.014160000020638108 0.014362999936565757 0.014362999936565757 0.01448600017465651 0.014526000013574958 0.014607999939471483 0.014648000011220574 0.0147299999371171 0.0147299999371171 0.014974000165238976 0.015055000083521008 0.01509599993005395 0.01509599993005395 0.01509599993005395 0.015137000009417534 0.015137000009417534 0.015298999845981598 0.015340000158175826 0.015381000004708767 0.015544000081717968 0.015625 0.016032000072300434 0.01611399999819696 0.01611399999819696 0.01631700014695525 0.016478999983519316 0.01676399982534349
Open Search Page TTI Baseline
Mean: 630.137 ms
Stdev: 27.373 ms (4.3%)
Runs: 577.0603430001065 590.8773610000499 593.7207440000493 601.239135999931 604.4144289998803 606.1883549999911 613.2681889999658 613.5766610000283 614.5138350001071 617.1883539999835 618.8409420000389 618.9514979999512 619.1285410001874 619.2871910000686 620.4212650000118 620.5594069999643 620.7461340001319 621.4985760001 622.322834999999 627.1304929999169 631.1331789998803 645.272501999978 646.3457849998958 647.756266000215 650.9412439998705 656.295614000177 656.3570150001906 663.303588999901 674.4372160001658 681.4883629998658 684.5174559999723 685.6094569999259

Current
Mean: 624.067 ms
Stdev: 19.269 ms (3.1%)
Runs: 574.291300999932 594.192790000001 595.682983999839 596.5679130000062 605.0643720000517 605.339152000146 607.0803629998118 611.8152270000428 611.8363850000314 614.7435720001813 616.85742200003 618.9071459998377 620.3912769998424 624.7657069999259 624.9846599998418 627.5274249999784 627.6880289998371 628.8568930001929 629.2393809999339 632.3174230000004 632.7259929999709 632.7901210000273 634.0308840000071 642.5214849999174 642.6898189999629 642.8000080001075 644.1237800000235 644.4205330000259 644.925578000024 653.4178880001418 663.4795329999179

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/tgolen in version: 1.2.62-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.62-1 🚀

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.

5 participants