[HOLD for payment 2024-03-14] [$500] [MEDIUM] Expense Report - In an offline created expense report, a negative request amount is displayed in the Header and Title

lanitochka17 commented 8 months 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!

Version Number: 1.4.39-0 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/35913

Action Performed:

Preconditions: Set up an OldDot administrator account, invite the employee to the policy https://sites.google.com/applausemail.com/applause-expensifyproject/wiki-guides/newdot-categories?authuser=0

Open https://staging.new.expensify.com/
Log in with the account of the employee added to the policy
Navigate to the group policy chat room
Turn off the internet
Create a manual request and send it to the WS room
Go to Report Conversation

Expected Result:

IOU created offline should not display a negative amount in the header and title

Actual Result:

In an offline created IOU, a negative request amount is displayed in the Header and Title

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/2ae68ee6-1f65-44c1-9ead-fb20579a7f9b

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013de0165fd934bba3
Upwork Job ID: 1755994535213568000
Last Price Increase: 2024-02-09
Automatic offers:

paultsimura | Contributor | 0

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

lanitochka17 commented 8 months ago

We think that this bug might be related to #vip-bills

paultsimura commented 8 months ago

Proposal

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

Optimistically created Expense report contains a negative value in the title.

What is the root cause of that problem?

When setting the optimistic reportName of the newly created Expense report, we use the incorrect value for the formatted total:

https://github.com/Expensify/App/blob/ad3d78c1ce86c99b3b3e0fb65300a54705e22c77/src/libs/ReportUtils.ts#L2820-L2824

The storedTotal is a negated numeric value that should be stored in the DB: we store the Expense reports' total as a negative value. However, this shouldn't affect the user-visible content – report title.

Note: this happens only with the enabled report fields beta. With it, we show the report field value in the title. Otherwise, we calculate the title dynamically and negate this amount correctly.

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

When coming online, the RequestMoney API response changes this report title to the Expense Report #{reportID}:

We should change the optimistic expense report creation to set the reportName in a similar way so that the optimistic data is aligned with the future Server response:

reportName: `Expense Report ${reportID}`,

https://github.com/Expensify/App/blob/ad3d78c1ce86c99b3b3e0fb65300a54705e22c77/src/libs/ReportUtils.ts#L2841

What alternative solutions did you explore? (Optional)

If we don't want to change the report title format, but keep the "${policy} owes $amount" format for the optimistic expense reports, we should simply use the total instead of storedTotal when building the formattedTotal, which is used only in the report title:

const formattedTotal = CurrencyUtils.convertToDisplayString(total, currency);

mallenexpensify commented 8 months ago

Checking to see if this fits with #wave6-collect-submitters cuz of "Create a manual request and send it to the WS room"

mananjadhav commented 7 months ago

@mallenexpensify Should this be further reviewed?

mallenexpensify commented 7 months ago

Yes please @mananjadhav , thanks for asking. I've added this to the wave 6 project.

mananjadhav commented 7 months ago

@mallenexpensify @nkuoch The proposal by @paultsimura looks promising but I want to confirm if we want to update the report title to match what comes from the backend or just fix the total?

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 7 months ago

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

nkuoch commented 7 months ago

Hmm I'm not 100% sure, so checking with @marcaaron ?

marcaaron commented 7 months ago

Haven't worked much on this feature, but happy to lend a hand...

this happens only with the enabled report fields beta

@paultsimura are you saying that if we are not on the beta the bug does not exist? I didn't quite follow how this relates to "report fields".

Otherwise, we calculate the title dynamically and negate this amount correctly.

Where does this happen? In the backend? And you are suggesting we need to also do it in the frontend?

The storedTotal is a negated numeric value that should be stored in the DB: we store the Expense reports' total as a negative value. However, this shouldn't affect the user-visible content – report title.

This explanation generally makes sense to me. Gonna pass this to @thienlnam though since I believe they have worked on the report fields stuff.

marcaaron commented 7 months ago

I want to confirm if we want to update the report title to match what comes from the backend or just fix the total?

@mananjadhav I did not understand this question. What does "fix the total" mean to you?

thienlnam commented 7 months ago

Is this still happening? We merged a PR recently that disables the report title for anything other than expense / paid collect policies. So this should no longer show up for IOU reports

paultsimura commented 7 months ago

Let me try to answer to all of the above 🙂

@thienlnam – it's still happening. I'm sure the QA team just mixed the Expense/IOU terminology a bit. Here's the recording from the latest main, it's exactly Expense-related bug:

https://github.com/Expensify/App/assets/12595293/b2f4010b-a8cd-480d-aedd-7eaf21291fe9

paultsimura commented 7 months ago

are you saying that if we are not on the beta the bug does not exist? I didn't quite follow how this relates to "report fields".

Yes, without beta it doesn't happen because, without this beta, we use the report title that's calculated on the fly (on the client). For the Expense report, the title is calculated based on the report's total. However, when the Beta is enabled, the reportFields.title takes priority and is shown instead of being calculated on the fly – this makes sense as the report title can be changed to a custom one, and we will want to show it instead of trying to re-calculate it. Moreover, as I explained later on, with the new approach, we no longer use the "{policy} owes {amount}" Expense report title, but instead the "Expense Report #{reportID}". So IMO, the issue here is just that we've updated the BE part to set the Expense report's name to "Expense Report #{reportID}", but did not update the client to generate the same report name optimistically. And that's what I suggest we should fix as the optimistic data should match the BE response.

cc: @marcaaron

thienlnam commented 7 months ago

Ah I see, thanks for the clarification.

cc @trjExpensify I think we've talked about this, but should the Expense report title be overwritten on an expense report with report fields like this?

trjExpensify commented 7 months ago

Sorry, not sure I'm following the question?

thienlnam commented 7 months ago

When you request money from a workspace, it looks the report title takes on custom values based on what is happening inside the report - In the example where it shows (BNewDot Test Workspace - Expensify US paid $3.80)

However, when you add custom report fields on an expense report, it overwrites that title to something like "Expense Report #{reportID}".

Should that happen with these report titles, or should we preserve the custom title from before?

trjExpensify commented 7 months ago

Okay cool, so I think this confused me:

However, when you add custom report fields on an expense report, it overwrites that title to something like "Expense Report #{reportID}".

It's a "custom report title". As in, if I added a "custom report field" for a list of customers to select on the report, that wouldn't assume the title. But If I set a custom report title (e.g Expense Report #{reportID}) for reports on my workspace, it would.

thienlnam commented 7 months ago

Yeah, by default the report is created with Expense Report #{reportID}.

But it sounds like, unless it's a required title field / OR the user manually updates the title, then the report should keep the title in the front-end of showing "US - owes $3.60"?

trjExpensify commented 7 months ago

Hm, so an admin looking at their workspace settings see's a configuration option for the "report title", but the report shows with a different one in the header? That seems a bit confusing.

I think if we've committed to bringing in report titles for expense reports, we need to use the report title. So whether that be the default set on workspace creation, a customised one by the admin in the workspace settings, or a modified one (if enforcement is disabled) by the member.

If we want to change the default for expenseReports to something that's not today's default {report:type} {report:startdate} because it feels better, we should look at that. Though I believe it was modified to this formula when we released invoice reports, because it's one default title formula on a workspace that applies to both report types.

Sidebar, for iouReports "off-workspace" they should still retain what exists today in NewDot (i.e %payer% [owes || paid] %amount%) as report titles aren't a feature for P2P.

CC: @JmillsExpensify

JmillsExpensify commented 7 months ago

Here's my two cents on this one:

If a custom title exists, we should show it. We're going to bring this over to NewDot in the workspace editor as well
That said, I actually think we should only "honor" the custom title on Collect and Control (I'm fairly certain that the formula also exists for Free workspaces, since the default formula comes when the workspace is created, right?)
On the report title, I do have concerns how this'll end up looking, because so much of our early designs have optimized for the default title, but I think this is also what it is. So let's just keep a pulse on what our customers are experiencing
- Then in terms of changing the default title, I could see an argument that the default title for "Simplified Collect" policies should mimic the existing default title we've built. Not incredibly passionate, though I think that's a reasonable approach and also keeps the default experience looking just like we've intended it to be built.

trjExpensify commented 7 months ago

That said, I actually think we should only "honor" the custom title on Collect and Control (I'm fairly certain that the formula also exists for Free workspaces, since the default formula comes when the workspace is created, right?)

Yeah, I didn't consider free because we're deprecating the policy type soon. Given that, I would just apply whatever we want to do universally on expenseReports without adding an "isFree" condition. It's not worth it at this point, IMO.

Then in terms of changing the default title, I could see an argument that the default title for "Simplified Collect" policies should mimic the existing default title we've built. Not incredibly passionate, though I think that's a reasonable approach and also keeps the default experience looking just like we've intended it to be built.

I don't quite know what that means? Newly created collect workspaces from now have a new default report title formula set or something? If not, then we're in the same spot as I mentioned above where you can see a "report title" to configure with a default formula set on creation that's different from the report header.

JmillsExpensify commented 7 months ago

I think we're still several months away from deprecating Free right? Like that doesn't happen until this summer at the very earliest based on the timeframe for wave8 release4.

As for the other part, yes I'm referring to Collect workspaces that are created from now on. Let's just see those policies with what we think is the optimal report title for NewDot. We're going to move sign-ups to NewDot very soon, and at that point, that also means that we're going to put them on a simplified Collect policy very soon as well.

trjExpensify commented 7 months ago

I think we're still several months away from deprecating Free right? Like that doesn't happen until this summer at the very earliest based on the timeframe for wave8 release4.

Whaaat? We're deprecating free with release 1.

trjExpensify commented 7 months ago

As for the other part, yes I'm referring to Collect workspaces that are created from now on. Let's just see those policies with what we think is the optimal report title for NewDot.

Okay, but recognising we need to separate the report title logic out for the report types on the workspace. As invoice uses the default report title too.

Let's just see those policies with what we think is the optimal report title for NewDot.

On this specifically though, any suggestions for the formula?

{report:policyname} {report:status} {report:total} gets us kinda' close but we'd end up with this in reality:

Duraflame open $10.00
Duraflame processing $10.00
Duraflame approved $10.00
Duraflame reimbursed $10.00

.. unless we do some mapping to our new front-end references for these states in NewDot maybe:

Duraflame draft $10.00
Duraflame owes $10.00
Duraflame approved $10.00
Duraflame paid $10.00

"Draft" is the one it falls down on though in that structure, unless we faux it which doesn't seem right. Overall, I'm open to dropping this for expenseReports and even sticking with {report:type} {report:startdate} for now. After all, what we have in NewDot was driven by the iouReport and reverse polarity design.

Equally, if we do think today's workspace default report title doesn't work and we're open to separating out the invoice report type/title logic, then something like this might work for an updated expenseReport default:

[{report:status}] {report:submit:from:firstname}'s expenses {report:startdate}

[Draft] Tom's expenses 2024-02-15
[Owed] Tom's expenses 2024-02-15
[Approved] Tom's expenses 2024-02-15
[Paid] Tom's expenses 2024-02-15

JmillsExpensify commented 7 months ago

I think we're still several months away from deprecating Free right? Like that doesn't happen until this summer at the very earliest based on the timeframe for wave8 release4.

Whaaat? We're deprecating free with release 1.

Say what? How can we deprecate Free until we minimally have More features built out. That's not release 1. Otherwise it's impossible to edit a distance rate.

trjExpensify commented 7 months ago

I don't think we need to wait for that (especially if it ends up being something you can't turn off haha).

But that aside, we have a couple of options:

OldDot workspace editor (via supportal when they ask, because we're going to lock it down)
Or we could literally keep this card on a page called Distance until we replace it with rate setting etc.

I'd prefer those than delaying being rid of free for release 2 where distance is slated.

JmillsExpensify commented 7 months ago

Let's take this offline. I don't think we need to rush this really, and equally I'd love to rip all of the Free plan settings out in one go, rather than have some pages old and some pages new. How long is the release 2 delay going to be? I would guess a matter of weeks.

trjExpensify commented 7 months ago

Probably more than that looking at the scope, but yeah cool, we can take that part offline. Back to fixing this issue though..

I think @paultsimura is correct with this:

So IMO, the issue here is just that we've updated the BE part to set the Expense report's name to "Expense Report #{reportID}", but did not update the client to generate the same report name optimistically. And that's what I suggest we should fix as the optimistic data should match the BE response.

Note: Though one thing, when I create a new group workspace the default report title formula is {report:type} {report:startdate}, so I don't actually know where we got {report:type} #{reportID} from here? The two should be the same. Why aren't they, @thienlnam?

If we don't like that/want to pivot to some other formula, then we can follow-up adjacent to this issue that's fixing the bug of what has already been implemented. As far as I can tell from the report fields doc, that was the intention for paid plans in NewDot as it stands:

For reports created on paid plan policies, we’ll show in NewDot the default report title configured in the OldDot policy editor which should happen automatically and no special logic would need to be considered here.

thienlnam commented 7 months ago

I think that was just an example, the default should still be the same so it probably is {report:type} {report:startdate}

So it sounds like in this issue, we'll go forward with the assumption that if you create a collect policy, the default report title will stick and so it should show as {report:type} {report:startdate} or whatever is set there

JmillsExpensify commented 7 months ago

Yeah, I think we're cool with that for the time being. Down the line I do think we need to update titles to match NewDot front-end report states (draft, owed, approved, paid), but that's polish for another day.

trjExpensify commented 7 months ago

So it sounds like in this issue, we'll go forward with the assumption that if you create a collect policy, the default report title will stick and so it should show as {report:type} {report:startdate} or whatever is set there

Yep, make sense. Let's just make sure it is actually using whatever is set there, including the correct default on workspace creation.

paultsimura commented 7 months ago

I think that was just an example

No, that's the actual data that comes from the BE when I create the Expense in that WS:

However, once I enable the "Enforce Default Report Title" in OD, the report name comes as {report:type} {report:startdate} indeed.

paultsimura commented 7 months ago

Proposal

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

Optimistically created Expense report contains a negative value in the title.

What is the root cause of that problem?

When setting the optimistic reportName of the newly created Expense report, we use the incorrect value for the formatted total:

https://github.com/Expensify/App/blob/ad3d78c1ce86c99b3b3e0fb65300a54705e22c77/src/libs/ReportUtils.ts#L2820-L2824

The storedTotal is a negated numeric value that should be stored in the DB: we store the Expense reports' total as a negative value. However, this shouldn't affect the user-visible content – report title.

Note: this happens only with the enabled report fields beta. With it, we show the report field value in the title. Otherwise, we calculate the title dynamically and negate this amount correctly.

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

When coming online, the RequestMoney API response changes this report title to the Expense Report #{reportID} if the "Default Report Title" is not enforced, and the actual formula value (e.g. {report:type} {report:startdate}) if it's enforced:

We should change the optimistic Expense report creation to set the reportName based on this report field formula for the policies where Report Fields are enabled:

reportName: `Expense Report ${reportID}`,

https://github.com/Expensify/App/blob/ad3d78c1ce86c99b3b3e0fb65300a54705e22c77/src/libs/ReportUtils.ts#L2841

What alternative solutions did you explore? (Optional)

If we don't want to change the report title format, but keep the "${policy} owes $amount" format for the optimistic expense reports, we should simply use the total instead of storedTotal when building the formattedTotal, which is used only in the report title:

    const expenseReport: OptimisticExpenseReport = {
        reportID: generateReportID(),
        chatReportID,
        policyID,
        type: CONST.REPORT.TYPE.EXPENSE,
        ownerAccountID: payeeAccountID,
        currency,
        // We don't translate reportName because the server response is always in English
        reportName: `${policyName} owes ${formattedTotal}`,
        stateNum,
        statusNum,
        total: storedTotal,
        notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
        parentReportID: chatReportID,
        lastVisibleActionCreated: DateUtils.getDBTime(),
    };

    const reportFields = getReportFieldsByPolicyID(policyID);
    const titleReportField = getTitleReportField(reportFields ?? {});
    if (!!titleReportField && reportFieldsEnabled(expenseReport)) {
        const isDefaultValueEnforced = !titleReportField.deletable;
        if (isDefaultValueEnforced) {
            expenseReport.reportName = titleReportField.defaultValue; // Need to replace the values in the formula with the real report's data here as well
        } else {
            expenseReport.reportName = `Expense Report #${expenseReport.reportID}`;
        }
    }

https://github.com/Expensify/App/blob/e3331db018e826dc20bb34ad33b3fb77ee6e8fb8/src/libs/ReportUtils.ts#L2907-L2923

paultsimura commented 7 months ago

@thienlnam I have reposted my proposal with the flow from the discussion. Could you please check if I understood the expectations here correctly?

trjExpensify commented 7 months ago

When coming online, the RequestMoney API response changes this report title to the Expense Report #{reportID} if the "Default Report Title" is not enforced, and the actual formula value (e.g. {report:type} {report:startdate}) if it's enforced:

Ah, so something is wonky there then right? Those formulas should be the same with or without enforcement. Where is Expense Report #{reportID} coming from? :/

thienlnam commented 7 months ago

Ah crap, thinking about it more we have another problem with offline optimistic handling.

When a policy has a title report field, we want to always use the value for the reportName. This is whatever is set in the template ex. {report:type} {report:startdate} or any of the other template values added from here

However - we can't always generate the template report name optimistically without data from the BE. An example would be a template like {report:oldID} {report:reimbursementid} {field:Employee ID}.

Instead of building a system to format template values in the FE, we should just always use the value returned from the BE for it. But that would mean if we generated it optimistically, we would want a way to indicate the user that "Report name will be added when you're back online"

Ah, so something is wonky there then right? Those formulas should be the same with or without enforcement. Where is Expense Report #{reportID} coming from? :/

Yeah this is interesting, I wonder if the request money flow is updating it to this

paultsimura commented 7 months ago

@thienlnam got you. To sum it up, what would we expect as an optimistic name in the outcome of this GH, if we don't use formulas? We kinda need to set the report title anyway 🙂

thienlnam commented 7 months ago

cc @trjExpensify or @JmillsExpensify do either of you have an opinion on this?

When a policy has a title report field, we want to always use the value for the reportName. This is whatever is set in the template ex. {report:type} {report:startdate} or any of the other template values added from here

However - we can't always generate the template report name optimistically without data from the BE. An example would be a template like {report:oldID} {report:reimbursementid} {field:Employee ID}.

So when you create an expense report offline, since we don't know what the report title formula will end up being, what should we use as the reportName?

Maybe some placeholder text like Expense Report ${reportID} or Generated when back online, or just the formula itself ex {report:type} {report:startdate}?

paultsimura commented 7 months ago

or just the formula itself ex {report:type} {report:startdate}?

Couldn't we combine both? Replace the values we have on FE, and leave the formula for those we don't have.

Because info like report:type, report:id, or report:startdate we do have on the FE when creating the optimistic expense report. If we expect that majority of workspaces will keep using this default formula, then we can deliver a seamless experience to most of the clients. And if some WS will want to use some BE-dependent formula, then they'll see the {report:whatever} until they come back online. WDYT @thienlnam?

melvin-bot[bot] commented 7 months ago

@mananjadhav, @mallenexpensify, @thienlnam Whoops! This issue is 2 days overdue. Let's get this updated quick!

thienlnam commented 7 months ago

That's not a bad idea - though since this is an edge case, I personally think we should go with the simplest solution first and then see if there are any issues with it down the line in which case we can optimistically generate the formula

For now though, could we just

Show the formula value as is {report:type} {report:startdate}
Update the field so it shows as the pending update grey so that there is an indication it will change when online

trjExpensify commented 7 months ago

Sorry, this slipped off my radar. Remind me, if you have a report title formula on OldDot today with a value like {report:submit:date}, then we show that "raw" formula in the title of say an open unsubmitted report, right? Because there is no submitted date yet to include.

In which case, I don't think it's unreasonable to take this approach below, which is somewhat familiar in situations like above where you're including certain formulas in the title that can't be "calculated" yet:

Couldn't we combine both? Replace the values we have on FE, and leave the formula for those we don't have.

Because info like report:type, report:id, or report:startdate we do have on the FE when creating the optimistic expense report. If we expect that majority of workspaces will keep using this default formula, then we can deliver a seamless experience to most of the clients. And if some WS will want to use some BE-dependent formula, then they'll see the {report:whatever} until they come back online.

I do think if we have the data on the FE, we should use it and for the default report title on workspaces today we have that at least which is a good start - given people's tendency to not stray from the default all too much {report:type} {report:startdate}.

thienlnam commented 7 months ago

Sorry, this slipped off my radar. Remind me, if you have a report title formula on OldDot today with a value like {report:submit:date}, then we show that "raw" formula in the title of say an open unsubmitted report, right? Because there is no submitted date yet to include.

I tested this and it actually seems like we drop the formula value from the value until it's available.

With an enforced formula of {report:type} {report:startdate} {report:submit:date}

Open Report Screenshot 2024-02-20 at 3 09 57 PM

Report once submitted Screenshot 2024-02-20 at 3 10 12 PM

trjExpensify commented 7 months ago

Got it, and when all formulas being used in the default title are not available yet, we show the formula raw so it's not empty. Like here on an open report with a default title formula of {report:submit:date}:

So I think the consistent thing to do would be to align with OldDot, such that the report being viewed has a consistent title across both platforms.

If we can generate the formula in the report title on the FE, let's do it and show it.
If some of the default title and formulas can be shown, but some of them can't, let's show the ones we can and omit the ones we can't.
If all of the formulas in the title can't be generated (or what's being used is N/A for the report in its current state), then let's show the raw formula.

paultsimura commented 7 months ago

If we can generate the formula in the report title on the FE, let's do it and show it.

If some of the default title and formulas can be shown, but some of them can't, let's show the ones we can and omit the ones we can't.

If all of the formulas in the title can't be generated (or what's being used is N/A for the report in its current state), then let's show the raw formula.

Makes the most sense to me 👍 I've been communicating here back and forth without being assigned, but I think my proposal covers the latest expected behavior – just will keep the "missing formula" thing in mind when (if) working on the PR.

thienlnam commented 7 months ago

Yeah sounds good, @paultsimura has been talking through these requirements with us so I'll go ahead and assign

melvin-bot[bot] commented 7 months ago

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

Expensify / App