[HOLD for payment 2024-07-22] [$250] Tag - App crashes when saving 0 as custom tag name

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.83-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4631162
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace settings > Tags
3. Click Settings > Custom tag name
4. Enter 0 and save it

Expected Result:

App will not crash

Actual Result:

App crashes when saving 0 as custom tag name

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6512905_1718353988266.bandicam_2024-06-14_16-29-17-888.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01aa311cffaf18adc9
• Upwork Job ID: 1802720126377724186
• Last Price Increase: 2024-06-24
• Automatic offers:
• ZhenjaHorbach | Reviewer | 102869898
• bernhardoj | Contributor | 102869900

Issue Owner

Current Issue Owner: @danieldoglas

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@slafortune FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #wave-collect - Release 1

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

App crash when trying to save tag name as 0.

What is the root cause of that problem?

When we save the tag name as 0, the BE responds with an invalid error. If it fails, we set an errors object to the policy tag list collection.

App/src/libs/actions/Policy/Tag.ts

Lines 557 to 563 in f0b6355

	value: {
	errors: {
	[oldName]: oldName,
	[newName]: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'),
	},
	[newName]: null,
	[oldName]: oldPolicyTags,

So, it becomes:

{
errors: {...},
TagName: {...},
}

Then, it crashes in this line of code

App/src/pages/workspace/tags/WorkspaceTagsSettingsPage.tsx

Line 36 in f0b6355

const hasEnabledOptions = OptionsListUtils.hasEnabledOptions(Object.values(policyTags ?? {}).flatMap(({tags}) => Object.values(tags)));

The code gets the values of the object and then extracts the tags property from them. TagName has the tags property, but errors doesn't have the tags property because it's not a tag.

What changes do you think we should make in order to solve the problem?

If we see the offline with feedback here,

App/src/pages/workspace/tags/WorkspaceTagsSettingsPage.tsx

Lines 76 to 78 in f0b6355

	<OfflineWithFeedback
	errors={policyTags?.[policyTagLists[0].name]?.errors}
	pendingAction={policyTags?.[policyTagLists[0].name]?.pendingAction}

we can see that the component expects the error property to be added inside the tag object, just like what we did with the pendingAction.

App/src/libs/actions/Policy/Tag.ts

Line 538 in f0b6355

[newName]: {...oldPolicyTags, name: newName, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD},

So, we need to update the failure data to

[newName]: null,
[oldName]: {
    ...oldPolicyTags,
    errors: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'),
},

Last thing we need to do is to clear the error when pressing the offline with feedback close (X) button.

[kpadmanabhan]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Naming or renaming policy tag list setting with name as '0' crashes the app with failure.

What is the root cause of that problem?

Error in console.
src\pages\workspace\tags\WorkspaceTagsSettingsPage.tsx

Uncaught TypeError: Cannot convert undefined or null to object
    at Function.values (<anonymous>)
    at eval (WorkspaceTagsSettingsPage.tsx:35:1)
    at Array.flatMap (<anonymous>)
    at WorkspaceTagsSettingsPage (WorkspaceTagsSettingsPage.tsx:33:138)
    at renderWithHooks (react-dom.development.js:16175:18)
    at updateFunctionComponent (react-dom.development.js:20382:20)
    at beginWork (react-dom.development.js:22425:16)
    at HTMLUnknownElement.callCallback (react-dom.development.js:4161:14)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:4210:16)
    at invokeGuardedCallback (react-dom.development.js:4274:31)

App calls backend API RenamePolicyTaglist with valid oldName property and newName property as '0'. Backend responds back with error.

{
    "code": 666,
    "jsonCode": 402,
    "type": "Expensify\\Error\\InvalidParameterError",
    "UUID": "4456F600-4114-4B54-A7BD-4C3431656E18",
    "message": "Invalid input parameter",
    "title": "",
    "data": [],
    "htmlMessage": "",
    "onyxData": [],
    "requestID": "893aeca77e751c99-AMS"
}

While processing the response after converting policyTags to flatMap, which expects tag names inside it (instead of generic error that comes back from API), the code fails to fetch tags.

var hasEnabledOptions = OptionsListUtils.hasEnabledOptions(Object.values(policyTags !== null && policyTags !== void 0 ? policyTags : {}).flatMap(function (_ref2) {
    var tags = _ref2.tags;
    return Object.values(tags);
  }));

Code expects property tags inside the response. This works fine in case of a successful response as there is tags property inside tagsListData . But in case of errors, though response code is HTTP 200, tags property is completely missing.
As the code always assumes the presence of tags property, this fails in case of errors.

What changes do you think we should make in order to solve the problem?

Assuming that we always get a successful response is incorrect. Frontend code must be able to handle error responses in a graceful way.

Suggest following approaches for this

1. Check that tags property is not undefined. If that is missing, fallback for error response and fetch and display appropriate error as a red error text under tagList setting name field, based on the error message returned from backend. Display options for error need UI/UX input.
2. Alternatively, accept only valid tag names from frontend using regex. Also as the field is pre-populated with value Tag, do not allow changing the pre-populated text, instead forcefully accept only appending to this value. Need product input on this.
3. What are possible valid tag names and invalid tag names? Why is '0' an invalid tag name? Would like to understand this requirement to help in choosing the best option.

What alternative solutions did you explore? (Optional)

Backend to return tags as empty property in API response and add error as a different property, rather than replacing tags property.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01aa311cffaf18adc9

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach (External)

[ZhenjaHorbach]

Thanks everyone for proposals !

I agree with @bernhardoj proposal

Given that we already have an error handler for WorkspaceTagsSettingsPage

And we don't need to pass errors separately from tag object since it looks like unexpected behavior (Actually it's only place where we pass errors separately from tag object for ONYXKEYS.COLLECTION.POLICY_TAGS )

App/src/libs/actions/Policy/Tag.ts

Lines 557 to 564 in f0b6355

	value: {
	errors: {
	[oldName]: oldName,
	[newName]: ErrorUtils.getMicroSecondOnyxError('workspace.tags.genericFailureMessage'),
	},
	[newName]: null,
	[oldName]: oldPolicyTags,
	},

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[danieldoglas]

Interesting, checking this on our side before assigning - I'm not sure if we should allow 0 as a tag name.

[danieldoglas]

We should allow 0 on our side. I'm working on a fix for that.

[bernhardoj]

@danieldoglas I think we can fix the FE first to handle the errors properly, then fix the BE to allow 0, otherwise, the app will crash again every time there is an edit tag error. What do you think?

[ZhenjaHorbach]

Agree
We still have incorrect failureData for renamePolicyTag
And to prevent similar crashes, it would be nice to fix this

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[kpadmanabhan]

@danieldoglas / @ZhenjaHorbach : is someone working on this on FE? Or can we consider this proposal?

[ZhenjaHorbach]

@kpadmanabhan
Thank you for your proposal

We are not doing FE part yet

But I still think the right thing to do is fix failureData for renamePolicyTag
Since this is the only place where we use errors separately from tags
Plus this doesn't match the typing for ONYXKEYS.COLLECTION.POLICY_TAGS

[kpadmanabhan]

@ZhenjaHorbach : I agree completely on this. failureData for renamePolicyTag must be fixed. I belive that in addition to this there needs to be safe response handling done in frontend.

[danieldoglas]

I agree we need to fix the front end to deal with this as well.

[melvin-bot]

📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @ZhenjaHorbach

[danieldoglas]

Backend changes implemented, waiting to be merged/deployed

[danieldoglas]

This is kind of done?

@slafortune not sure why it didn't create the comments, the app part it's already in production!

[slafortune]

Thanks for the heads up @danieldoglas - so are we just in the 7 day holding period for regressions? Which would be 7/17

[ZhenjaHorbach]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:

Crash is related to this PR where we try to get tags property from errors object
But the main root cause where we pass errors separately from tag objects is related to this PR

• [@ZhenjaHorbach] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

https://github.com/Expensify/App/pull/42004/files#r1681339465
https://github.com/Expensify/App/pull/37734/files#r1681340313

• [@ZhenjaHorbach] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

NA

• [@ZhenjaHorbach] Determine if we should create a regression test for this bug.

I'm not sure we need a test for this bug
Because it's much harder to reproduce now
Since 0 is now a valid value
Plus I can't think of a stable way how we can throw an error for testing

• [@ZhenjaHorbach] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

But just in case I added test steps

Regression Test Proposal

1. Go to workspace tags page
2. Press Settings
3. Press the tag name
4. Rename the tag to invalid value
5. When it fails, verify the tag name reverts to the previous name
6. Press the close error button
7. Verify the error is cleared

Do we agree 👍 or 👎 ?

[bernhardoj]

@slafortune yes, the payment should be due today (7/17). I have requested the payment in ND.

[slafortune]

@ZhenjaHorbach - C+ Role - has been paid $250 via UpWorks
@bernhardoj - Contributor Role - is due $250 via NewDot

[JmillsExpensify]

$250 approved for @bernhardoj