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

[marcaaron]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

Coming in from #4481 (review)

Try to create a workspace and it fails for some reason (I didn't catch why)

Expected Result:

Some useful feedback about what went wrong.

Actual Result:

App crashes because we try to access a property on an API response that is null

Workaround:

I'm not sure why the policy wouldn't be created but probably we can look at the backend to find out some reasons why.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

[marcaaron]

@kevinksullivan This kind of feels like a possible N6 blocker, but I didn't catch why the response failed so unsure if it's worth prioritizing. Basically something in this API command throws when trying to create a free policy and then the app crashes and we should make sure this is not possible + add better error handling for this flow.

https://github.com/Expensify/Web-Expensify/blob/b8233a0d4c05d6efcb1d58928f04532ba472cd88/lib/PolicyAPI.php#L42

[kevinksullivan]

Hm I have not seen this one yet. Was there any unique steps you took to get here @marcaaron ? Also, when you say the app crashes, it completely closes out, right? So what would error handling look like in this context?

[marcaaron]

I didn't experience any bug so I don't have a reproduction, it's just something I noticed that must be cleaned up. If something does go wrong then my guess would be the app would crash (I don't have time to verify this right now). As long as this API returns a jsonCode 200 we won't have anything to worry about (but I'm pretty sure it can return other codes).

[aldo-expensify]

I did a small test hardcoding a throw in https://github.com/Expensify/Web-Expensify/blob/b8233a0d4c05d6efcb1d58928f04532ba472cd88/lib/PolicyAPI.php#L42

throw new ExpError('67eb67e4-eb23-410f-8de4-b61f3ae7ed3f', 'Your admin has restricted your group from creating new policies.', 'Policy Creation Restricted');

Good news is the app doesn't fully crash, at least with this kind of error because it is handled by this code:

App/src/libs/actions/Policy.js

Lines 223 to 228 in f7b24d7

	if (response.jsonCode !== 200) {
	// Show the user feedback
	const errorMessage = translateLocal('workspace.new.genericFailureMessage');
	Growl.error(errorMessage, 5000);
	return;
	}

but the error message coming from the back is lost. Do we want to make some improvements to try to show the error?, since it is just an error message from the back, it would not be easy to internationalize.

The back responds with:

  UUID: "67eb67e4-eb23-410f-8de4-b61f3ae7ed3f",
  code: 666,
  data: [],
  htmlMessage: "",
  jsonCode: 666,
  message: "Your admin has restricted your group from creating new policies.",
  requestID: "WnjhRW",
  title: "Policy Creation Restricted",
  type: "Expensify\\Libs\\Error\\ExpError"
}

[marcaaron]

Hmm but we are still accessing a property that doesn't exist. Looks like you tested this on web @aldo-expensify, can you try to verify what happens on a mobile release build? I think it will crash.

[aldo-expensify]

Hmm but we are still accessing a property that doesn't exist. Looks like you tested this on web @aldo-expensify, can you try to verify what happens on a mobile release build? I think it will crash.

Yep, I tested on web, I'll test on mobile.

[aldo-expensify]

can you try to verify what happens on a mobile release build? I think it will crash.

Followed steps to test release build, same behaviour, I can see the growl:

Screen.Recording.2021-09-22.at.2.48.49.PM.mov

Hmm but we are still accessing a property that doesn't exist.

Are you saying that it is possible for the response variable to be equal to null? Do you know under which conditions this can happen?

I see that there is no catch, but that is the case in many API calls.

[MelvinBot]

Eep! 4 days overdue now. Issues have feelings too...

[MelvinBot]

Still overdue 6 days?! Let's take care of this!

[MelvinBot]

10 days overdue. I'm getting more depressed than Marvin.

[MelvinBot]

12 days overdue. Walking. Toward. The. Light...

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[danieldoglas]

This seems to have been addressed in this PR $#5687

@johnmlee101 is it ok to close this?

[johnmlee101]

Cool if you can't reproduce I think its good to close out.