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

[$250] Scan Receipt - No message when deleting a receipt from a scanned report #53998

Open
2 of 8 tasks
IuliiaHerets opened this issue Dec 12, 2024 · 12 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Dec 12, 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.75-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5334039
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Windows 11 / Chrome
App Component: Workspace Settings

Action Performed:

  1. Log in as the employee
  2. Navigate to a expense request conversation that has a receipt
  3. Click on the receipt thumbnail
  4. Verify there's a 3 dot menu on the top right
  5. Click on Delete Receipt
  6. Verify there's a modal to confirm the receipt deletion
  7. Confirm the action
  8. Verify the receipt is deleted from the request

Expected Result:

Verify a system message is posted in the conversation indicating the receipt was removed

Actual Result:

When deleting the receipt no message is displayed in the conversation.
It appears only after refreshing the page

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6691887_1733990857111.Nomessagewhendeletereciept.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021867401523775541525
  • Upwork Job ID: 1867401523775541525
  • Last Price Increase: 2024-12-13
Issue OwnerCurrent Issue Owner: @arosiclair
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 12, 2024
Copy link

melvin-bot bot commented Dec 12, 2024

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

@linhvovan29546
Copy link
Contributor

linhvovan29546 commented Dec 12, 2024

Edited by proposal-police: This proposal was edited at 2024-12-12 15:48:23 UTC.

Proposal

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

When deleting the receipt no message is displayed in the conversation.
It appears only after refreshing the page

What is the root cause of that problem?

In the Report screen, there is logic to filter report actions here. When the DetachReceipt API is called, it returns onyxData with an empty message format("html": ""). This causes the shouldReportActionBeVisible
function to mark the action as isDeletedAction (isDeleted=true), resulting in the message not being displayed.

const isDeleted = isDeletedAction(reportAction);
const isPending = !!reportAction.pendingAction;
return !isDeleted || isPending || isDeletedParentAction(reportAction) || isReversedTransaction(reportAction);

function isDeletedAction(reportAction: OnyxInputOrEntry<ReportAction | OptimisticIOUReportAction>): boolean {
const message = reportAction?.message ?? [];
if (!Array.isArray(message)) {
return message?.html === '' || !!message?.deleted;
}
// A legacy deleted comment has either an empty array or an object with html field with empty string as value
const isLegacyDeletedComment = message.length === 0 || message.at(0)?.html === '';
return isLegacyDeletedComment || !!message.at(0)?.deleted;
}

API Details Screenshot 2024-12-12 at 10 25 31 PM Screenshot 2024-12-12 at 10 51 48 PM

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

We can update the condition in the shouldReportActionBeVisible to check if reportAction.actionName=CONST.REPORT.ACTIONS.TYPE.MANAGER_DETACH_RECEIPT to the action is visible.

!isDeleted || isPending || isDeletedParentAction(reportAction) || isReversedTransaction(reportAction) || reportAction.actionName=CONST.REPORT.ACTIONS.TYPE.MANAGER_DETACH_RECEIPT

Alternatively, we can update here to minimize the risk of regression.

ReportActionsUtils.shouldReportActionBeVisible(reportAction, reportAction.reportActionID, canUserPerformWriteAction),

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

  • Fix the issue on the backend to return the correct message
  • Add message value with the expected data to successData

@MitchExpensify MitchExpensify moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Dec 13, 2024
@MitchExpensify MitchExpensify added the External Added to denote the issue can be worked on by a contributor label Dec 13, 2024
@melvin-bot melvin-bot bot changed the title Scan Receipt - No message when deleting a receipt from a scanned report [$250] Scan Receipt - No message when deleting a receipt from a scanned report Dec 13, 2024
Copy link

melvin-bot bot commented Dec 13, 2024

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

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

melvin-bot bot commented Dec 13, 2024

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

@rushatgabhane
Copy link
Member

seems like a backend issue to me 🎀👀🎀

but it looks like we can do it on frontend too? #53998 (comment)

Copy link

melvin-bot bot commented Dec 13, 2024

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

@daledah
Copy link
Contributor

daledah commented Dec 15, 2024

Proposal

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

  • When deleting the receipt no message is displayed in the conversation.
    It appears only after refreshing the page

What is the root cause of that problem?

  • This issue appears because of the BE issues. In API Detach_Receipt, BE already returns the message type MANAGERDETACHRECEIPT, which will display removed a receipt. This message has message.at(0).html === '', so it is not displayed in the report action list because of it is considered as deleted message because of:

    return message?.html === '' || !!message?.deleted;

  • Even if we fix the above issue from the BE side, the issue is still encountered in offline mode. When we update other fields, such as amount, merchant, description, ..., the optimistic updated actions are always added via:

App/src/libs/actions/IOU.ts

Lines 2736 to 2742 in ff2fc94

// Step 3: Build the modified expense report actions
// We don't create a modified report action if:
// - we're updating the waypoints
// - we're updating the distance rate while the waypoints are still pending
// In these cases, there isn't a valid optimistic mileage data we can use,
// and the report action is created on the server with the distance-related response from the MapBox API
const updatedReportAction = ReportUtils.buildOptimisticModifiedExpenseReportAction(transactionThread, transaction, transactionChanges, isFromExpenseReport, policy, updatedTransaction);

but we didn't do it when detaching the receipt.

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

  • First, we need to fix the issue from BE side to make sure the report action returned in Detach_Receipt API is correct.

  • Then, we need to handle the offline mode. To do it, create a function to build an optimistic update report action (which will display "removed a receipt") when detaching the receipt, it is the same as:

App/src/libs/ReportUtils.ts

Lines 5263 to 5266 in ff2fc94

/**
* Builds an optimistic modified expense action with a randomly generated reportActionID.
*/
function buildOptimisticModifiedExpenseReportAction(

function buildOptimisticDetachReceipt(reportID: string) {
    return {
        actionName: CONST.REPORT.ACTIONS.TYPE.MANAGER_DETACH_RECEIPT,
        actorAccountID: currentUserAccountID,
        automatic: false,
        avatar: getCurrentUserAvatar(),
        created: DateUtils.getDBTime(),
        isAttachmentOnly: false,
        message: [
            {
                // Currently we are composing the message from the originalMessage and message is only used in OldDot and not in the App
                text: 'You',
                style: 'strong',
                type: CONST.REPORT.MESSAGE.TYPE.TEXT,
            },
            {
                text: ' detached a receipt from expense.',
                style: 'strong',
                type: CONST.REPORT.MESSAGE.TYPE.TEXT,
            },
        ],
        person: [
            {
                style: 'strong',
                text: currentUserPersonalDetails?.displayName ?? String(currentUserAccountID),
                type: 'TEXT',
            },
        ],
        pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
        reportActionID: NumberUtils.rand64(),
        reportID,
        shouldShow: true,
    };
}
  • The last step is to update the function by adding the below logic:
 const expenseReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transaction?.reportID}`] ?? null;
    const updatedReportAction = ReportUtils.buildOptimisticDetachReceipt(expenseReport?.reportID ?? '-1');

    optimisticData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${updatedReportAction?.reportID}`,
        value: {
            [updatedReportAction.reportActionID]: updatedReportAction as OnyxTypes.ReportAction,
        },
    });
    optimisticData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${updatedReportAction?.reportID}`,
        value: {
            lastVisibleActionCreated: updatedReportAction.created,
            lastReadTime: updatedReportAction.created,
        },
    });
    failureData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${updatedReportAction?.reportID}`,
        value: {
            lastVisibleActionCreated: expenseReport?.lastVisibleActionCreated,
            lastReadTime: expenseReport?.lastReadTime,
        },
    });
    successData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReport?.reportID}`,
        value: {
            [updatedReportAction.reportActionID]: {pendingAction: null},
        },
    });
    failureData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReport?.reportID}`,
        value: {
            [updatedReportAction.reportActionID]: {
                ...(updatedReportAction as OnyxTypes.ReportAction),
                errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericEditFailureMessage'),
            },
        },
    });

    const parameters: DetachReceiptParams = {transactionID, reportActionsID: updatedReportAction.reportActionID};

function detachReceipt(transactionID: string) {

  • Note: In the example above, I introduced a new parameter to the API: reportActionsID: updatedReportAction.reportActionID. The backend will use this ID as the report action ID instead of generating a new one.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Dec 15, 2024
Copy link

melvin-bot bot commented Dec 16, 2024

@arosiclair, @rushatgabhane, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@arosiclair
Copy link
Contributor

arosiclair commented Dec 16, 2024

seems like a backend issue to me 🎀👀🎀

Agreed I'll fix this internally. I'm not sure if we want offline support for this specifically, but I'll look into it.

@arosiclair arosiclair added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue labels Dec 16, 2024
@arosiclair arosiclair added Weekly KSv2 and removed Daily KSv2 labels Dec 16, 2024
@melvin-bot melvin-bot bot removed the Overdue label Dec 16, 2024
@daledah
Copy link
Contributor

daledah commented Dec 16, 2024

@arosiclair Do you think we need to handle offline case like we did when updating other transaction fields like amount, description, etc? Currently, when deleting a receipt offline, there is no "removed a receipt" message until we back online

@melvin-bot melvin-bot bot added the Overdue label Dec 25, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

@arosiclair @rushatgabhane @MitchExpensify 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!

@arosiclair
Copy link
Contributor

I posted a PR to fix the hidden message issue. We can add offline support next, but that will require more changes which I haven't written yet.

@daledah you can start drafting changes in the meantime. The optimistic originalMessage and message for the action should be formatted like this:

{
  "originalMessage": {
    "transactionID": 123,
    "merchant": "${merchant}"
  },
  "message": [
    {
      "type": "COMMENT",
      "html": "detached a receipt from expense '${merchant}'",
      "text": "detached a receipt from expense '${merchant}'",
      "whisperedTo": []
    }
  ]
}

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. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

6 participants