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

Get&returnValueList=reportSummaryList #3894

Merged
merged 5 commits into from
Jul 14, 2021
Merged

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Jul 7, 2021

Details

Replaces Get&returnValueList=reportStuff with reportSummaryList.

This cuts initial fetching and re-fetching of reports from ~ 7 seconds to 1.5 seconds on a test account with many large chats and should speed things up for users.

Fixed Issues

$ #3875

Tests

Note: Pull Auth main branch and re-build before testing!

  1. Create an account with several hundred reports each having 1000+ comments (clitools can help with this)
  2. Sign in and verify that you are able to do so and the LHN populates in a reasonable amount of time less than a couple of seconds and not 5-7 seconds.
  3. We are looking for the response time of the command Get&returnValueList=reportSummaryList which can be analyzed in the network tab

QA Steps

For QA we'll do the same steps but ask @JmillsExpensify or someone with a large number of chats to test this out for us on staging.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Jul 7, 2021
@marcaaron marcaaron changed the title [WIP] Get&returnValueList=reportSummaryList [HOLD Auth 5747] Get&returnValueList=reportSummaryList Jul 7, 2021
@marcaaron
Copy link
Contributor Author

marcaaron commented Jul 13, 2021

This is still on HOLD (waiting for Auth deploy) but I'm moving it into reviews

@marcaaron marcaaron marked this pull request as ready for review July 13, 2021 18:40
@marcaaron marcaaron requested a review from a team as a code owner July 13, 2021 18:40
@MelvinBot MelvinBot requested review from Gonals and removed request for a team July 13, 2021 18:40
@Jag96 Jag96 self-requested a review July 13, 2021 21:52
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Ok, after some back and forth w/ testing it looks like if the user has a pinned chat, the getReportSummaryList command fails in auth

Logs for user without any pinned chats (tested by navigating to https://www.expensify.com/api?command=Get&returnValueList=reportSummaryList&reportIDList=MY_REPORT_ID): logs

Logs for user with pinned chats (same URL, just pinned a chat and refreshed): logs

@Jag96 Jag96 changed the title [HOLD Auth 5747] Get&returnValueList=reportSummaryList [HOLD Auth 5769] Get&returnValueList=reportSummaryList Jul 14, 2021
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

With the latest changes in https://github.com/Expensify/Auth/pull/5769 this is working great for me!

@marcaaron marcaaron changed the title [HOLD Auth 5769] Get&returnValueList=reportSummaryList Get&returnValueList=reportSummaryList Jul 14, 2021
@marcaaron
Copy link
Contributor Author

Looks like the auth change has been deployed so I'm going to merge this guy 🤞

@marcaaron marcaaron merged commit 480ddc6 into main Jul 14, 2021
@marcaaron marcaaron deleted the marcaaron-getReportSummaryList branch July 14, 2021 18:04
@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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

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

@isagoico
Copy link

@marcaaron @JmillsExpensify Hello! let us know when this is tested so we can check it off the list 🙇

@marcaaron
Copy link
Contributor Author

I think we probably want to make this a blocker and wait until https://github.com/Expensify/Auth/pull/5780 has been deployed. Right, @Jag96 ?

@marcaaron marcaaron added the DeployBlockerCash This issue or pull request should block deployment label Jul 19, 2021
@github-actions github-actions bot added the Hourly KSv2 label Jul 19, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@marcaaron marcaaron removed the DeployBlockerCash This issue or pull request should block deployment label Jul 19, 2021
@marcaaron
Copy link
Contributor Author

nvm left more context on the checklist we can check this one off I think..

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants