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

Clean up policy actions #6080

Merged
merged 7 commits into from
Nov 4, 2021
Merged

Clean up policy actions #6080

merged 7 commits into from
Nov 4, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Oct 27, 2021

cc @roryabraham Longer term we probably want to solve the issue of why we can't get policies and create them at the same time, but this feels like a logical refactor. I think by splitting these out into their own methods things feel more intentional - even if the behavior is exactly the same for now.

Details

Clean up Policy actions

Fixed Issues

$ #6153

Tests / QA Steps

  1. Log out of NewDot
  2. Sign up for a new account via OldDot on mobile web with a private domain
  3. Choose "Set up my company for free"
  4. Verify that the page opens to NewDot and the workspace is visible
  5. Close the workspace and validate the email
  6. Sign into OldDot with the same account (desktop web) and navigate to policies. Select the workspace.
  7. Verify the page opens to NewDot with the workspace visible.
  8. Sign out of NewDot and repeat steps 5 & 6

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Oct 27, 2021
@marcaaron marcaaron changed the title [WIP] Clean up policy actions Clean up policy actions Nov 1, 2021
@marcaaron marcaaron marked this pull request as ready for review November 1, 2021 21:43
@marcaaron marcaaron requested a review from a team as a code owner November 1, 2021 21:43
@MelvinBot MelvinBot requested review from deetergp and removed request for a team November 1, 2021 21:43
src/libs/actions/Policy.js Show resolved Hide resolved
@roryabraham roryabraham dismissed their stale review November 2, 2021 00:29

I've seen the error in my ways

@@ -27,7 +27,7 @@ import {
} from '../../../components/Icon/Expensicons';
import Permissions from '../../../libs/Permissions';
import ONYXKEYS from '../../../ONYXKEYS';
import {create, isAdminOfFreePolicy} from '../../../libs/actions/Policy';
import * as Policy from '../../../libs/actions/Policy';
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Why * and not just replace create with createAndNavigate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More context here: #5984 (comment)

Basically we are going to change these all eventually so I got to this one now.

Copy link
Contributor

@deetergp deetergp 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 and tests well. Had one NAB question that I'll wait on before merging.

@roryabraham roryabraham merged commit fcd1dc2 into main Nov 4, 2021
@roryabraham roryabraham deleted the marcaaron-fixUpPolicyMethods branch November 4, 2021 03:44
@OSBotify
Copy link
Contributor

OSBotify commented Nov 4, 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

OSBotify commented Nov 4, 2021

🚀 Deployed to staging by @roryabraham in version: 1.1.13-3 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.14-4 🚀

platform result
🤖 android 🤖 failure ❌
🖥 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.

4 participants