Redirect to New Workspace Creation with Invalid Policy Id in Workspace URL #4481

mananjadhav · 2021-08-06T23:27:49Z

@timszot Can you please review?

Details

Added redirect to New Workspace page in case the URL contains invalid policy Id
Moved navigation in workspace creation to next promise then block
Added long duration to the error message

Fixed Issues

$ #4350

Tests

Tested with invalid policyId in the URL (random numbers and characters)
Tested with undefined in the URLs for Policy Id
Tested New Workspace creation and then the redirection with this flow
Verified existing workspace redirects are not impacted.

QA Steps

Tested On

Web
Mobile Web

Screenshots

Web

invalid-policyid-redirect-web.mp4

Mobile Web

invalid-policyid-redirect-mweb.mp4

…e with error message

mananjadhav · 2021-08-06T23:30:24Z

@timszot Work done. Few things with this PR.

Need help with the message translations.
I've added 2 fixes in workspace creation flow too - added .then block and successMessage translations
Error duration is 3500ms, compared to regular 2000ms. Please let me know if 3500ms works.

timszot · 2021-08-09T20:29:24Z

Sorry for the delay here. I'll give this a review and work on getting the translations for you as well, but please give us some time as we're currently prioritising a company wide internal project.

TomatoToaster · 2021-08-11T15:53:44Z

Hey just a note for when this hits QA stage. We're getting rid of the Create Workspace page so these changes will be in effect slightly differently. We are making every instance where you'd see a modal to create a workspace instead just auto-create the workspace with a default name. So here, instead of seeing the modal when the policy in the link doesn't exist, one will just be auto created.

I'd say this doesn't need any further QA when it hits staging and instead the QA will be handled in the other PR where we're changing the Create Workspace behavior. Just making a note here so that not seeing a Create Workspace modal is NOT treated as a regression for this PR.

mananjadhav · 2021-08-11T16:06:06Z

@TomatoToaster Are you saying that with the other PR, my current PR's modal won't show. One workspace with the default name will be auto created and I don't need to handle it in the current PR, right?

TomatoToaster · 2021-08-11T18:51:02Z

Yep you got it. Nothing left to do on your side 👍🏾

isagoico · 2021-08-11T19:03:11Z

@mananjadhav Hello! Any QA needed for this PR?

mananjadhav · 2021-08-11T19:09:16Z

@isagoico From QA, we could just check that it doesn't land up to a blank Workspace Card page?

OSBotify · 2021-08-11T22:18:34Z

🚀 Deployed to staging by @timszot in version: 1.0.83-2 🚀

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

botify · 2021-08-17T18:01:31Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-08-24. 🎊

OSBotify · 2021-08-17T18:18:38Z

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

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

marcaaron

Noticed a bug on dev related to this PR, but unsure if it's causing any significant issues. What I observed was a failure to create a policy and then the app crashes. Unfortunately, I didn't manage to catch why the policy wasn't created.

Gonna create an issue to make sure we handle this case properly.

marcaaron · 2021-09-16T04:56:22Z

src/libs/actions/Policy.js

            Navigation.dismissModal();
-            Navigation.navigate(ROUTES.getWorkspaceCardRoute(response.policyID));
+            Navigation.navigate(ROUTES.getWorkspaceCardRoute(res.policyID));


Heads up, res will be null if we get a non 200 jsonCode here. Since we are returning early in the first .then() block the next once will still go and we end up trying to access policyID on null.

Okay. Give me a day I'll check. Afaik, Workspace creation was manual with the form when this PR was merged and we did test this.

I'll fix it anyway.

@TomatoToaster Does your change cover this as well? or I'll cover this?

Oh hey, I totally did not do this, my bad for missing this comment! I put up a PR here: #5687

marcaaron · 2021-09-16T04:58:06Z

src/pages/workspace/WorkspaceSidebar.js

-    if (_.isEmpty(policy)) {
-        return null;
-    }
+    useEffect(() => {


Please follow the style guide - https://github.com/Expensify/App/blob/main/STYLE.md#are-we-allowed-to-use-insert-brand-new-react-feature-why-or-why-not

Okay. I'll check and update.

I'll mostly be working on this issue #5185 and it involves working on WorkspaceSidebar. I'll update this block of code too when working with this PR.

Will change WorkspaceSidebar to a class component.

Hey just want to note, I actually got rid of the useEffect in one of my PRs when I was editing this component. You won't need to update it!

@TomatoToaster Thanks for the note. :)

mananjadhav added 3 commits August 7, 2021 04:29

fix(invalid-policyid): Redirect invalid PolicyId to new workspace pag…

de3c636

…e with error message

fix(invalid-policyid): Added success message translation

6d53613

fix(invalid-policyid): Promise changes for onyx to get valid id

fd3036f

mananjadhav requested a review from a team as a code owner August 6, 2021 23:27

MelvinBot requested review from timszot and removed request for a team August 6, 2021 23:27

timszot approved these changes Aug 10, 2021

View reviewed changes

timszot merged commit e1b046e into Expensify:main Aug 10, 2021

OSBotify mentioned this pull request Aug 11, 2021

Deploy Checklist: Expensify.cash 2021-08-11 #4554

Closed

53 tasks

marcaaron reviewed Sep 16, 2021

View reviewed changes

marcaaron mentioned this pull request Sep 16, 2021

When failing to create a policy the app will crash due to bad property access #5306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirect to New Workspace Creation with Invalid Policy Id in Workspace URL #4481

Redirect to New Workspace Creation with Invalid Policy Id in Workspace URL #4481

mananjadhav commented Aug 6, 2021

mananjadhav commented Aug 6, 2021

timszot commented Aug 9, 2021

TomatoToaster commented Aug 11, 2021

mananjadhav commented Aug 11, 2021

TomatoToaster commented Aug 11, 2021

isagoico commented Aug 11, 2021

mananjadhav commented Aug 11, 2021

OSBotify commented Aug 11, 2021

botify commented Aug 17, 2021

OSBotify commented Aug 17, 2021

marcaaron left a comment

marcaaron Sep 16, 2021

mananjadhav Sep 17, 2021

mananjadhav Sep 21, 2021

TomatoToaster Oct 6, 2021

marcaaron Sep 16, 2021

mananjadhav Sep 17, 2021

mananjadhav Sep 20, 2021

TomatoToaster Sep 21, 2021 •

edited

Loading

mananjadhav Sep 21, 2021

Redirect to New Workspace Creation with Invalid Policy Id in Workspace URL #4481

Redirect to New Workspace Creation with Invalid Policy Id in Workspace URL #4481

Conversation

mananjadhav commented Aug 6, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

mananjadhav commented Aug 6, 2021

timszot commented Aug 9, 2021

TomatoToaster commented Aug 11, 2021

mananjadhav commented Aug 11, 2021

TomatoToaster commented Aug 11, 2021

isagoico commented Aug 11, 2021

mananjadhav commented Aug 11, 2021

OSBotify commented Aug 11, 2021

botify commented Aug 17, 2021

OSBotify commented Aug 17, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomatoToaster Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomatoToaster Sep 21, 2021 •

edited

Loading