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

[PAID] [$500] Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter #38355

Closed
Tracked by #27262
tgolen opened this issue Mar 14, 2024 · 37 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

@tgolen
Copy link
Contributor

tgolen commented Mar 14, 2024

This is coming from #27262. You can read the issue description there to get context behind the problem being solved and the mess being cleaned up.

Problem

ReportUtils.getIOUReportActionDisplayMessage() is called from a view component. It contains logic which calls const transaction = TransactionUtils.getTransaction(originalMessage.IOUTransactionID ?? ''); (a deprecated method).

Why this is important to fix

It maintains more pure and exact flow of data through the react application. If the view is using transaction data, then it needs to subscribe to the transaction in Onyx so that it's assured that the transaction object will never be stale or out-of-date.

Solution

  • Pass transaction as a parameter to getIOUReportActionDisplayMessage() and get the parameter from withOnyx() in the view component.
  • Remove the deprecated method completely
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cae110240999840b
  • Upwork Job ID: 1768370164615811072
  • Last Price Increase: 2024-03-14
  • Automatic offers:
    • hoangzinh | Reviewer | 0
    • FitseTLT | Contributor | 0
Issue OwnerCurrent Issue Owner: @strepanier03
@tgolen tgolen added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 14, 2024
@tgolen tgolen self-assigned this Mar 14, 2024
@melvin-bot melvin-bot bot changed the title Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter [$500] Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter Mar 14, 2024
Copy link

melvin-bot bot commented Mar 14, 2024

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

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

melvin-bot bot commented Mar 14, 2024

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

Copy link

melvin-bot bot commented Mar 14, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Mar 14, 2024

Proposal

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

Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter

What is the root cause of that problem?

Enhancement

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

Update getIOUReportActionDisplayMessage to accept transaction as a parameter instead of this

App/src/libs/ReportUtils.ts

Lines 4958 to 4959 in 02e8025

const transaction = TransactionUtils.getTransaction(originalMessage.IOUTransactionID ?? '');
const transactionDetails = getTransactionDetails(!isEmptyObject(transaction) ? transaction : null);

and pass transaction from withOnyx (using transactionID action.originalMessage.IOUTransactionID) for REportactionItem here

And for context menu action we will subscribe to transaction in here

},
reportActions: {
key: ({originalReportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`,
canEvict: false,
},

we will pass reportActions and reportActionID to the key and we will subscribe to transaction with transactionID we get from the report action's originalMessage.IOUTransactionID and pass the transaction to onPress in the payload when the context menu item is copy to clipboard ( textTranslateKey=='reportActionContextMenu.copyToClipboard') here
onPress={(event) => interceptAnonymousUser(() => contextAction.onPress?.(closePopup, {...payload, event}), contextAction.isAnonymousAction)}
description={contextAction.getDescription?.(selection) ?? ''}

and we will pass the transaction when we call getIOUReportActionDisplayMessage
const displayMessage = ReportUtils.getIOUReportActionDisplayMessage(reportAction);
Clipboard.setString(displayMessage);

What alternative solutions did you explore? (Optional)

for the context menu ,of course we can omit the transaction parameter and continue the current method of fetching transaction as we are currently doing because we don't have stale data worry for it as the function will be executed when the user presses copy to clipboard not when context menu component is first rendered 👍

@allgandalf
Copy link
Contributor

allgandalf commented Mar 14, 2024

Proposal

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

Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter

What is the root cause of that problem?

We want to update getIOUReportActionDisplayMessage as getTransaction is depricated

function getIOUReportActionDisplayMessage(reportAction: OnyxEntry<ReportAction>): string {

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

First we need to update getIOUReportActionDisplayMessage to accept one additional prop :

function getIOUReportActionDisplayMessage( transaction OnyxEntry<Transaction>): string {

Then remove the reportAction references as we get it from component:

App/src/libs/ReportUtils.ts

Lines 4958 to 4959 in aca0f8c

const transaction = TransactionUtils.getTransaction(originalMessage.IOUTransactionID ?? '');
const transactionDetails = getTransactionDetails(!isEmptyObject(transaction) ? transaction : null);

Finally we need to get transaction parameter with withOnyx in each component and pass it, currently we use getIOUReportActionDisplayMessage in :

iouMessage = ReportUtils.getIOUReportActionDisplayMessage(action);

const displayMessage = ReportUtils.getIOUReportActionDisplayMessage(reportAction);

What alternative solutions did you explore? (Optional)

N/A

@Krishna2323
Copy link
Contributor

Krishna2323 commented Mar 14, 2024

Proposal

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

Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter

What is the root cause of that problem?

Enhancement

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

We need to pass transaction from these two places, in ReportActionItemMessage we can use withOnyx & in ContextMenuActions we need to use Onyx.connect.
We will use reportAction.originalMessage.IOUTransactionID to get the data fro Onyx.

iouMessage = ReportUtils.getIOUReportActionDisplayMessage(action);

const displayMessage = ReportUtils.getIOUReportActionDisplayMessage(reportAction);

Here we need to accept transaction prop and remove the line.

App/src/libs/ReportUtils.ts

Lines 4925 to 4928 in 2737e44

/**
* Return iou report action display message
*/
function getIOUReportActionDisplayMessage(reportAction: OnyxEntry<ReportAction>): string {

const transaction = TransactionUtils.getTransaction(originalMessage.IOUTransactionID ?? '');

Result

Alternative

Instead of adding additional prop, we can utilize the transactions we have in ReportUtils, we have already connected to Onyx for transactions. We can use it like

allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${iouMessage?.IOUTransactionID}`] ?? ({} as Transaction)

App/src/libs/ReportUtils.ts

Lines 499 to 509 in 2737e44

let allTransactions: OnyxCollection<Transaction> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.TRANSACTION,
waitForCollectionCallback: true,
callback: (value) => {
if (!value) {
return;
}
allTransactions = Object.fromEntries(Object.entries(value).filter(([, transaction]) => transaction));
},
});

@allgandalf
Copy link
Contributor

Updated Proposal

Removed the reference of reportAction from the new function in my proposal

@Krishna2323
Copy link
Contributor

Proposal Update

Added minor detail.
We will use reportAction.originalMessage.IOUTransactionID to get the data fro Onyx.

@tgolen
Copy link
Contributor Author

tgolen commented Mar 14, 2024

@Krishna2323 Why do you suggest using Onyx.connect() in ContextMenuActions.tsx instead of withOnyx?

@FitseTLT
Copy link
Contributor

In context menu action the funtion is called when the user press copy to clipboard and the transaction we get with getTransaction is the uptodated one why do we even need connect don't get it we should make the transaction parameter optional and whenever we access it from components we pass it from withOnyx and for others like context menu action we just fetch the transaction as we are using it currently @tgolen

@tgolen
Copy link
Contributor Author

tgolen commented Mar 14, 2024

@FitseTLT I think it's better to connect the context menu to onyx to get the data it needs:

  1. This is how data is supposed to flow through a react application (from the top-down, passed via components), so it is following standard practices
  2. It presents a good pattern in our code for others to follow (engineers will ironically only choose bad patterns to follow. eg. ReportUtils.getPolicy() is the next one to clean up).
  3. Even if the transacation is technically up-to-date in that instance, it's still not promoting proper patterns, so I'd rather do it correctly

All of this results in higher-quality, easier to maintain, easier to debug code across the app.

@FitseTLT
Copy link
Contributor

Update based on #38355 (comment)

@Krishna2323
Copy link
Contributor

@tgolen, I think we can simply use the transactions from Onyx which we already have in ReportUtils to get the transaction.
We are already using it in two places like that in ReportUtils

const transaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${iouMessage?.IOUTransactionID}`] ?? ({} as Transaction);

const transaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`] ?? {};

I have updated my proposal

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 15, 2024

Proposal

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

Update getIOUReportActionDisplayMessage to accept the transaction as a parameter

What is the root cause of that problem?

We should pass transacation as parameter and get it via withOnyx to the component re-render when the transaction is changed

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

  1. In getIOUReportActionDisplayMessage function, add an extra param transaction
function getIOUReportActionDisplayMessage(reportAction: OnyxEntry<ReportAction>, transaction: OnyxEntry<Transaction>): string {

and then we can remove this line

const transaction = getTransaction(originalMessage.IOUTransactionID ?? '');

  1. Get transaction via withOnyx in ReportActionItemMessage
export default withOnyx<ReportActionItemMessageProps, ReportActionItemMessageOnyxProps>({
    transaction: {
        key: ({action}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${(action.originalMessage as IOUMessage).IOUTransactionID ?? '0'}`,
    },
})(ReportActionItemMessage);
  1. Add transaction in ContextMenuActionPayload type
transaction?: OnyxEntry<Transaction>;

type ContextMenuActionPayload = {

  1. Get transaction via withOnyx in BaseReportActionContextMenu
withOnyx<BaseReportActionContextMenuProps, BaseReportActionContextMenuTransactionOnyxProps>({
    transaction: {
        key: ({reportActions, reportActionID}) => {
            const reportAction = reportActions?.[reportActionID];
            return `${ONYXKEYS.COLLECTION.TRANSACTION}${(reportAction?.originalMessage as IOUMessage)?.IOUTransactionID}`;
        },
    },
})(

And then add transaction into payload here

const payload: ContextMenuActionPayload = {

  1. Update the test for getIOUReportActionDisplayMessage function by creating a random transaction and pass it into getIOUReportActionDisplayMessage function
const transaction = createRandomTransaction(1);

await waitForBatchedUpdates();
await measureFunction(() => ReportUtils.getIOUReportActionDisplayMessage(reportAction, transaction));

https://github.com/nkdengineer/App/blob/4e599b2cd6061b52d3964dc69e7575e210f0581d/tests/perf-test/ReportUtils.perf-test.ts#L202

Test branch: https://github.com/nkdengineer/App/tree/fix/38355

What alternative solutions did you explore? (Optional)

NA

@hoangzinh
Copy link
Contributor

@FitseTLT @Krishna2323 @GandalfGwaihir. Thanks for the proposals, everyone. There are a few things that don't look good to me in your proposals:

  • @FitseTLT your proposal overall looks good. But the way you pass transactions in BaseReportActionContextMenu doesn't look good to me. I prefer passing it in the context action payload as described in @nkdengineer here
  • @Krishna2323 your proposal hasn't satisfied the requirements where we should avoid using Onyx.connect here.
  • @GandalfGwaihir your proposal is not correct because ContextMenuActions is not a component, so we can't "we need to get transaction parameter with withOnyx in each component and pass it" as you mentioned

@hoangzinh
Copy link
Contributor

I'm inclined to @nkdengineer's proposal here #38355 (comment):

  • It satisfies the requirements in the description.
  • It's correct to me.
  • It's detailed

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Mar 15, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Mar 15, 2024

But the way you pass transactions in BaseReportActionContextMenu doesn't look good to me. I prefer passing it in the context action payload as described in @nkdengineer #38355 (comment)

Really don't get it @hoangzinh I think you misunderstood me I am proposing same as the selected proposal and I did propose to pass the transaction in the payload of onPress callback here is the a text from my proposal

pass the transaction to onPress in the payload when the context menu item is copy to clipboard ( textTranslateKey=='reportActionContextMenu.copyToClipboard') here

I really think you should give my proposal a second look. I have stated what should be done in detail same as the selected proposal and giving code snippet is not a must when a contributor provides what should be done in detail as I did
This what I said

we will pass reportActions and reportActionID to the key and we will subscribe to transaction with transactionID we get from the report action's originalMessage.IOUTransactionID and pass the transaction to onPress in the payload when the context menu item is copy to clipboard ( textTranslateKey=='reportActionContextMenu.copyToClipboard') here

cc: @tgolen

@hoangzinh
Copy link
Contributor

Hi @FitseTLT just wanna confirm that, in your proposal, you mentioned that you would pass transaction in this line

onPress={(event) => interceptAnonymousUser(() => contextAction.onPress?.(closePopup, {...payload, event}), contextAction.isAnonymousAction)}

And only pass transaction if it's a copyToClipboard, isn't it?

@FitseTLT
Copy link
Contributor

Hi @FitseTLT just wanna confirm that, in your proposal, you mentioned that you would pass transaction in this line

onPress={(event) => interceptAnonymousUser(() => contextAction.onPress?.(closePopup, {...payload, event}), contextAction.isAnonymousAction)}

And only pass transaction if it's a copyToClipboard, isn't it?

Yes. filteredContextMenuActions contains the different menu items in the context menu and we only need transaction parameter for onPress (payload) for copy to clipboard ( textTranslateKey=='reportActionContextMenu.copyToClipboard')

textTranslateKey: 'reportActionContextMenu.copyToClipboard',
icon: Expensicons.Copy,

That's why I said 👍

@tgolen
Copy link
Contributor Author

tgolen commented Mar 15, 2024

I discussed this with @hoangzinh and we agreed to go with the proposal from @FitseTLT.

It was the most complete and it was posted earlier than the proposal from nkdengineer.

@melvin-bot melvin-bot bot added the Overdue label Mar 21, 2024
@strepanier03
Copy link
Contributor

@davidcardoza - Because it seemed like the best fit based on this doc.

I originally thought VSB but it says anything related to IOU and bills are "elsewhere".

Then under VIP-bills it says "Anything related to bills and invoicing as part of an IOU request".

@davidcardoza
Copy link
Contributor

That makes sense, thanks for explaining.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot changed the title [$500] Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter [HOLD for payment 2024-04-15] [$500] Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter Apr 8, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Apr 8, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

This comment was marked as resolved.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Apr 14, 2024
@strepanier03
Copy link
Contributor

@hoangzinh - Please complete the checklist as soon as you can and then I'll handle your payment.

@FitseTLT - I've paid your contract and closed it in Upwork, thanks!

@melvin-bot melvin-bot bot removed the Overdue label Apr 16, 2024
@hoangzinh
Copy link
Contributor

Hi @strepanier03 we don't have a checklist for this issue because it's not a bug fix but a technical improvement.

@hoangzinh
Copy link
Contributor

oh @tgolen @FitseTLT we still have another follow-up task here #38882 (comment)

@tgolen
Copy link
Contributor Author

tgolen commented Apr 17, 2024 via email

@strepanier03
Copy link
Contributor

Thanks @hoangzinh for clarification 🙌.

@strepanier03 strepanier03 changed the title [HOLD for payment 2024-04-15] [$500] Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter [PAID] [$500] Update getIOUReportActionDisplayMessage() to accept the transaction as a parameter Apr 17, 2024
@strepanier03
Copy link
Contributor

Okay, this is all paid out and contracts are closed. Thank you both, @hoangzinh and @FitseTLT, for your work on this.

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
Status: Done
Development

No branches or pull requests

8 participants