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.78k forks source link

[$500] The updated amount is not copied to clipboard. #26664

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!


Action Performed:

  1. Request money
  2. Open IOU and change amount.
  3. Click on Copy to clipboard on IOU item.
  4. Paste to chat.

    Expected Result:

    The updated amount should appear.

    Actual Result:

    The updated amount does not appear.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.62-4 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/52d27192-1b42-47d6-a76a-06ea16f67a69

https://github.com/Expensify/App/assets/43996225/dd023c59-0db7-4089-93b9-42c6cd51d213

Expensify/Expensify Issue URL: Issue reported by: @oleksandr-pantsyr Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692986644530329

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012d2ba9ac8db0e8b1
  • Upwork Job ID: 1699071512490926080
  • Last Price Increase: 2023-09-05
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

paultsimura commented 1 year ago

Proposal

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

The modified request amount is not copied to the clipboard.

What is the root cause of that problem?

On updating the money request, we only update the transaction and create a new updatedReportAction in the thread https://github.com/Expensify/App/blob/4304851b42882c3e221ebb5d00996ef39cfe704c/src/libs/actions/IOU.js#L982

image

The message on the original reportAction is not updated. While the copyToClipboard action copies the action's message here: https://github.com/Expensify/App/blob/94c20d7af722f9c5c720b4ca1ba0620b90ddfe9d/src/pages/home/report/ContextMenu/ContextMenuActions.js#L190

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

  1. Pass parentReportAction as another parameter to IOU.editMoneyRequest here: https://github.com/Expensify/App/blob/931524f4f64bd67c46ca0abb2bc93598b79fac34/src/pages/EditRequestPage.js#L95-L98
  2. In the IOU.editMoneyRequest, along with 2 existing merge requests, add the following:
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReport.reportID}`,
            value: {
                [parentReportAction.reportActionID]: {
                    message: ReportUtils.getIOUReportActionMessage(
                        parentReportAction.originalMessage.IOUReportID,
                        parentReportAction.originalMessage.type,
                        updatedTransaction.modifiedAmount,
                        updatedTransaction.comment.comment,
                        updatedTransaction.modifiedCurrency,
                    ),
                },
            },
        },

This will update the message of the original reportAction.

Note: This apparently requires updating the EditMoneyRequest API handler on BE as well, because the optimistically stored message gets eventually reset to the original one.

What alternative solutions did you explore? (Optional)

alphaboss1104 commented 1 year ago

This is BE issue. Because when we send the editMoneyRequst request to BE, it doesn't return any value to set the MoneyRequestPreview 's reportAction. image

As I see @paultsimura 's opinion for root cause is correct but his solution isn't enough. In his solution he merged only message but we should merge all necessary details.

paultsimura commented 1 year ago

My proposal fixes the clipboard issue, right what is described in the ticket.

However, I'd like to understand what is the "all necessary details" - from my understanding, the details like currency, description, amount are stored in the originalMessage, which should not be modified. And all of that data can (and should) be fetched from the transaction.

As I see @paultsimura 's opinion for root cause is correct but his solution isn't enough.

In his solution he merged only message but we should merge all necessary details.

zanyrenney commented 1 year ago

Yep, this is reproducible, adding the external label.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @zanyrenney is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

parasharrajat commented 1 year ago

Being discussed on Slack. https://expensify.slack.com/archives/C01GTK53T8Q/p1693927193607219

parasharrajat commented 1 year ago

So it seems that it should have been fixed in https://github.com/Expensify/App/pull/24630.

oleksandr-pantsyr commented 1 year ago

Hi @parasharrajat thank you for investigating. But my issue is different from your issue. your issue is that the IOU comment is not copied. But my issue even if IOU comment is copied, it does not reflect the updated amount. I don't think my issue will be fixed even if #24630 is fixed.

parasharrajat commented 1 year ago

OK, I see that it is different from https://github.com/Expensify/App/issues/24569.

Both proposals are wrong. you are looking at the old reportaction but we are forgetting that it is an action that won't change. Instead, we add more modified actions for details being updated.

So if we want to copy the exact message as previewed, then the message needs to be formatted from data instead of using static values from actions.

@mountiny because you worked on it earlier. What do you think about this issue?

mountiny commented 1 year ago

@parasharrajat thats correct the MoneyRequestAction component is basically just a view for the Transaction which is assocaited with ti and the link is in the message.IOUTransactionID field, this way you can pull the transaction object and get the data you want

BhuvaneshPatil commented 1 year ago

Proposal

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

The updated amount is not copied to clipboard

What is the root cause of that problem?

We are getting the copied text from html property but if we debug that, html property shows the previous value

Screenshot 2023-09-07 at 9 55 17 PM

That's why we are not getting updated value.

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

To get the value of text correctly we should notice how it's being done in MoneyRequestPreview. We use getTransactionDetails() method for getting required fields.

We can add new check for checking if the action is IOU or not (which is in case of money request preview) And handle that separately.

More concisely -

There is another caveat to this, handling in case of description is present.

We need confirmation from expensify how to handle that, current behaviour is requested ₹343.00 for abdasd

function will look something like this (needs some polishing)

function getTransactionMessage(reportAction) {
    const originalMessage = _.get(reportAction, 'originalMessage', {});
    // get transactionId
    const transactionId = _.get(originalMessage, 'IOUTransactionID', 0);
    const transaction = getTransaction(transactionId);
    const reportID = _.get(transaction, 'reportID');
    const report = lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {}) || {};
    const details = {
        created: getCreated(transaction),
        amount: getAmount(transaction, lodashGet(report, 'type') === CONST.REPORT.TYPE.EXPENSE),
        currency: getCurrency(transaction),
        comment: getDescription(transaction),
        merchant: getMerchant(transaction),
    };

    return `requested ${CurrencyUtils.convertToDisplayString(details.amount, details.currency)}${details.comment && ` for ${details.comment}`}`;
}
else if (isIOUAction && _.get(originalMessage, 'type', '') === CONST.IOU.REPORT_ACTION_TYPE.CREATE) {
                    const str = TransactionUtils.getTransactionMessage(reportAction);
                    Clipboard.setString(str);
                }

result -

https://github.com/Expensify/App/assets/27822551/2ca39082-f75e-4bdc-b51d-c8cfb3c5c0d8

What alternative solutions did you explore? (Optional)

parasharrajat commented 1 year ago

@BhuvaneshPatil Thanks for the proposal.

There are a few types of IOU in the App.

 MONEY_REQUEST_TYPE: {
            SEND: 'send',
            SPLIT: 'split',
            REQUEST: 'request',
        },

Now, the function needs to give a properly formatted string. Can you please lay out the logic of the function on how will you create the message?

Hint: Try creating all types of requests and observe them, modify your function to fit in. Then edit these requests and observe again.

You will be able to get the messages used from original message strings for all types. Normally, the same is used in LHN as the last message text.

BhuvaneshPatil commented 1 year ago

sure @parasharrajat

We can add another check in the context menu action. ex -

if(isIOU && !isSplitBill){
//
}
paultsimura commented 1 year ago

Proposal

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

The modified request amount is not copied to the clipboard.

What is the root cause of that problem?

On copy-to-clipboard action, we copy only the static reportAction.message instead of building it dynamically from transaction.

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

https://github.com/Expensify/App/blob/94c20d7af722f9c5c720b4ca1ba0620b90ddfe9d/src/pages/home/report/ContextMenu/ContextMenuActions.js#L185

To the chain of if/else, add the following logic:

} else if (ReportActionsUtils.isMoneyRequestAction(reportAction)) {
    const transaction = getTransaction(originalMessage.IOUTransactionID);
    const {
        amount,
        currency,
        comment
    } = ReportUtils.getTransactionDetails(transaction);

    const iouMessage = ReportUtils.getIOUReportActionMessage(
        transaction.reportID,
        originalMessage.type,
        amount,
        comment,
        currency,
   );

    const iouMessageHtml = lodashGet(_.last(iouMessage), 'html', '');
    Clipboard.setString(iouMessageHtml);
}

This ReportUtils.getIOUReportActionMessage method is used for building the initial message, so using it here should do the thing. Also, my solution is versatile and supports any type of IOU report action along with the request description updates (which is currently not copied to clipboard either).

The only possible improvement I see is to move this to a separate function inside the ReportUtils. Additionally, I would recommend modifying the ReportUtils.getIOUReportActionMessage function in order to support localization instead of using hardcoded strings.

Results of the same request being edited:

image image

What alternative solutions did you explore? (Optional)

parasharrajat commented 1 year ago

Thanks, I am discussing this issue as a possible regression.

BhuvaneshPatil commented 1 year ago

Updated proposal

melvin-bot[bot] commented 1 year ago

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

pecanoro commented 1 year ago

I am going to close this issue because it's basically the same root cause as https://github.com/Expensify/App/issues/26571 and this was created after.

BhuvaneshPatil commented 1 year ago

@pecanoro This issue is about money request and the other one is about task and the solution is totally different

BhuvaneshPatil commented 1 year ago

The issue #26571 is possible dupe of https://github.com/Expensify/App/issues/26753

Please confirm at your end

pecanoro commented 1 year ago

@BhuvaneshPatil It's a regression for the same PR, so we need to solve it in a single PR.