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.

Sign up for GitHub

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

Jump to bottom

Add 'OpenIOUDetailsPage' API command #9878

Merged
merged 5 commits into from
Jul 18, 2022
Merged

Add 'OpenIOUDetailsPage' API command #9878

merged 5 commits into from
Jul 18, 2022

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Jul 13, 2022 •
edited
Loading

CC @Justicea83 & @vitHoracek

Details

Front end implementation of the OpenIOUDetailsPage API Refactor

Fixed Issues

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

Tests

1) Test an IOU Request with a new user

  1. Sign into NewDot as user 1
  2. Start a chat with a user 2 (a user you have not chatted with before)
  3. Create an IOU request (either from within the chat, or from the LHN menu)
  4. Tap the IOUPreview to open the IOU Details Modal
  5. A single IOU transaction should be shown in the Modal

2) Test an old, inactive IOU Request

  1. Following on from test 1, settle the request as the other user
  2. The IOU should be shown as settled, for both user 1 and 2
  3. For both users, sign out and back in
  4. Navigate back to the chatReport and tap the IOUPreview again
  5. The IOU history should be shown (instead of an empty list)

3) Ensure that additional IOU History is loaded

  1. Create an IOU Transaction with any user, or locate an existing one
  2. Ssend some random messages to that user (more than 30 or 35). You can quickly do it on the web, just type a few letters in compose box then hit enter.
  3. Now Logout of the app, so local data will be cleared.
  4. Now Login back again try to tap on the IOU balance
  5. You should see the IOU Transaction, instead of a blank page

4. Verify that no errors appear in the JS console

PR Review Checklist

Contributor (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 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 included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • 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 was added in all src/languages/* files
    • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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
  • 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 */
    • Any functional components have the displayName property
    • 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 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
  • 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 verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • 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 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 was added in all src/languages/* files
    • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
    • Any functional components have the displayName property
    • 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 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.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

QA Steps

  • Run the above tests
  • Verify that no errors appear in the JS console

Screenshots

Web

Screenshot 2022-07-14 at 17 16 12

Mobile Web

Simulator Screen Shot - iPhone 13 - 2022-07-15 at 16 00 14

Desktop

Screenshot 2022-07-15 at 15 40 10

iOS

Simulator Screen Shot - iPhone 13 - 2022-07-15 at 15 56 54

Android

Screenshot_1657897376

@Julesssss Julesssss self-assigned this Jul 13, 2022
@Julesssss Julesssss mentioned this pull request Jul 13, 2022
Closed
89 tasks
@Julesssss Julesssss mentioned this pull request Jul 14, 2022
Merged
@Julesssss Julesssss changed the title Add API read command Add 'OpenIOUDetailsPage' API command Jul 14, 2022
@Julesssss
Copy link
Contributor Author

Final step before I remove the WIP label is to update Onyx. Just waiting for this PR to be merged.

@mountiny
Copy link
Contributor

Merged the onyx PR!

@Julesssss
Copy link
Contributor Author

Thanks! Still held on the Web-E PR actually, but it should be ready now. Finally

@Julesssss Julesssss changed the title Add 'OpenIOUDetailsPage' API command [HOLD on Web-Expensify#34144] Add 'OpenIOUDetailsPage' API command Jul 14, 2022
@Julesssss Julesssss changed the title [HOLD on Web-Expensify#34144] Add 'OpenIOUDetailsPage' API command Add 'OpenIOUDetailsPage' API command Jul 15, 2022
@Julesssss Julesssss marked this pull request as ready for review July 15, 2022 15:07
@Julesssss Julesssss requested a review from a team as a code owner July 15, 2022 15:07
@melvin-bot melvin-bot bot requested review from Justicea83 and removed request for a team July 15, 2022 15:07
@Julesssss
Copy link
Contributor Author

Julesssss commented Jul 15, 2022 •
edited
Loading

@Justicea83 & @vitHoracek -- ready for review 🤞

If you see an error about mergeCollection not being valid, try running npm run clean to force the React-Native-Onyx update.

@mountiny mountiny self-requested a review July 18, 2022 11:46
mountiny
mountiny approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Looks good tome, tested locally and worked well:
image

src/libs/actions/Report.js
@@ -1132,6 +1132,19 @@ function openReport(reportID) {
});
}

/**
* Gets the IOUReport and the associated report actions.
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB:

Suggested change
* Gets the IOUReport and the associated report actions.
* Gets the IOUReport and its associated report actions.

Justicea83
Justicea83 approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@Justicea83 Justicea83 left a comment

Choose a reason for hiding this comment

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

Changes look good to me

@mountiny mountiny changed the title Add 'OpenIOUDetailsPage' API command [HOLD] Add 'OpenIOUDetailsPage' API command Jul 18, 2022
@mountiny
Copy link
Contributor

Just adding HOLD until https://github.com/Expensify/Web-Expensify/pull/34144 hits production so we don't accidentally get this to staging before that would happen.

@Justicea83
Copy link
Contributor

I knew something could happend so I left it unmerged, once bitten..😂

@mountiny mountiny changed the title [HOLD] Add 'OpenIOUDetailsPage' API command Add 'OpenIOUDetailsPage' API command Jul 18, 2022
@mountiny
Copy link
Contributor

And https://github.com/Expensify/Web-Expensify/pull/34144 is in prod. Merging!

@mountiny mountiny merged commit 786c451 into main Jul 18, 2022
@mountiny mountiny deleted the jules-implementOpenIOUDetailsPageAPI branch July 18, 2022 18:19
@melvin-bot melvin-bot bot added the Emergency label Jul 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 18, 2022

@mountiny looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@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.

@mountiny
Copy link
Contributor

The tests were passing, not an emergency.

@OSBotify OSBotify mentioned this pull request Jul 18, 2022
Closed
79 tasks
@Julesssss
Copy link
Contributor Author

Thanks both!

@Julesssss Julesssss mentioned this pull request Jul 19, 2022
Closed
@mountiny
Copy link
Contributor

@Julesssss Np! I see you have linked an additional issue and the steps to reproduce there are a bit different than the test steps in this PR. Should we update those to make sure Applause tests it?

@Julesssss
Copy link
Contributor Author

The new test case should covers that issue too (logging out and back in to clear the data), but I've added it as a 3rd test just to be sure 👍

@mountiny
Copy link
Contributor

Gotcha! Thanks ❤️

@Julesssss
Copy link
Contributor Author

Added a 3rd test.

@mountiny mountiny mentioned this pull request Jul 19, 2022
Closed
@MariaHCD MariaHCD mentioned this pull request Jul 20, 2022
Closed
@neil-marcellini neil-marcellini mentioned this pull request Jul 20, 2022
Closed
neil-marcellini added a commit that referenced this pull request Jul 20, 2022
@neil-marcellini
Revert "Merge pull request #9878 from Expensify/jules-implementOpenIO…
7164394 
…UDetailsPageAPI"

This reverts commit 786c451, reversing
changes made to fa09740.
@neil-marcellini neil-marcellini mentioned this pull request Jul 20, 2022
Closed
93 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @luacmartins in version: 1.1.85-8 🚀

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

@Julesssss Julesssss mentioned this pull request Aug 17, 2022
Closed
93 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Justicea83 Justicea83 Justicea83 approved these changes

@mountiny mountiny mountiny approved these changes

Assignees

@Julesssss Julesssss

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Julesssss @mountiny @Justicea83 @OSBotify