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] Only load full policies one-by-one as needed #6087

Merged
merged 19 commits into from
Nov 5, 2021
Merged

Conversation

roryabraham
Copy link
Contributor

Details

So we want to make sure that whenever a workspace or any of its sub-pages attempts to render, then we'll load the full policy for that workspace. So whenever we navigate to a workspace page (and did not just come from one), then we'll load that full policy.

Fixed Issues

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

Tests

  1. Sign into NewDot using an account with a workspace.
  2. Go to /settings. Verify using logs or the network console that no full policy is loaded.
  3. Click on an existing workspace. Verify using logs or the network console that the full policy is loaded.
  4. Refresh the page. Verify using logs or the network console that the full policy is loaded.
  5. Click on the back arrow. Verify you are taken back to the settings page.
  6. Click on an existing workspace. Verify using logs or the network console that the full policy is loaded.
  7. Click on a workspace sub-page. Verify using logs or the network console that no full policy is loaded.
  8. Refresh the page on one of the workspace sub-pages. Verify using logs or the network console that the full policy is loaded.
  9. Click on the x to close the modal. Verify that the full settings modal closes.

QA Steps

Only regression tests.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@roryabraham roryabraham requested review from iwiznia and flodnv October 27, 2021 20:54
@roryabraham roryabraham self-assigned this Oct 27, 2021
@roryabraham roryabraham requested a review from a team as a code owner October 27, 2021 20:54
@MelvinBot MelvinBot requested review from Gonals and removed request for a team October 27, 2021 20:55
@roryabraham
Copy link
Contributor Author

Something I missed in this implementation – the WorkspaceInvitePage is in it's own navigator, so if you link directly to the workspace invite page then you're not guaranteed to have a policy loaded. I think this is just an oversight and it should really be in the same navigator as the rest of the workspace pages.

@tgolen tgolen removed the request for review from flodnv November 2, 2021 19:25
@roryabraham
Copy link
Contributor Author

Something I missed in this implementation – the WorkspaceInvitePage is in it's own navigator, so if you link directly to the workspace invite page then you're not guaranteed to have a policy loaded. I think this is just an oversight and it should really be in the same navigator as the rest of the workspace pages.

I went ahead and addressed this and moved the WorkspaceInvitePage into the same navigator as the rest of the workspace pages.

@roryabraham roryabraham requested review from iwiznia and tgolen November 3, 2021 01:22
@roryabraham
Copy link
Contributor Author

Updated and ready for review!

@roryabraham roryabraham requested a review from iwiznia November 3, 2021 18:53
iwiznia
iwiznia previously approved these changes Nov 3, 2021
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I chatted with Rory 1:1 and suggested the following changes:

  1. Change API.getPolicyList() to only accept a single string argument for policyID and throw otherwise (this will remove the need to have a log)
  2. Change the HOC so that it passes the policy object to the consumers (this is a more typical pattern for an HOC so it would make it more consistent)
  3. Make sure the only pages that need the policy object are the ones using the HOC

@roryabraham
Copy link
Contributor Author

PR has been updated per @tgolen's comment above ^

@roryabraham roryabraham requested review from tgolen and iwiznia November 3, 2021 21:39
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Looks really good. Just this one thing I noticed.

@roryabraham roryabraham requested a review from tgolen November 3, 2021 21:58
tgolen
tgolen previously approved these changes Nov 3, 2021
@iwiznia
Copy link
Contributor

iwiznia commented Nov 4, 2021

You got conflicts

@Gonals
Copy link
Contributor

Gonals commented Nov 4, 2021

Looking good overall, but there's a conflict!

@roryabraham roryabraham requested a review from tgolen November 4, 2021 17:11
@roryabraham
Copy link
Contributor Author

Conflicts resolved!

.then((data) => {
if (data.jsonCode === 200) {
const policy = data.policyList[0] || {};
const policy = lodashGet(data, 'policyList[0]', {});
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: Did not know this worked, thought you had to do lodashGet(data, ['policyList', 0], {});

@Gonals Gonals merged commit 0a92efa into main Nov 5, 2021
@Gonals Gonals deleted the Rory-KillPolicyList branch November 5, 2021 15:51
@OSBotify
Copy link
Contributor

OSBotify commented Nov 5, 2021

✋ 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 by @Gonals in version: 1.1.14-5 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀

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.

5 participants