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

[Awaiting payment] [$250] [Dupe detection] Description review appears when distance expenses are created without description #47974

Closed
6 tasks done
IuliiaHerets opened this issue Aug 24, 2024 · 26 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 24, 2024

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


Version Number: v9.0.24-1
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat.
  3. Submit two same distance expenses without description.
  4. Go to transaction thread of any of the expense.
  5. Click Review duplicates.
  6. Click Keep this one (any).

Expected Result:

Description review will not show up because both distance expenses are created without description.

Actual Result:

Description review appears when both distance expenses are created without description.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6581468_1724512231051.20240824_230448.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d6cf4756c6ab01be
  • Upwork Job ID: 1828335490161750402
  • Last Price Increase: 2024-08-27
Issue OwnerCurrent Issue Owner: @adelekennedy
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 24, 2024
Copy link

melvin-bot bot commented Aug 24, 2024

Triggered auto assignment to @adelekennedy (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-control

@IuliiaHerets
Copy link
Author

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 25, 2024

Proposal

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

Dupe detect -Description review appears when distance expenses are created without description

What is the root cause of that problem?

  1. When we create distance request the comment object includes waypoints and each waypoint has unique key, due to this areAllCommentsEqual always returns false for distance transactions.

// Helper function to check if all comments are equal
function areAllCommentsEqual(items: Array<OnyxEntry<Transaction>>, firstTransaction: OnyxEntry<Transaction>) {
return items.every((item) => lodashIsEqual(item?.comment, firstTransaction?.comment));
}

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

We should remove the keyForList property from each waypoint before comparing the comment object.

    const getTransactionCommentWithoutWaypointKeys = (firstTransaction: OnyxEntry<Transaction['comment']>) => {
        // Remove keyForList from each waypoint
        const updatedWaypoints = Object.fromEntries(Object.entries(firstTransaction?.waypoints ?? {}).map(([key, waypoint]) => [key, (({keyForList, ...rest}) => rest)(waypoint)]));

        // Create the new object
        const updatedData = {
            ...firstTransaction,
            waypoints: updatedWaypoints,
        };

        return updatedData;
    };

    // Helper function to check if all comments are equal
    function areAllCommentsEqual(items: Array<OnyxEntry<Transaction>>, firstTransaction: OnyxEntry<Transaction>) {
        return items.every((item) => {
            if (item?.comment?.waypoints) {
                return lodashIsEqual(getTransactionCommentWithoutWaypointKeys(item.comment), getTransactionCommentWithoutWaypointKeys(firstTransaction?.comment));
            }
            return lodashIsEqual(item?.comment, firstTransaction?.comment);
        });
    }

What alternative solutions did you explore? (Optional)


The allCommentsAreEmpty also doesn't seem to be correct item?.comment === undefined.

const allCommentsAreEmpty = isFirstTransactionCommentEmptyObject && transactions.every((item) => item?.comment === undefined);

We can update item?.comment === undefined to:

  1. item?.comment === undefined to !item?.comment?.comment.
  2. typeof item?.comment === 'object' && item?.comment?.comment === ''.
  3. Or item?.comment === undefined || item?.comment?.comment === ''

Result

discription_confirmation_issue.mp4

@daledah
Copy link
Contributor

daledah commented Aug 25, 2024

Proposal

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

  • Dupe detect -Description review appears when distance expenses are created without description

What is the root cause of that problem?

  • When comparing two transactions to check if their description data are equal or not:

    const allCommentsAreEqual = areAllCommentsEqual(transactions, firstTransaction);
    const allCommentsAreEmpty = isFirstTransactionCommentEmptyObject && transactions.every((item) => item?.comment === undefined);

    we use incorrect data. The description data should should transaction.comment.comment, but we use transaction.comment:
    return items.every((item) => lodashIsEqual(item?.comment, firstTransaction?.comment));

    const allCommentsAreEmpty = isFirstTransactionCommentEmptyObject && transactions.every((item) => item?.comment === undefined);

  • In case of distance request, the transaction.comment data for each transaction is not equal because they contains the waypoints data as well.

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

        return items.every((item) => lodashIsEqual(getDescription(item), getDescription(firstTransaction)));

const allCommentsAreEmpty = isFirstTransactionCommentEmptyObject && transactions.every((item) => item?.comment === undefined);

                const allCommentsAreEmpty = isFirstTransactionCommentEmptyObject && transactions.every((item) => getDescription(item) === '');

What alternative solutions did you explore? (Optional)

@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Aug 27, 2024
Copy link

melvin-bot bot commented Aug 27, 2024

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

@melvin-bot melvin-bot bot changed the title Dupe detect -Description review appears when distance expenses are created without description [$250] Dupe detect -Description review appears when distance expenses are created without description Aug 27, 2024
@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 27, 2024
Copy link

melvin-bot bot commented Aug 27, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 27, 2024
@trjExpensify trjExpensify changed the title [$250] Dupe detect -Description review appears when distance expenses are created without description [$250] [Dupe detection] Description review appears when distance expenses are created without description Aug 28, 2024
@trjExpensify
Copy link
Contributor

Dupe detection is a #wave-control project. CC: @dylanexpensify @pecanoro @parasharrajat @kubabutkiewicz

@pecanoro pecanoro self-assigned this Aug 28, 2024
@mananjadhav
Copy link
Collaborator

Are we looking at regression or something @trjExpensify ? Should I go ahead with the proposal reviews?

@trjExpensify
Copy link
Contributor

Go ahead with the reviews, I was just bringing it to the attention of the dupe detection team.

@pecanoro
Copy link
Contributor

The cause and proposals seem to be the same as this one #47975 so we can go ahead with this one for now

@pecanoro
Copy link
Contributor

@mananjadhav Could you review the proposals when you have a moment?

@mananjadhav
Copy link
Collaborator

I think it makes sense to use getDescription. Hence @daledah's proposal looks good to me.

🎀 👀 🎀 C+ reviewed.

Copy link

melvin-bot bot commented Sep 2, 2024

Current assignee @pecanoro is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 2, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Sep 26, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

This issue has not been updated in over 15 days. @mananjadhav, @pecanoro, @adelekennedy, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@mananjadhav
Copy link
Collaborator

PR has been merged and deployed to staging

@pecanoro pecanoro added Weekly KSv2 and removed Monthly KSv2 labels Sep 26, 2024
@pecanoro
Copy link
Contributor

pecanoro commented Oct 7, 2024

PR has been on production for a week, not sure why the automatization did not work, so I am going to adjust manually

@pecanoro pecanoro added Daily KSv2 and removed Reviewing Has a PR in review labels Oct 7, 2024
@melvin-bot melvin-bot bot added the Overdue label Oct 7, 2024
@pecanoro pecanoro removed the Weekly KSv2 label Oct 7, 2024
@melvin-bot melvin-bot bot removed the Overdue label Oct 7, 2024
@pecanoro pecanoro added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Oct 7, 2024
@pecanoro pecanoro changed the title [$250] [Dupe detection] Description review appears when distance expenses are created without description [Awaiting payment] [$250] [Dupe detection] Description review appears when distance expenses are created without description Oct 7, 2024
@pecanoro pecanoro removed the Weekly KSv2 label Oct 7, 2024
@mananjadhav
Copy link
Collaborator

mananjadhav commented Oct 8, 2024

@adelekennedy this is ready for payout.

I don't have a specific offending PR for this one. We've been comparing raw comment hence the issue. But I feel we should have a regression test for this.

Regression test proposal:

  1. Go to workspace chat.
  2. Submit two same distance expenses without description.
  3. Go to transaction thread of any of the expense.
  4. Click Review duplicates.
  5. Click Keep this one (any).
  6. Verify that: Description review will not show up because both distance expenses are created without description.

@adelekennedy
Copy link

adelekennedy commented Oct 9, 2024

Payouts due:

Upwork job is here. @daledah please apply!

@daledah
Copy link
Contributor

daledah commented Oct 9, 2024

@adelekennedy Hi, can you create the offer and send to me at https://www.upwork.com/freelancers/~0138d999529f34d33f

@melvin-bot melvin-bot bot added the Overdue label Oct 11, 2024
@adelekennedy
Copy link

@daledah hired you for the job

@melvin-bot melvin-bot bot removed the Overdue label Oct 14, 2024
@daledah
Copy link
Contributor

daledah commented Oct 14, 2024

@adelekennedy Many thanks, accepted offer 👍

@adelekennedy
Copy link

payments made

@garrettmknight
Copy link
Contributor

$250 approved for @mananjadhav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

8 participants