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 infinite loop in withFullPolicy HOC #6531

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

roryabraham
Copy link
Contributor

Details

We were previously relying on a route's path param (the visible path for the route in the URL). The problem is that param is optional, and is only present if the app is opened via a deep-link (or via direct URL manipulation on web). Instead, we'll rely on the route name, which is required, and the route params, which will be there if provided.

Fixed Issues

$ #6526

Tests

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

QA Steps

  1. Sign into NewDot with a new account.
  2. Create a workspace.
  3. Verify using the network console that there is no infinite loop performing GET requests to fetch a policy.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@roryabraham roryabraham requested a review from a team as a code owner November 30, 2021 02:11
@roryabraham roryabraham self-assigned this Nov 30, 2021
@MelvinBot MelvinBot requested review from pecanoro and removed request for a team November 30, 2021 02:11
@roryabraham
Copy link
Contributor Author

roryabraham commented Nov 30, 2021

I personally think this issue is severe enough that we should CP the fix to staging.

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

LGTM and works well

@chiragsalian
Copy link
Contributor

Discussed 1:1 with rory and we feel like this should go out the door sooner (context). So gonna go ahead and merge this now.

@chiragsalian chiragsalian merged commit 8d8395f into main Nov 30, 2021
@chiragsalian chiragsalian deleted the Rory-FixInfiniteLoopInWithFullPolicy branch November 30, 2021 03:05
github-actions bot pushed a commit that referenced this pull request Nov 30, 2021
…lPolicy

Fix infinite loop in withFullPolicy HOC

(cherry picked from commit 8d8395f)
@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 2021

🚀 Deployed to staging by @chiragsalian in version: 1.1.17-8 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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.

None yet

3 participants