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

fix:14670 update expensify-common version #14940

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

tienifr
Copy link
Contributor

@tienifr tienifr commented Feb 8, 2023

Details

Fixed Issues

$ #14670
PROPOSAL: #14670 (comment)

Tests

  1. Open the app
  2. Turn off the internet (1)
  3. Open any chat
  4. Send the message defined for each case and verify LHN message is shown as expected for the respective case.

Case 1 - h1 or #
Message

# Heading
Normal text on the next line

LHN
Heading

Case 2 - quote text or >
Message

> quote text
Normal text on the next line

LHN
quote text

Case 3 - preformatted text or ```
Message

``` preformatted ```
Normal text on the next line

LHN
preformatted

(1) Because the solution has just been implemented in front-end side, we need to turn off the internet to prevent backend from updating wrong value. After updating BE side, we can skip step 2

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Open the app
  2. Turn off the internet (1)
  3. Open any chat
  4. Send the message defined for each case and verify LHN message is shown as expected for the respective case.

Case 1 - h1 or #
Message

# Heading
Normal text on the next line

LHN
Heading

Case 2 - quote text or >
Message

> quote text
Normal text on the next line

LHN
quote text

Case 3 - preformatted text or ```
Message

``` preformatted ```
Normal text on the next line

LHN
preformatted

(1) Because the solution has just been implemented in front-end side, we need to turn off the internet to prevent backend from updating wrong value. After updating BE side, we can skip step 2

  • 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
14670-web-1.mp4
Mobile Web - Chrome
14670-chrome-1.mp4
Mobile Web - Safari
14670-safari-1.mp4
Desktop
14670-desktop-1.mp4
iOS
14670-ios-1.mp4
Android
14670-android-1.mp4

@tienifr tienifr requested a review from a team as a code owner February 8, 2023 03:26
@melvin-bot melvin-bot bot requested review from techievivek and removed request for a team February 8, 2023 03:27
@MelvinBot
Copy link

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

@tienifr tienifr changed the title fix: update version fix:14670 update version Feb 8, 2023
@tienifr tienifr changed the title fix:14670 update version fix:14670 update expensify-common version Feb 8, 2023
@tienifr tienifr marked this pull request as draft February 8, 2023 03:29
@tienifr tienifr marked this pull request as ready for review February 8, 2023 04:08
@Santhosh-Sellavel
Copy link
Collaborator

@techievivek please assign me & @puneetlath as reviewers here, thanks!

@Santhosh-Sellavel
Copy link
Collaborator

Thanks, @Julesssss for taking care of Expensify/expensify-common#498 and for your review here!

@Santhosh-Sellavel
Copy link
Collaborator

@tienifr
Friendly note:

Please try to link issues properly the first time, double check before submitting the PR. Because only linking issues correctly would request a review from C+ & CME assigned on the issue.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Feb 8, 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 & Desktop
Screen.Recording.2023-02-10.at.6.31.11.AM.mov
Mobile Web - Safari
Screen.Recording.2023-02-10.at.6.44.06.AM.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.-.2023-02-10.at.06.41.09.mp4
Android & Mobile Web - Chrome
Screen.Recording.2023-02-10.at.6.49.43.AM.mov

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Feb 8, 2023

@tienifr Can you include test steps for blockquote|pre also?

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

Waiting for this #14940 (comment)

@tienifr
Copy link
Contributor Author

tienifr commented Feb 9, 2023

Hi @Santhosh-Sellavel , I have included test steps for blockquote|pre and also updated test videos. Please help me check it. Thank you!

Copy link
Contributor

@techievivek techievivek left a comment

Choose a reason for hiding this comment

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

Waiting for @Santhosh-Sellavel to complete the checklist.

@Santhosh-Sellavel
Copy link
Collaborator

@tienifr
let's improve the test steps other wise looks good.

The first 3 steps are good. After that define each case that should be tested, so it would more clear for QA.
4. Send the message defined for each case and verify LHN message is shown as expected for the respective case.

Case 1 - h1 or #

Message

# Heading
Normal text on the next line

LHN
Heading

Define Cases 2 & 3

thanks!

@tienifr
Copy link
Contributor Author

tienifr commented Feb 10, 2023

@Santhosh-Sellavel I just updated the test steps, please help to check again

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

LGTM!

@tienifr
Copy link
Contributor Author

tienifr commented Feb 10, 2023

@techievivek This PR is given 2 approvals, please take a look at it and merge it if there is no problem

@techievivek techievivek merged commit 88cf963 into Expensify:main Feb 10, 2023
@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 685.458 ms → 694.995 ms (+9.537 ms, +1.4%)
App start nativeLaunch 20.200 ms → 20.621 ms (+0.421 ms, +2.1%)
App start regularAppStart 0.016 ms → 0.015 ms (-0.001 ms, -9.0%)
Open Search Page TTI 601.167 ms → 597.854 ms (-3.313 ms, -0.6%)
App start runJsBundle 190.031 ms → 185.774 ms (-4.257 ms, -2.2%)
Show details
Name Duration
App start TTI Baseline
Mean: 685.458 ms
Stdev: 27.689 ms (4.0%)
Runs: 633.4599480000325 645.3842210001312 650.8368420000188 654.8109240001068 655.0849089999683 655.1982359997928 659.8120539998636 660.5245670001023 662.0594219998457 662.9923700001091 667.0273759998381 670.6898440001532 672.2792219999246 678.3974170000292 680.9328379998915 681.6230520000681 690.6473829997703 691.0770519999787 691.5736309997737 696.3642710000277 697.5184490000829 698.6656039999798 705.11221700022 706.7397460001521 706.81288699992 706.897812999785 709.0227359998971 715.054270000197 720.1214580000378 724.3910810002126 729.1474480000325 754.3973719999194

Current
Mean: 694.995 ms
Stdev: 24.904 ms (3.6%)
Runs: 655.6625680001453 658.0443679997697 660.0552260000259 663.0058079999872 671.9303279998712 674.6479779998772 677.4765070001595 681.317218999844 682.4521840000525 682.6360869999044 682.9588799998164 683.8331539998762 685.1237739999779 685.7347209998406 687.1762669999152 688.9242830001749 693.5950969997793 694.9846970001236 699.6785300001502 705.1625500000082 705.2207320001908 707.8820420000702 707.9625160000287 712.531589999795 712.797348999884 717.4884159998037 734.3290070001967 735.4321699999273 747.8212950001471 753.9812779999338
App start nativeLaunch Baseline
Mean: 20.200 ms
Stdev: 2.023 ms (10.0%)
Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 22 22 22 22 22 24 24 26

Current
Mean: 20.621 ms
Stdev: 2.235 ms (10.8%)
Runs: 17 17 18 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 21 21 22 22 22 23 25 25 27
App start regularAppStart Baseline
Mean: 0.016 ms
Stdev: 0.001 ms (7.9%)
Runs: 0.013875000178813934 0.014241999946534634 0.014322999864816666 0.015014000236988068 0.015339999925345182 0.015339999925345182 0.01534100016579032 0.015420999843627214 0.015461999922990799 0.015625 0.015625 0.015625 0.015664999838918447 0.015869999770075083 0.01590999960899353 0.015910000074654818 0.016032000072300434 0.016112999990582466 0.01615400006994605 0.016398000065237284 0.01688600005581975 0.01688600005581975 0.017171000130474567 0.017212000209838152 0.01725299982354045 0.01741599990054965 0.017578000202775 0.017821999732404947 0.018920999951660633 0.019816000014543533

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.1%)
Runs: 0.013509000185877085 0.01358999963849783 0.01375299971550703 0.0138349998742342 0.013956000097095966 0.013998000416904688 0.014037999790161848 0.014119000174105167 0.0142000000923872 0.014201000332832336 0.0143630001693964 0.01436399994418025 0.014403999783098698 0.014607999939471483 0.014688999857753515 0.01472900016233325 0.0147299999371171 0.014771000016480684 0.014851000159978867 0.014934000093489885 0.015176999848335981 0.01525900000706315 0.015461999922990799 0.015503000002354383 0.015625 0.015786999836564064 0.0157880000770092 0.015949999913573265 0.01599099999293685 0.016112999990582466
Open Search Page TTI Baseline
Mean: 601.167 ms
Stdev: 12.741 ms (2.1%)
Runs: 579.2800699998625 580.2163490001112 583.2540700002573 586.6257319999859 587.8680829997174 588.4563799998723 590.7295739999972 591.947061999701 592.6821290003136 592.6955569996499 592.8154710000381 594.3907469999976 598.3438719999976 599.1153970002197 601.7631029998884 601.7646889998578 603.2818200001493 603.3038330003619 604.9950359999202 605.3541260003112 606.0768639999442 606.1494549997151 607.2917490000837 608.7522380002774 609.4849860002287 609.9073489997536 611.6392419999465 617.2986249998212 622.702961999923 623.3153490000404 634.6638999995776

Current
Mean: 597.854 ms
Stdev: 14.964 ms (2.5%)
Runs: 579.3293460002169 580.1975099998526 582.0204269997776 582.0590420002118 582.8558760001324 585.2204180001281 585.7542729997076 586.581991000101 586.9940600004047 587.012369999662 588.6883549997583 589.1958830002695 590.2737219999544 590.3409019997343 590.5088709997945 594.1903889998794 595.0878090001643 595.2536220001057 595.6515709999949 596.4331869999878 600.0285240001976 600.5002039996907 600.8411050001159 607.0026040002704 609.9800629997626 612.5166020002216 612.9608559999615 616.6388759999536 617.8465990000404 618.8447259999812 629.3740240000188 641.1405440000817
App start runJsBundle Baseline
Mean: 190.031 ms
Stdev: 20.232 ms (10.6%)
Runs: 157 160 165 167 168 170 170 171 174 178 181 182 182 184 185 186 189 190 191 192 196 197 198 201 204 207 215 215 217 223 226 240

Current
Mean: 185.774 ms
Stdev: 14.548 ms (7.8%)
Runs: 162 163 165 167 170 172 173 174 175 177 179 180 181 182 183 183 183 187 188 190 190 196 196 198 200 201 202 204 209 214 215

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/techievivek in version: 1.2.69-2 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.69-2 🚀

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.

7 participants