search

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.58k stars 2.92k forks source link

[HOLD for payment 2024-12-07] [$250] RBR is shown on paid report previews #52037

Open iwiznia opened 3 weeks ago

iwiznia commented 3 weeks ago

I see a RBR in the workspace chat but not in the expense previews in it There are indeed violations in the expenses though as shown in the expense page. but they should not show the RBR in the workspace chat, since it is already paid.

image image image

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021854179900043130166
  • Upwork Job ID: 1854179900043130166
  • Last Price Increase: 2024-11-20
  • Automatic offers:
    • ikevin127 | Reviewer | 105021252
    • FitseTLT | Contributor | 105021254
Issue OwnerCurrent Issue Owner: @JmillsExpensify
melvin-bot[bot] commented 3 weeks ago

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

nyomanjyotisa commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-11-05 14:18:22 UTC.

Proposal

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

RBR is shown on paid report previews

What is the root cause of that problem?

We show the RBR on ReportPreview only based on the shouldShowRBR, not like in MoneyRequestPreviewContent where we show the RBR if the shouldShowRBR is true and the isSettled is false

https://github.com/Expensify/App/blob/e3452deb071e2af45b0c9afd879f4441e3d503b5/src/components/ReportActionItem/ReportPreview.tsx#L485-L490

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

Only show RBR if not settled, like we did in MoneyRequestPreviewContent

https://github.com/Expensify/App/blob/e3452deb071e2af45b0c9afd879f4441e3d503b5/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L353-L358

https://github.com/Expensify/App/blob/e3452deb071e2af45b0c9afd879f4441e3d503b5/src/components/ReportActionItem/ReportPreview.tsx#L485-L490

{!iouSettled && shouldShowRBR && (
    <Icon
        src={Expensicons.DotIndicator}
        fill={theme.danger}
    />
)}

What alternative solutions did you explore? (Optional)

iwiznia commented 3 weeks ago

We show the RBR without settled check in workplace chat

You can see in my first screenshot that the settled check is shown in the workspace chat report preview

nyomanjyotisa commented 3 weeks ago

@iwiznia ohh, I mean here we dont include the settled check, updated my proposal to make the RCA more clear

iwiznia commented 3 weeks ago

Ah ok, that's not correct as that would mean we never show RBR when settled, which is wrong.

FitseTLT commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-11-20 23:15:32 UTC.

Proposal

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

LHN - RBR stays for paid report

What is the root cause of that problem?

In all MoneyRequestPreview, MoneyReportView and LHN RBR we are showing RBR whenever there is a report or transaction violations without checking if the report is settled one https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/ReportActionItem/ReportPreview.tsx#L168-L170 https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L112 https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/LHNOptionsList/OptionRowLHNData.tsx#L40

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

We will need to make two changes according to the new summary given as all other parts of the summary are already included in our current code base

Red dots appear on expense report and transaction preview components as long as violations/rules have been broken and the report is not reimbursed or closed.

https://github.com/Expensify/App/blob/2d2555b173b47c0bce0b1c16b275f1f98639664c/src/components/LHNOptionsList/OptionRowLHNData.tsx#L41

    const shouldDisplayReportViolations = !ReportUtils.isSettled(fullReport) && ReportUtils.isReportOwner(fullReport) && ReportUtils.hasReportViolations(reportID);

https://github.com/Expensify/App/blob/2d2555b173b47c0bce0b1c16b275f1f98639664c/src/components/ReportActionItem/ReportPreview.tsx#L515

                                        {shouldShowRBR && !iouSettled && (

we can also add ReportUtils.isClosedReport check if needed

What alternative solutions did you explore? (Optional)

FitseTLT commented 3 weeks ago

@iwiznia We had this issue here and closed without fixing it, so re-commented the proposal here too.

jliexpensify commented 3 weeks ago

Thanks @FitseTLT - waiting on @iwiznia to review your comment.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

dexterlegrand commented 3 weeks ago

Contributor details Your Expensify account email: legranddexter08@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01166a125bd57d0627 I will modify the RBR logic to include a conditional check on the expense’s payment status and then update the logic so that if an expense is marked as paid, the RBR indicator is suppressed across all views, including the workspace chat. This can be achieved by adding a paymentStatus validation in the RBR component or middleware responsible for handling expense flags. For instance, ensure that the RBR check only applies when paymentStatus !== 'PAID'. And I can implement a centralized function or service that determines when to display the RBR flag, standardizing its behavior across different parts of the application (workspace chat, expense previews, and expense page). This service can be a function named like shouldDisplayRBR, which evaluates criteria such as isPaid, hasPolicyViolation, and any other relevant flags. All parts of the UI that display the RBR can then call this centralized function, ensuring consistency. If the workspace chat updates asynchronously or in real-time, it is important to ensure that any change in the expense’s status triggers an update in the workspace chat as well. In that case, I need to use websockets to keep the RBR status in sync with the expense's current state. And I will review the rendering conditions for RBR flags in each part of the UI. In my experience, many systems experiences inconsistencies due to scattered conditions across components. So I can refactor the RBR rendering logic to leverage a single source of truth if necessary. By implementing these changes, I can make the RBR indicator behaves consistently and aligns with the expected functionality.

melvin-bot[bot] commented 3 weeks ago

📣 @dexterlegrand! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 2 weeks ago

@jliexpensify, @ikevin127 Eep! 4 days overdue now. Issues have feelings too...

ikevin127 commented 2 weeks ago

We had a similar issue here which was closed as NAB:

the consensus there was that it is expected to still have RBR show because of existing violations, see https://github.com/Expensify/App/issues/48400#issuecomment-2367773018:

  • The red circle on showing on the report preview is working as intended, because the report has violations and we show that no matter what the state of the report is.
  • After paying, the red circle should remain in the report preview, for the same reason as in the first bullet
  • After clearing the cache, the report preview isn't showing the red circle, and after clicking on the preview, the transaction previews aren't showing the red circle. These are bugs because they should be showing for the same reason as in the first bullet

The https://github.com/Expensify/App/issues/48400#issuecomment-2390337662 that the issue was closed with:

From this https://github.com/Expensify/App/issues/48400#issuecomment-2367773018, seems part of the behavior is expected. Closing it out for now, but we can re-open if it's reported again as an issue

In case this issue is a dupe of that then I think we're good to close it as NAB - but otherwise if I'm missing something and this one is different then I think I'll have to start reviewing the proposals ? Please let me know!

cc @jliexpensify @iwiznia

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

jliexpensify commented 2 weeks ago

@iwiznia thoughts on this? I agree that the red dots are confusing, but also can understand @cead22's point here:

The red circle on showing on the report preview is working as intended, because the report has violations and we show that no matter what the state of the report is.

ikevin127 commented 2 weeks ago

Not overdue, waiting on input regarding https://github.com/Expensify/App/issues/52037#issuecomment-2469171616 before moving forward with the issue.

iwiznia commented 2 weeks ago

Oh @JmillsExpensify was going to post a summary of a discussion we had summarizing how this should work.

melvin-bot[bot] commented 2 weeks ago

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

ikevin127 commented 2 weeks ago

Not overdue, still waiting on input regarding https://github.com/Expensify/App/issues/52037#issuecomment-2469171616 before moving forward with the issue.

melvin-bot[bot] commented 1 week ago

@jliexpensify @ikevin127 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

JmillsExpensify commented 1 week ago

@iwiznia I still have this one on my list. Should I post the summary in this issue or create a new one?

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

iwiznia commented 1 week ago

Here sounds good to me

ikevin127 commented 1 week ago

Awaiting on summary to be posted coming from https://github.com/Expensify/App/issues/52037#issuecomment-2488894910 so we can move forward with the issue.

JmillsExpensify commented 1 week ago

Ok I'll prioritize that now.

JmillsExpensify commented 1 week ago

I think this is where we landed.

Terms

RBR - Exclusively refers to reports that are marked as unread and are visible in the LHN. RBRed rows in the LHN carry a red dot and are also always pinned. Red dot - Refers to a pattern of using a red circle incon – on expenses, reports, in settings, etc. Red dots disappear when either an X has been clicked, an issue with an expense has been resolved, or an expense report has been paid. Red text - Red words that appears in the transaction thread, next to whatever field has broken an amount (e.g. Missing category or Receipt required). Workspace chat - policyExpenseChat Preview component - The "boxes" or "cards" that appear for reports or transactions. Clicking a preview component shows the report or transaction details.

Logic for RBR and red dots

Submitters

FitseTLT commented 1 week ago

Most of the changes align with current code so Updated proposal to include the missing checks 👍

ikevin127 commented 1 week ago

I was able to reproduce the issue following these steps:

[!tip]

  1. Submit two failing scan expenses to another account (you have access to), this can be done by adding a picture that it's not an actual receipt such that the scan would fail and the expenses Amount is $0 -> violation asking to input Amount.
  2. Navigate to one of the failed scan expenses and input the Amount -> which would fix the violation of the expense.
  3. Login into the other account and pay the expenses via Pay elsewhere for both expenses (including the one with pending violation).
  4. Back on the expenses submitter side, in the report with the other account where the groupped expenses can be seen, notice that RBR is showing whlist if clicked on the preview and seeing the expenses separately, no RBR can be seen.

@JmillsExpensify @iwiznia If any of you think that I'm missing a certain flow like WS related expenses, please let me know the steps and will make sure to double check that the selected proposed solution is covering the fix for that flow as well.

@FitseTLT Any specific reason for adding the second check here:

https://github.com/Expensify/App/blob/2d2555b173b47c0bce0b1c16b275f1f98639664c/src/components/ReportActionItem/ReportPreview.tsx#L515

instead of here ?

https://github.com/Expensify/App/blob/056d6541d875378e0c2d2b6add5609cb9f49532f/src/components/ReportActionItem/ReportPreview.tsx#L365

If not, I'd say let's add it there as a way to centralize the shouldShowRBR variable at the component level instead of keeping it separate and adding !iouSettled only for the RBR icon, as shouldShowRBR is also used below to display the GBR indicator.

I see a RBR in the workspace chat but not in the expense previews in it There are indeed violations in the expenses though as shown in the expense page. but they should not show the RBR in the workspace chat, since it is already paid.

Assuming the Expected result of the issue is to not show the RBR on expense group if the expenses are paid (settled) and given the latest specifications are reinforcing that, let's go with @FitseTLT's proposal as the RCA is correct and the solution fixes the issue.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

JmillsExpensify commented 1 week ago

@ikevin127 that sounds good to me. In terms of testing, it'd also be easy to manually create an expense and then trigger one of the standard violations that get set when the policy is created ($2k individual expense, $25 receipt required amount, etc.).

ikevin127 commented 1 week ago

cc @grgia for visibility

melvin-bot[bot] commented 1 week ago

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

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

📣 @FitseTLT 🎉 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 📖

grgia commented 1 week ago

Looks good, assigning!

JmillsExpensify commented 1 week ago

Going to assign myself as BZ in case follow-on discussion comes up

ikevin127 commented 6 days ago

cc @grgia for visibility as we're awaiting PR review

melvin-bot[bot] commented 2 days ago

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

melvin-bot[bot] commented 2 days ago

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

If no regressions arise, payment will be issued on 2024-12-07. :confetti_ball:

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

melvin-bot[bot] commented 2 days ago

@ikevin127 @JmillsExpensify @ikevin127 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

ikevin127 commented 1 day ago

BugZero Checklist:

Bug classification Source of bug: - [ ] 1a. Result of the original design (eg. a case wasn't considered) - [x] 1b. Mistake during implementation - [ ] 1c. Backend bug - [ ] 1z. Other: Where bug was reported: - [ ] 2a. Reported on production - [ ] 2b. Reported on staging (deploy blocker) - [x] 2c. Reported on both staging and production - [ ] 2d. Reported on a PR - [ ] 2z. Other: Who reported the bug: - [ ] 3a. Expensify user - [x] 3b. Expensify employee - [ ] 3c. Contributor - [ ] 3d. QA - [ ] 3z. Other:

Regression Test Proposal

  1. Create a workspace and a required tag field for that workspace then invite another member.
  2. Enable workflows, add manual submission and set the admin as an approver.
  3. Create two expense from the employee side without selecting tag and observe that there is a RBR shown on the report preview.
  4. Approve and pay the expense report from the admin side and from the employee side verify that the RBR is now cleared from the report preview and not showing anymore.

Do we agree 👍 or 👎.