Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.79k forks source link

[$250] Multi tags - Tag disappears after reordering tags and app crashes after disabling all tags #44626

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 3 months ago

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-3.2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers):
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Expected Result:

In Step 8, multi tag A will not disappear after reordering tags In Step 10, app will not crash

Actual Result:

In Step 8, multi tag A disappears after reordering tags In Step 10, app crashes after disabling all the tags

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/b14eea7d-5d75-4d30-9ae6-d35dd869a52c

Bug6527371_1719578072235!Independent.-.Multi.Level.tags.csv

logs (2).txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0123c6d5064aec7c2e
  • Upwork Job ID: 1806779895355813021
  • Last Price Increase: 2024-07-05
Issue OwnerCurrent Issue Owner: @yuwenmemon
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @Christinadobrzyn (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 commented 3 months ago

@Christinadobrzyn 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 commented 3 months ago

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

Christinadobrzyn commented 3 months ago

I'm not getting the crash in testing but I am seeing the missing tag after re-organisation - I think this can be external.

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0123c6d5064aec7c2e

melvin-bot[bot] commented 3 months ago

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

suneox commented 3 months ago

Proposal

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

Filter invalid policy tags

What is the root cause of that problem?

POLICY_TAGS sync data delay & inconsistency when reorder and disable tags group and some policy tag missing tags

Screenshot 2024-06-29 at 23 23 07

then we have this logic check hasEnabledTags but tags in undefined so app will crash when execute Object.values(tags)

https://github.com/Expensify/App/blob/4622e19c1da5137bc7b227f5b65bf3d83f96a909/src/libs/OptionsListUtils.ts#L1366

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

We can filter valid tags when get policyTagLists

function getTagLists(policyTagList: OnyxEntry<PolicyTagList>): Array<ValueOf<PolicyTagList>> {
    if (isEmptyObject(policyTagList)) {
        return [];
    }

    return Object.values(policyTagList)
-       .filter((policyTagListValue) => policyTagListValue !== null)
+       .filter((policyTagListValue) => policyTagListValue !== null && !!policyTagListValue.tags)
        .sort((tagA, tagB) => tagA.orderWeight - tagB.orderWeight);
}

Or filter valid tags before execute Object.values(tags)

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

What alternative solutions did you explore? (Optional)

parasharrajat commented 3 months ago

@suneox There are two issues in OP. One is about disappearing tags and the other is about the crash. You didn't explain the solution for the first one.

suneox commented 3 months ago

@suneox There are two issues in OP. One is about disappearing tags and the other is about the crash. You didn't explain the solution for the first one.

@parasharrajat About the first issue is caused by inconsistent synced data at this line

Inconsistent synced data https://github.com/Expensify/App/assets/11959869/aaae4dcd-e6b2-4f2c-85e3-d8f76833f54e
parasharrajat commented 3 months ago

Can you elaborate on it? What do you mean by inconsistent sync? You are referring to the Onyx subscription.

suneox commented 3 months ago

Can you elaborate on it? What do you mean by inconsistent sync? You are referring to the Onyx subscription.

I mention Onyx subscription when changes in order or disabled tags in the classic app, the data on the new app is not updated correctly. I also tried to sign out and log in again, but the policy tags are still different between the classic app and the new app.

parasharrajat commented 3 months ago

Ok. Thanks. I believe both of these issues are backend problems. tags is a required key on the PolicyTag structure so the backend should always return a value for this key even if it is empty {}.

There are a few issues.

  1. I saw that policy tags data has some lists where the tags key is [] which is supposed to be {}.
  2. Secondly the the tags key should always be present, it can't be null.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 3 months ago

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

parasharrajat commented 3 months ago

@AndrewGable Could you please check this?

suneox commented 3 months ago

I found other places that also check for existing tags before getting the object value, so I think we should check on the client side to avoid crashes.

parasharrajat commented 3 months ago

That part I agree so we should probably remove those checks everywhere when backend is fixed. let's wait for @AndrewGable to look into this.

AndrewGable commented 3 months ago

@yuwenmemon - Do you mind stepping in here as the multi tag expert?

yuwenmemon commented 3 months ago

Sure thing

Christinadobrzyn commented 2 months ago

Just checking - do we have an update for Melvin?

parasharrajat commented 2 months ago

@yuwenmemon Bunmp

yuwenmemon commented 2 months ago

I'm OOO for the July 4th holiday so will circle back on this on Monday hopefully.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@yuwenmemon, @parasharrajat, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Christinadobrzyn commented 2 months ago

Hi @yuwenmemon do you have an update on this for Melvin?

yuwenmemon commented 2 months ago

Not yet, but switching this to internal as per the conversation above.

melvin-bot[bot] commented 2 months ago

@yuwenmemon @parasharrajat @Christinadobrzyn this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Christinadobrzyn commented 2 months ago

Update for Melvin - This is going to be worked on when we have the bandwidth

Christinadobrzyn commented 2 months ago

I think this is still in a holding pattern

melvin-bot[bot] commented 2 months ago

@yuwenmemon, @parasharrajat, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Christinadobrzyn commented 2 months ago

@yuwenmemon do you think you'll have the bandwidth to work on this? If not, I can reach out to the channel for another volunteer!

melvin-bot[bot] commented 2 months ago

@yuwenmemon, @parasharrajat, @Christinadobrzyn Still overdue 6 days?! Let's take care of this!

Christinadobrzyn commented 2 months ago

I think this is still in a holding pattern - waiting for a volunteer

yuwenmemon commented 2 months ago

Looking at this today

yuwenmemon commented 2 months ago

No longer seeing this crash. Additionally don't see the disappearing tag issue either. Closing:

https://github.com/user-attachments/assets/f8716caf-1fdd-475f-b231-0727b340000e

An argument could be made that the tag order should update in real-time when the tags are reordered by the admin but that's such an edge case I'm fine ignoring that for now.