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

[PROD QA] Navigate to Concierge chat from Statement #14050

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

thienlnam
Copy link
Contributor

@thienlnam thienlnam commented Jan 6, 2023

Details

Fixed Issues

$ #11097

Tests

Difficult to test on platforms besides web unless you're on production since there is no way to access the statement on NewDot without modifying the URL

  1. Access the statement using the URL http://localhost:8080/statements/202201/
  2. You should see something like this

Screen Shot 2022-10-03 at 3 26 01 PM

3. Click on the concierge email and ensure that it redirects you to the concierge chat

Screen Shot 2022-10-03 at 3 26 32 PM

To test on other devices, this is what I do:

{
icon: Expensicons.ChatBubble,
text: this.props.translate('sidebarScreen.newChat'),
onSelected: () => Navigation.navigate(ROUTES.NEW_CHAT),
},

Update to
onSelected: () => Navigation.navigate(ROUTES.getWalletStatementWithDateRoute('202211')),

This will override the new chat button to open up the statement for November, 2022

Offline tests

QA Steps

No QA

INTERNAL QA - PROD QA ONLY

  1. On a mobile device, go to your Concierge chat (on your expensify account).
  2. You should see a message link this - click on the link

Screen Shot 2022-10-03 at 3 35 04 PM

3. On the statement page, click on the Concierge email address 4. It should open up the chat with Concierge in NewDot

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
Screen.Recording.2023-01-06.at.3.25.14.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-01-06.at.3.33.42.PM.mov
Mobile Web - Safari

Running into SSL errors (will test on prod)

Desktop
Screen.Recording.2023-01-06.at.3.34.52.PM.mov
iOS Running into SSL errors (will test on prod)
Android x

@thienlnam thienlnam self-assigned this Jan 6, 2023
@thienlnam thienlnam changed the title Navigate to Concierge chat from Statement [PROD QA] Navigate to Concierge chat from Statement Jan 6, 2023
@thienlnam thienlnam marked this pull request as ready for review January 6, 2023 23:35
@thienlnam thienlnam requested a review from a team as a code owner January 6, 2023 23:35
@melvin-bot melvin-bot bot requested review from mananjadhav and stitesExpensify and removed request for a team January 6, 2023 23:36
@melvin-bot
Copy link

melvin-bot bot commented Jan 6, 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]

@thienlnam thienlnam removed the request for review from mananjadhav January 6, 2023 23:41
@thienlnam
Copy link
Contributor Author

Removing since this requires an internal engineer to test the statement

@stitesExpensify stitesExpensify added the InternalQA This pull request required internal QA label Jan 9, 2023
@stitesExpensify
Copy link
Contributor

stitesExpensify commented Jan 9, 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
2023-01-09_15-48-20.mp4
Mobile Web - Chrome

n/a

Mobile Web - Safari

n/a

Desktop

n/a

iOS

n/a

Android

n/a

@stitesExpensify
Copy link
Contributor

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

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

LGTM

@stitesExpensify stitesExpensify merged commit eb3d9af into main Jan 9, 2023
@stitesExpensify stitesExpensify deleted the jack-conciergeStatementModal branch January 9, 2023 22:58
@OSBotify
Copy link
Contributor

OSBotify commented Jan 9, 2023

✋ 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

github-actions bot commented Jan 9, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start nativeLaunch 10.344 ms → 19.600 ms (+9.256 ms, +89.5%) 🔴
App start regularAppStart 0.014 ms → 0.020 ms (+0.006 ms, +42.6%) 🔴
App start runJsBundle 185.300 ms → 183.233 ms (-2.067 ms, -1.1%)
App start TTI 673.446 ms → 665.577 ms (-7.869 ms, -1.2%)
Open Search Page TTI 617.127 ms → 602.077 ms (-15.049 ms, -2.4%)
Show details
Name Duration
App start nativeLaunch Baseline
Mean: 10.344 ms
Stdev: 1.946 ms (18.8%)
Runs: 8 8 8 8 8 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 10 10 10 11 11 12 13 13 13 14 15 15

Current
Mean: 19.600 ms
Stdev: 1.381 ms (7.0%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 21 21 21 22 23 23
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.002 ms (11.7%)
Runs: 0.01220700005069375 0.012614000122994184 0.012614000122994184 0.012655000202357769 0.012696000281721354 0.012776999734342098 0.0129399998113513 0.012980000115931034 0.012980999890714884 0.013264999724924564 0.013387000188231468 0.013427999801933765 0.013630999717861414 0.013631000183522701 0.013794000260531902 0.013875999953597784 0.014200999867171049 0.014241000171750784 0.014485999941825867 0.014892000239342451 0.015137000009417534 0.015176999848335981 0.015421000309288502 0.015544000081717968 0.015746999997645617 0.016193999908864498 0.0167239997535944 0.0167239997535944 0.018391999881714582 0.018635999877005816

Current
Mean: 0.020 ms
Stdev: 0.002 ms (8.8%)
Runs: 0.0176189998164773 0.017822000198066235 0.017903999891132116 0.0183100001886487 0.018554999958723783 0.018635999877005816 0.01875800034031272 0.018962000031024218 0.019286999944597483 0.0194089999422431 0.019776000175625086 0.020182000007480383 0.020263999700546265 0.020304000005126 0.020345999859273434 0.02038600016385317 0.020466999616473913 0.020711000077426434 0.02103699976578355 0.02119999984279275 0.021240000147372484 0.021402999758720398 0.021606999915093184 0.022013000212609768 0.02237899973988533 0.022419999819248915 0.022501999977976084 0.02254300005733967 0.022745999973267317 0.025797000154852867
App start runJsBundle Baseline
Mean: 185.300 ms
Stdev: 15.634 ms (8.4%)
Runs: 157 163 164 164 168 171 174 176 179 179 180 180 181 181 182 183 183 185 186 187 189 195 197 197 198 206 208 212 216 218

Current
Mean: 183.233 ms
Stdev: 19.996 ms (10.9%)
Runs: 154 155 162 162 163 164 166 171 171 173 173 174 175 175 176 177 179 185 186 190 192 194 195 198 202 204 206 216 218 241
App start TTI Baseline
Mean: 673.446 ms
Stdev: 29.188 ms (4.3%)
Runs: 619.472442000173 633.9766549998894 637.0273159998469 638.9643140002154 644.9907970000058 645.4026870001107 649.3916270001791 653.5968260001391 655.1257989997976 659.9142470001243 661.5339730000123 662.2831890000962 662.9551429999992 665.0987630002201 666.1018289998174 666.384856000077 666.6430870001204 674.9622439998202 675.4787170002237 681.6610719999298 682.1093089999631 683.693605999928 689.9049390000291 691.8618520000018 693.3721170001663 695.0128460000269 698.6742899999954 715.1028109998442 727.998281000182 733.8260690001771 744.3004979998805

Current
Mean: 665.577 ms
Stdev: 33.739 ms (5.1%)
Runs: 626.4703230001032 628.2709530000575 628.2770150001161 629.6177079998888 629.654310000129 633.7494350001216 634.9985090000555 637.0993090001866 638.3137849997729 638.8168600001372 640.0262719998136 645.0281389998272 647.5734649999067 652.0510430000722 655.0854830001481 655.9477209998295 659.4861539998092 660.4226069999859 670.9998889998533 671.0972750000656 678.372357999906 678.5538980001584 684.6459030001424 689.2986980001442 689.3747160001658 690.7007039999589 718.9686500001699 721.3448169999756 722.6304069999605 730.8403480001725 745.1651920001023
Open Search Page TTI Baseline
Mean: 617.127 ms
Stdev: 19.655 ms (3.2%)
Runs: 579.7555750003085 587.244262999855 591.0373539999127 592.0908619998954 594.3547370000742 594.7575679998845 598.5521650002338 601.3063559997827 602.0621340000071 602.3808190003037 604.4637460000813 607.0845550000668 610.9920250000432 611.3199060000479 616.6660969997756 617.0522869997658 619.4551189998165 620.0542810000479 620.7817379999906 623.371460000053 624.8377279997803 625.3642580001615 625.5885830000043 630.6384680001065 635.7202149997465 636.6752940001898 639.5072430004366 639.99947099993 640.3520509996451 644.0952559998259 654.2851569997147 656.2012940002605

Current
Mean: 602.077 ms
Stdev: 11.460 ms (1.9%)
Runs: 577.6317960000597 582.0198969999328 584.5266929999925 586.0213629999198 586.9673270001076 590.0797930001281 590.3673100001179 591.6289470000193 596.1171470000409 599.1033529997803 599.884358999785 600.8822429999709 601.9295250000432 603.2269290001132 603.8225920000114 604.2768959999084 605.2979330001399 605.4401039998047 606.4522299999371 607.0040279999375 608.3176669999957 608.5363770001568 608.6551520000212 609.569987999741 614.1944990004413 615.4410810000263 615.8930669999681 618.0418710000813 619.6016040001996 621.3825690001249

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

github-actions bot commented Jan 9, 2023

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

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀

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 InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants