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.57k stars 2.91k forks source link

[$250] Add posted date to Expensify Card transactions #52358

Open JmillsExpensify opened 1 week ago

JmillsExpensify commented 1 week ago

Problem: Credit card transactions are unique because it's possible to buy something one day, but then the transaction isn't processed by the issuing bank until a couple of days later. Both dates are important, because the receipt will always have the transaction date, yet the accountant (and accounting system) need the posted date.

Solution: Let's add the posted date to all Expensify Card transactions. What this means in practice is that:

I've annotated the mockup below so that this is hopefully all clear. 385205287-be8d697c-5bef-4333-b179-e69d8455f517

Other notes for emphasis:

Issue OwnerCurrent Issue Owner: @mountiny
melvin-bot[bot] commented 1 week ago

Current assignee @JmillsExpensify is eligible for the NewFeature assigner, not assigning anyone new.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

melvin-bot[bot] commented 1 week ago

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

mkzie2 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-12 09:01:16 UTC.

Proposal

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

Add posted date to Expensify Card transactions

What is the root cause of that problem?

New feature

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

  1. Create a variable for date description as we did for amountDescription here

    let dateDescription = translate('common.date')
  2. When the transaction is the card transaction, get the posted date from transaction data, if it exists add posted data to the dateDescription

if (postedDate) {
    dateDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.posted')} ${postedDate}`
}

https://github.com/Expensify/App/blob/e3632a9877542727230814ffc9fdd78bd087ecbd/src/components/ReportActionItem/MoneyRequestView.tsx#L235

  1. Update the description prop here to dateDescription variable
description={dateDescription} 

https://github.com/Expensify/App/blob/e3632a9877542727230814ffc9fdd78bd087ecbd/src/components/ReportActionItem/MoneyRequestView.tsx#L593

What alternative solutions did you explore? (Optional)

mkzie2 commented 1 week ago

Updated proposal.

ikevin127 commented 1 week ago

♻️ Reviewing proposal.

ikevin127 commented 1 week ago

@mkzie2's proposal makes sense to me. The solution follows the Amount field's model for adding additional data which is what we're looking for based on the issue's emphasis notes. Other details can be worked on during the PR phase. The solution result would look like this:

Spoiler Screenshot 2024-11-12 at 15 28 56

🎀👀🎀 C+ reviewed

@mountiny Is the BE part for this NewFeature completed ? I wasn't able to find any variable like / related to postedDate in the FE transaction types - therefore we might have to wait for BE to return the variable before moving on with the FE implementation for this issue.

melvin-bot[bot] commented 1 week ago

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

mountiny commented 1 week ago

Thanks! no the be is not completed. I fell for the trap from @JmillsExpensify called External issue.

I will have to whip up a PR for this

ikevin127 commented 1 week ago

Another thing I noticed when reviewing is that we have this check: https://github.com/Expensify/App/blob/7256ad6eef046be3345e0b260252f6ec47fc4203/src/libs/TransactionUtils/index.ts#L637-L642 based on which we will show the Posted YYYY-MM-DD label if postedDate exists (based on proposed solution): https://github.com/Expensify/App/blob/7256ad6eef046be3345e0b260252f6ec47fc4203/src/components/ReportActionItem/MoneyRequestView.tsx#L235-L242 but because of the !!transaction?.managedCard check which determines isCardTransaction, if managedCard (boolean) is false or doesn't exist in the transaction object -> isCardTransaction will be false -> label won't show.

@mountiny Is managedCard always populated and true on card transactions ? I wanted to bring up with you to ensure that we're tight here from the BE side, otherwise we could have a potential regression post-merge.

mkzie2 commented 1 week ago

@mountiny Please let me know once the backend PR is done and let me know what data in a transaction that I can use to get the postedDate

mountiny commented 1 week ago

@mkzie2 will let you know

mountiny commented 1 week ago

@ikevin127 I assume managedCard is for all card transactions give the logic you showed but I will check once I will work on the PR

melvin-bot[bot] commented 6 days ago

@JmillsExpensify, @mountiny, @ikevin127, @dubielzyk-expensify, @mkzie2 Huh... This is 4 days overdue. Who can take care of this?

ikevin127 commented 6 days ago

the be is not completed

mountiny commented 6 days ago

No update

JmillsExpensify commented 4 days ago

In the queue

mountiny commented 3 days ago

Made myself an owner as I was missing it my k2

mountiny commented 2 days ago

@ikevin127 @mkzie2 the BE is up and it should be deployed on Monday.

The property in transaction will be posted and its a string with format YYYYMMDD so we will have to format it to what we need. This is how it is saved in the DB so I prefer not to change it to avoid confusion and just process it in the App. Types need to be updated too obviously

mountiny commented 1 day ago

@mkzie2 @ikevin127 what is your eta on the pr? You can start working on it assuming the data is in posted field of a transaction. You cannot test card transactions nonetheless so you can go ahead

ikevin127 commented 1 day ago

Then @mkzie2 can move forward with opening the PR and use posted as the variable and I'll start reviewing as soon as it's ready for review.