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] Check if report.participants exists #14033

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

luacmartins
Copy link
Contributor

@luacmartins luacmartins commented Jan 5, 2023

Details

I'm not really sure how to reproduce the issue, but we do have Crashlytics crash reports for it so I figured we should add a check to prevent the error.

Fixed Issues

$ #14019

Tests

  1. Open a room chat
  2. Tap the chat header
  3. Verify that it opens and you can see the room avatar icons.
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Regular QA steps should cover this

  • 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
    • 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 ![web](https://user-images.githubusercontent.com/22219519/210870553-7810a309-18d1-480c-8bff-b10ffe0841c2.png)
Mobile Web - Chrome

chrome

Mobile Web - Safari

safari

Desktop desktop
iOS

ios

Android

android

@luacmartins luacmartins self-assigned this Jan 5, 2023
@luacmartins luacmartins marked this pull request as ready for review January 5, 2023 20:12
@luacmartins luacmartins requested a review from a team as a code owner January 5, 2023 20:12
@melvin-bot melvin-bot bot requested review from Luke9389 and sobitneupane and removed request for a team January 5, 2023 20:12
@melvin-bot
Copy link

melvin-bot bot commented Jan 5, 2023

@sobitneupane @Luke9389 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]

Copy link
Contributor

@sobitneupane sobitneupane left a comment

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 Screenshot 2023-01-06 at 12 14 02
Mobile Web - Chrome
Mobile Web - Safari
Desktop Screenshot 2023-01-06 at 12 15 52
iOS
Android

@luacmartins
Copy link
Contributor Author

Friendly bump for review!

@Luke9389 Luke9389 merged commit 5a09253 into main Jan 11, 2023
@Luke9389 Luke9389 deleted the cmartins-fixParticipantsLength branch January 11, 2023 00:45
@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 nativeLaunch 9.467 ms → 19.700 ms (+10.233 ms, +108.1%) 🟡
Open Search Page TTI 600.510 ms → 608.481 ms (+7.970 ms, +1.3%)
App start TTI 679.039 ms → 686.192 ms (+7.153 ms, +1.1%)
App start runJsBundle 185.710 ms → 190.906 ms (+5.197 ms, +2.8%)
App start regularAppStart 0.013 ms → 0.021 ms (+0.007 ms, +53.3%) 🟡
Show details
Name Duration
App start nativeLaunch Baseline
Mean: 9.467 ms
Stdev: 0.957 ms (10.1%)
Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 11 11 11 11 11

Current
Mean: 19.700 ms
Stdev: 1.656 ms (8.4%)
Runs: 17 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 23 23 24
Open Search Page TTI Baseline
Mean: 600.510 ms
Stdev: 23.566 ms (3.9%)
Runs: 562.7281499998644 567.3275149995461 569.9738779999316 571.1827809996903 573.1569420006126 579.6377369994298 580.385050999932 582.1988939996809 583.568889000453 583.9108069995418 586.0327959991992 591.3215740006417 592.1671150000766 592.6493729995564 594.36352599971 598.0042719999328 600.6016040006652 600.9649660000578 602.5233970005065 603.1483970005065 603.9204099997878 604.7740890001878 606.9332690006122 608.3243820006028 609.7885339995846 613.8845629999414 618.2359219994396 621.6108400002122 623.2284760000184 636.6420090002939 638.2642419999465 650.8209229996428 664.5689289998263

Current
Mean: 608.481 ms
Stdev: 26.109 ms (4.3%)
Runs: 563.4751389995217 563.7591549996287 573.4897459996864 573.9361990001053 577.0958670005202 579.041748999618 581.7573249991983 582.7278240006417 585.6370440004393 586.844686999917 595.1931560002267 598.5149739999324 600.019491000101 603.172201000154 603.2671309998259 606.7066249996424 607.4256589999422 607.5874839993194 609.8294680006802 614.3958740001544 621.6292730001733 622.6678060004488 624.2583009991795 625.700521000661 628.0771080004051 632.4782719993964 633.9263920001686 634.5795490005985 641.7831220002845 643.7865809993818 647.0334479995072 653.6407470004633 656.4304609997198
App start TTI Baseline
Mean: 679.039 ms
Stdev: 37.964 ms (5.6%)
Runs: 612.0591320004314 622.891335000284 631.2304090000689 635.8466509999707 639.3281009998173 642.352361000143 643.4891090001911 645.3563219998032 653.3165920004249 660.1151189999655 660.4796930002049 661.2392920004204 663.6180010000244 667.3153489995748 674.4737700000405 677.8087569996715 678.3898719996214 682.1694830004126 686.0728390002623 686.6872579995543 690.4300840003416 696.4712779996917 696.9061639998108 699.344242000021 713.2606469998136 715.7050249995664 716.7987780002877 734.2728289999068 742.2185129998252 757.0364279998466 763.5356780001894

Current
Mean: 686.192 ms
Stdev: 30.108 ms (4.4%)
Runs: 631.8597550000995 637.9739870000631 649.677501000464 653.3272620001808 654.341699000448 654.975642000325 662.4872829997912 662.826856999658 664.144983000122 667.2346970001236 667.4575089998543 669.0941580003127 671.6984729999676 675.1847320003435 676.2067999998108 683.5770650003105 685.4435019996017 686.4592359997332 687.3313049999997 690.5156600000337 696.2247400004417 700.9723889995366 704.2257510004565 708.675138999708 709.9270909996703 714.3654009997845 714.874049000442 715.2406829996035 729.2148660002276 738.0380889996886 740.3541729999706 754.2206330001354
App start runJsBundle Baseline
Mean: 185.710 ms
Stdev: 22.502 ms (12.1%)
Runs: 156 156 157 158 159 159 160 172 172 176 176 177 179 180 180 181 182 185 188 189 190 192 195 200 204 210 211 217 224 232 240

Current
Mean: 190.906 ms
Stdev: 20.038 ms (10.5%)
Runs: 157 162 165 171 173 174 174 175 176 177 178 178 178 181 183 186 188 188 194 195 196 201 203 207 207 209 210 211 221 225 231 235
App start regularAppStart Baseline
Mean: 0.013 ms
Stdev: 0.001 ms (4.5%)
Runs: 0.012004000134766102 0.012655000202357769 0.012736000120639801 0.012736000120639801 0.012736000120639801 0.012776999734342098 0.012899000197649002 0.012939000502228737 0.013223999179899693 0.013225000351667404 0.013306000269949436 0.013387000188231468 0.013427999801933765 0.013427999801933765 0.013427999801933765 0.013427999801933765 0.013591000810265541 0.013631999492645264 0.013671999797224998 0.013672000728547573 0.013793999329209328 0.013794000260531902 0.013996999710798264 0.013997000642120838 0.014159999787807465 0.014403999783098698 0.014444999396800995 0.014689000323414803

Current
Mean: 0.021 ms
Stdev: 0.002 ms (8.5%)
Runs: 0.017211999744176865 0.017903000116348267 0.018269999884068966 0.018432000651955605 0.018757999874651432 0.018799000419676304 0.018880000337958336 0.018881000578403473 0.019491000100970268 0.0197759997099638 0.019938000477850437 0.019979000091552734 0.02001999970525503 0.02034500055015087 0.02038600016385317 0.020507999695837498 0.020507999695837498 0.020874000154435635 0.021117999218404293 0.021158000454306602 0.021402999758720398 0.021484999917447567 0.021647000685334206 0.0217289999127388 0.021932000294327736 0.022216999903321266 0.022542000748217106 0.022665000520646572 0.022948999889194965 0.0235190000385046 0.024984000250697136

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.2.53-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.2.53-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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants