Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[No QA] Remove dead ExpensifyCash files #14786

Merged
merged 3 commits into from
Mar 1, 2023
Merged

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Feb 3, 2023

Details

Our app hasn't been called ExpensifyCash for a long time now. These files are long-unused.

Fixed Issues

$ none

Tests

Run an iOS build locally

Offline tests

none

QA Steps

None

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

Successful iOS build:

image

@roryabraham roryabraham self-assigned this Feb 3, 2023
@roryabraham roryabraham requested a review from a team as a code owner February 3, 2023 07:49
@melvin-bot melvin-bot bot requested review from youssef-lr and removed request for a team February 3, 2023 07:50
@melvin-bot
Copy link

melvin-bot bot commented Feb 3, 2023

@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

👍

@roryabraham
Copy link
Contributor Author

@youssef-lr bump

@Julesssss
Copy link
Contributor

Julesssss commented Feb 7, 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

Screenshot 2023-02-07 at 10 09 58

Mobile Web - Chrome

Screenshot_20230207_101715

Mobile Web - Safari

Simulator Screen Shot - iPhone 14 Pro - 2023-02-07 at 10 10 58

Desktop

Screenshot 2023-02-07 at 10 12 59

iOS
Android

Screenshot_1675765983

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

I tried to push this one forward with a full review, but the iOS compile is failing. I think we need to remove this final reference in project.pbxproj.

error: Build input file cannot be found: '/Users/jules/Projects/Expensidev/App/ios/ExpensifyCash-Bridging-Header.h'. Did you forget to declare this file as an output of a script phase or custom build rule which produces it? (in target 'NewExpensify' from project 'NewExpensify')

Screenshot 2023-02-07 at 10 37 43

All other platforms test well

@Julesssss
Copy link
Contributor

I wondered if this simply required a project clean, but sadly not. We'll need to make the change I suggested above.

@Julesssss
Copy link
Contributor

Bumping this @roryabraham

@roryabraham
Copy link
Contributor Author

@Julesssss updated and verified that the app builds successfully

@Julesssss
Copy link
Contributor

Added 'Run an iOS build locally' as the test, to confirm there are no more missing files errors.

@Julesssss
Copy link
Contributor

Built successfully, release build will be tested once merged and deployed:

Simulator Screen Shot - iPhone 14 Pro - 2023-03-01 at 10 28 28

@Julesssss Julesssss merged commit 472affc into main Mar 1, 2023
@Julesssss Julesssss deleted the Rory-RemoveExpensifyCash branch March 1, 2023 10:35
@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 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 Mar 1, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 604.709 ms → 677.907 ms (+73.198 ms, +12.1%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 604.709 ms
Stdev: 14.615 ms (2.4%)
Runs: 571.8864339999855 577.7359220013022 585.3047689981759 587.5622969977558 588.7142339982092 590.3572599999607 593.7637539990246 594.618285998702 595.6578369997442 596.4251710027456 597.0979419983923 598.3486739993095 600.1107989996672 602.3550619967282 603.1022539995611 603.412964001298 605.5941980034113 606.8798829987645 607.2550459988415 608.2271730005741 608.9164629988372 609.2725830003619 610.4586590006948 617.2578129991889 618.212727997452 619.4766849987209 619.5978600010276 620.2008059993386 622.3396000005305 627.156047001481 629.5669759996235 633.8295900002122

Current
Mean: 677.907 ms
Stdev: 19.918 ms (2.9%)
Runs: 626.8767089992762 627.2959400005639 647.5999349988997 661.3924969993532 662.9218759983778 664.6767590008676 665.2059740014374 665.2937010005116 667.0683599971235 669.2997649982572 669.5521650016308 671.1735850013793 671.3250329978764 674.6693120002747 676.350179001689 679.4484049975872 681.6730960011482 682.1211749985814 682.6454669982195 684.8463950008154 685.4305419996381 685.4906819984317 691.1998700015247 691.3325200006366 693.0580650009215 694.5829670019448 696.9960939995944 698.4187010005116 699.1274419985712 700.8689780011773 710.2875169999897 714.8009850010276

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 723.611 ms → 742.223 ms (+18.612 ms, +2.6%)
App start runJsBundle 199.188 ms → 204.033 ms (+4.846 ms, +2.4%)
App start nativeLaunch 19.300 ms → 20.281 ms (+0.981 ms, +5.1%)
App start regularAppStart 0.015 ms → 0.015 ms (+0.000 ms, +1.8%)
Show details
Name Duration
App start TTI Baseline
Mean: 723.611 ms
Stdev: 30.369 ms (4.2%)
Runs: 660.8913070000708 677.7557439990342 681.3744659982622 682.5220069997013 685.2807479985058 685.3971560001373 692.4832290001214 698.1517719998956 704.562081001699 713.125978000462 714.7680680006742 714.8893279992044 716.035950999707 717.4979909993708 721.436071999371 723.8711090013385 725.3435380011797 728.5858860015869 730.093557998538 730.680555999279 732.7668629996479 736.6414979994297 738.7594030015171 743.0189719982445 748.7871089987457 751.8772739991546 752.5984379984438 754.4264700002968 755.342773001641 763.9479079991579 782.4102209992707 790.2294090017676

Current
Mean: 742.223 ms
Stdev: 32.147 ms (4.3%)
Runs: 696.2599840015173 702.7283120006323 704.237906999886 707.4002810008824 707.7132660001516 707.7370829991996 707.7798230014741 708.2782780006528 709.935993000865 710.1276380009949 711.6305390000343 714.4230820015073 723.098684001714 734.0170629993081 737.1772460006177 739.273439001292 743.9368100017309 747.6654240004718 748.9849210008979 758.3136770017445 760.3809700012207 761.1071979999542 763.3790590018034 764.3194429986179 768.8086009994149 769.682009998709 772.5034010000527 791.6238959990442 794.1595529988408 794.2382460013032 794.5494830012321 795.6570320017636
App start runJsBundle Baseline
Mean: 199.188 ms
Stdev: 18.168 ms (9.1%)
Runs: 169 172 172 174 177 180 183 183 183 185 186 191 192 196 197 198 199 203 205 206 207 210 212 214 214 216 218 220 221 225 231 235

Current
Mean: 204.033 ms
Stdev: 18.641 ms (9.1%)
Runs: 175 181 181 183 184 186 187 189 189 191 191 192 195 196 199 204 206 209 211 216 216 218 219 219 220 221 226 232 233 252
App start nativeLaunch Baseline
Mean: 19.300 ms
Stdev: 1.696 ms (8.8%)
Runs: 17 17 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 21 21 21 22 22 22 24

Current
Mean: 20.281 ms
Stdev: 1.858 ms (9.2%)
Runs: 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 22 22 22 22 23 24 24 25
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (5.3%)
Runs: 0.013265002518892288 0.013509001582860947 0.013550002127885818 0.013630997389554977 0.01367199793457985 0.013794001191854477 0.013875000178813934 0.013916000723838806 0.013996999710798264 0.014038000255823135 0.014038000255823135 0.014159999787807465 0.014200996607542038 0.014281999319791794 0.014322999864816666 0.014364000409841537 0.01440500095486641 0.014485999941825867 0.014688998460769653 0.014689002186059952 0.014730002731084824 0.01477000117301941 0.014770999550819397 0.015014000236988068 0.015015002340078354 0.015135999768972397 0.015137001872062683 0.015461999922990799 0.015502996742725372 0.015625 0.016032002866268158 0.016642000526189804

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.4%)
Runs: 0.013183999806642532 0.013753000646829605 0.013794001191854477 0.013916000723838806 0.014118999242782593 0.014159999787807465 0.014201000332832336 0.014322999864816666 0.014364000409841537 0.014364000409841537 0.01448499783873558 0.014485999941825867 0.014566998928785324 0.014689002186059952 0.014689996838569641 0.014770999550819397 0.014851998537778854 0.015054997056722641 0.015096001327037811 0.01521800085902214 0.01521800085902214 0.01534000039100647 0.015461999922990799 0.015462998300790787 0.015463002026081085 0.015625 0.016235999763011932 0.017089001834392548

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

github-actions bot commented Mar 1, 2023

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

@Julesssss Julesssss removed the DeployBlockerCash This issue or pull request should block deployment label Mar 1, 2023
@Julesssss
Copy link
Contributor

Ignoring this and removing the label, there's no way that removing iOS files degraded performance.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

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

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants