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

[HOLD for payment][$250] IOU - App crashes when admin pay elsewhere #47542

Closed
3 of 6 tasks
IuliiaHerets opened this issue Aug 16, 2024 · 18 comments
Closed
3 of 6 tasks

[HOLD for payment][$250] IOU - App crashes when admin pay elsewhere #47542

IuliiaHerets opened this issue Aug 16, 2024 · 18 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 16, 2024

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: 9.0.21-0
Reproducible in staging?: Y
Reproducible in production?: Y
Issue was found when executing this PR #47398
Email or phone of affected tester (no customers): applausetester+gm103030@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Create a Control workspace.
  2. Submit an expense as an employee.
  3. Pay the expense as the admin.
  4. As admin navigate to payed thread transaction

Expected Result:

App should not crash

Actual Result:

App crashes when admin pay elsewhere

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6573451_1723765657184.Recording__3745.2.mp4

1608_1.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01374ae222703cc250
  • Upwork Job ID: 1825649554698082303
  • Last Price Increase: 2024-08-19
Issue OwnerCurrent Issue Owner: @thesahindia
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

Triggered auto assignment to @johncschuster (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.

@IuliiaHerets
Copy link
Author

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

@IuliiaHerets
Copy link
Author

@johncschuster 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

@bernhardoj
Copy link
Contributor

Proposal

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

App crash when employee submit request and then the admin open the expense report.

What is the root cause of that problem?

The error comes from hasEnabledTags function.

const shouldShowTag = isPolicyExpenseChat && (transactionTag || OptionsListUtils.hasEnabledTags(policyTagLists));

Inside the function, we map over the tag array, and the map callback destructuring the tag by taking the tags property, but in our case, the tags is undefined, so Object.values throws an error.

function hasEnabledTags(policyTagList: Array<PolicyTagLists[keyof PolicyTagLists]>) {
const policyTagValueList = policyTagList.map(({tags}) => Object.values(tags)).flat();

The tag array (policyTagList) normally looks like this:

[{name: "Tag", orderWeight: 0, required: false, tags: {...}}]

The array is constructed from the tag object by taking the object values.

App/src/libs/PolicyUtils.ts

Lines 274 to 279 in cf897a0

function getTagLists(policyTagList: OnyxEntry<PolicyTagLists>): Array<ValueOf<PolicyTagLists>> {
if (isEmptyObject(policyTagList)) {
return [];
}
return Object.values(policyTagList)

But in our case, the tag list looks like this:
image

Each object in the array structure is totally different from the normal structure above and doesn't have a tags property, so the app crashes.

The reason of that is because, when the admin receives the pusher data of the new money request, the transaction thread report contains a very minimal information which doesn't include policyID, so when we are trying to get the policy tag object, the policyID defaults to an empty string and fetch the whole tag collections.

policyTagList: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_TAGS}${report?.policyID ?? ''}`,
},

policy_A: {
   Tag: {},
},
policy_B: {
    Tag: {},
}

So, when we get the tag object values, we get [{Tag: {}}, {Tag: {}}] instead of [{name: "Tag", orderWeight: 0, required: false, tags: {...}}].

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

We need to default to -1 here so it won't fetch the whole collection of tag. If it's -1, then the policyTagList will be undefined and the array will be empty.

policyTagList: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_TAGS}${report?.policyID ?? ''}`,
},

We can default to -1 for other onyx data in the file too.

policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report?.policyID ?? ''}`,
},
policyCategories: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${report?.policyID ?? ''}`,
},
policyTagList: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_TAGS}${report?.policyID ?? ''}`,
},
parentReport: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID ?? ''}`,
},

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Aug 19, 2024
@melvin-bot melvin-bot bot changed the title IOU - App crashes when admin pay elsewhere [$250] IOU - App crashes when admin pay elsewhere Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01374ae222703cc250

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@thesahindia
Copy link
Member

@bernhardoj's proposal looks good!

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 21, 2024

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

@chiragsalian
Copy link
Contributor

Proposal LGTM. Feel free to create the PR @bernhardoj.
Also add a comment explaining why the default is -1 otherwise, it could be confusing for someone looking at the code

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 22, 2024
@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Aug 23, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @thesahindia

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 23, 2024
@johncschuster johncschuster changed the title [$250] IOU - App crashes when admin pay elsewhere [HOLD for payment][$250] IOU - App crashes when admin pay elsewhere Sep 10, 2024
@johncschuster
Copy link
Contributor

Payment Summary:

Contributor: @bernhardoj owed $250 via NewDot
Contributor+: @thesahindia owed $250 via NewDot

@johncschuster
Copy link
Contributor

@bernhardoj / @thesahindia do do we need regression test steps? If so, can you provide them?

@thesahindia
Copy link
Member

thesahindia commented Sep 15, 2024

Yes! We should add a test case. Here are the steps:-

  1. Create a Control workspace.
  2. Submit an expense as an employee.
  3. Pay the expense as the admin.
  4. As admin navigate to paid transaction thread
  5. Verify you don't see any error.

@johncschuster
Copy link
Contributor

Thanks for those steps! Please request payment from NewDot!

@bernhardoj
Copy link
Contributor

Requested in ND.

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

@garrettmknight
Copy link
Contributor

$250 approved for @thesahindia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: Done
Development

No branches or pull requests

7 participants