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

[HOLD for payment 2024-06-03] [Violations - Pending Receipts] Display the rter Violation with the Pending Pattern #39533

Closed
yuwenmemon opened this issue Apr 3, 2024 · 56 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 Engineering Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item.

Comments

@yuwenmemon
Copy link
Contributor

yuwenmemon commented Apr 3, 2024

Design Doc: https://docs.google.com/document/d/1zJqlTe_RajuBtfQYvbMx8PpXgA9CEnUGVyuqZihQ-ok/edit
Tracking GH: https://github.com/Expensify/Expensify/issues/372206

HOLD ON https://github.com/Expensify/Expensify/issues/384996


Similar to how we have TransactionUtils.isPending and TransactionUtils.isReceiptBeingScanned in NewDot to help us with other pending UI flows, we'll add a new method: TransactionUtils.hasPendingRTERViolation:

TransactionUtils.hasPendingRTERViolation

This will have a similar structure to the current TransactionUtils methods that check for / retrieve violations.

  • Params:
    • transactionID: string
    • transactionViolations: TransactionViolation[] - the set of violations for that specific transaction. For components where we are subscribing to all violations we can use the output of TransactionUtils.getTransactionViolations
  • Returns:
    • A boolean: If any of the violations for that transactionID in Onyx are the rter violation, with pendingPattern set to true

Additionally, because this will be called frequently with TransactionUtils.isPending and TransactionUtils.isReceiptBeingScanned let's also make and utilize a method that returns true if any of those return true: hasPendingUI()

We'll outline this in further detail below, but we'll call this method in:

  1. MoneyRequestPreviewContent.tsx
  2. ReportPreview.tsx
  3. MoneyRequestHeader.tsx

The first two already have access to the transactionViolations onyx collection key. However, we will need to add it to MoneyRequestHeader.tsx.

Updates to ReportActionItems: MoneyRequestPreview / ReportPreview

Now that we know if our transaction has the "7-Day Hold" rter violation, we can adjust the UI to reflect that it's "Pending". For ReportActionItem rows, we will do the same thing that was outlined for pending transactions and scanning receipts in the Update how we display / report expenses doc.
Screenshot 2024-04-03 at 11 27 42 AM

We will show the "pending" hover/message/icon UI for these two cases:

Updates to MoneyReportHeader

Similar to what we're doing for the Report Preview above, if the report contains only transactions with the rter violation (which again, we'll alias as hasPendingRTERViolation) we'll show the Next Steps banner indicating that the receipts are pending match with a credit card.

We'll also hide the Submit/Approve/Pay buttons in this case as well (i.e. all transactions are pending).

In the case of the "One-Expense" Chat, we'll use the same pattern as the MoneyRequestHeader as described below.

Updates to MoneyRequestHeader

If hasPendingRTERViolation is true, we'll show a MoneyRequestHeaderStatusBar in the MoneyRequestHeader here. This will be similar to what we already do for isPending and isScanning. The message will utilize the same hourglass icon and have the copy as seen below.

Let's also refactor this to be a bit less repetitive. We can create a getPendingType() method that checks if the transaction is RTER vs SCANNING vs PENDING and returns the translations keyed by that type.

Screenshot 2024-04-03 at 11 34 09 AM

"Mark as cash" Button

This will be covered in a separate issue

Issue OwnerCurrent Issue Owner: @mallenexpensify
@yuwenmemon yuwenmemon added Engineering NewFeature Something to build that is a new item. Daily KSv2 labels Apr 3, 2024
Copy link

melvin-bot bot commented Apr 3, 2024

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 3, 2024
Copy link

melvin-bot bot commented Apr 3, 2024

⚠️ 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 ⚠️

@JmillsExpensify JmillsExpensify moved this to Release 1: Spring 2024 (May) in [#whatsnext] #expense Apr 4, 2024
@yuwenmemon yuwenmemon changed the title [HOLD][Violations - Pending Receipts] Display the rter Violation with the Pending Pattern [Violations - Pending Receipts] Display the rter Violation with the Pending Pattern Apr 4, 2024
@yuwenmemon
Copy link
Contributor Author

Removing the HOLD on this

@grgia grgia self-assigned this Apr 5, 2024
@grgia
Copy link
Contributor

grgia commented Apr 5, 2024

Gonna assign to track if we end up merging this with the other pending changes in this issue

Slack convo in SWM- https://expensify.slack.com/archives/C04878MDF34/p1712273151578619

cc @BrtqKr :)

@grgia
Copy link
Contributor

grgia commented Apr 8, 2024

@BrtqKr could you comment so I can assign you? thanks!

@BrtqKr
Copy link
Contributor

BrtqKr commented Apr 8, 2024

Hey, I'd like to take over this issue

@grgia grgia added Daily KSv2 and removed Weekly KSv2 labels Apr 9, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 11, 2024
@grgia
Copy link
Contributor

grgia commented Apr 12, 2024

in progress

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 12, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

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

@grgia
Copy link
Contributor

grgia commented Apr 16, 2024

cc @smelaa are you working on this? If so, please comment so I can switch assignments

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

smelaa commented Apr 16, 2024

Hi, I am Aleksandra Smela from Software Mansion, an expert agency, and I would like to work on this issue

@yuwenmemon yuwenmemon assigned smelaa and unassigned BrtqKr Apr 16, 2024
@smelaa
Copy link
Contributor

smelaa commented Apr 16, 2024

Hi, as I am diving into the code I have a question to the design.

We will show the "pending" hover/message/icon UI for these two cases:
if the action is a Money Request Action and the linked Transaction has the rter violation…
if the action is a Report Preview containing a single transaction with the rter violation…

From what I understand we need to show „pending” hover/message/icon from the level of ReportActionItem. I was thinking to just add an additional condition here. The problem is that we do not have an access to transaction violations in ReportActionItem.
The solution might be to add Onyx.connect in TransactionUtils for transaction violations as we do for allTransactions and then just filter proper violations by transactionID.
Please let me know if that would be okay or I should find a different approach.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 27, 2024
@melvin-bot melvin-bot bot changed the title [Violations - Pending Receipts] Display the rter Violation with the Pending Pattern [HOLD for payment 2024-06-03] [Violations - Pending Receipts] Display the rter Violation with the Pending Pattern May 27, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

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

Copy link

melvin-bot bot commented May 27, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.75-1 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-06-03. 🎊

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

  • @ishpaul777 requires payment (Needs manual offer from BZ)
  • @smelaa does not require payment (Contractor)

Copy link

melvin-bot bot commented May 27, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@ishpaul777] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 2, 2024
Copy link

melvin-bot bot commented Jun 3, 2024

Payment Summary

Upwork Job

  • ROLE: @ishpaul777 paid $(AMOUNT) via Upwork (LINK)
  • Contributor: @smelaa is from an agency-contributor and not due payment

BugZero Checklist (@mallenexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@mallenexpensify
Copy link
Contributor

@ishpaul777 can you propose regression test steps plz?
Also... how much are you due here? If this is due more than the standard $250, please provide reasoning.

@ishpaul777
Copy link
Contributor

i dont have the full context for the feature (like how to get the rter Violation with a real user account). I am pretty sure the design doc in the issue Description should have the testing steps for whole feature. @mallenexpensify Can you please confirm i dont have access to docs

@ishpaul777
Copy link
Contributor

The issue misses the priorty tag, but we worked on this one as a HIGH priorty ticket so i think it should be $500 @yuwenmemon Can you please help in evaluatuion

@yuwenmemon
Copy link
Contributor Author

Yeah, that's accurate @ishpaul777 👍

@mallenexpensify
Copy link
Contributor

Yeah, that's accurate @ishpaul777 👍

I'm assuming this is related to us not needing regression steps here cuz it'll be covered in testing steps as part of the design doc.

@ishpaul777 , I posted this in #expensify-open-source yesterday

There have been questions about what constitutes a job being auto-increased to $500. These jobs are only the ones that have the High Priority label added to them (ie. issue with HIGH or CRITICAL in the title or project status/priority are not auto-increased to $500).

So the default amount here would be $250. if you believe compensation should be higher, follow the process here

@ishpaul777
Copy link
Contributor

ishpaul777 commented Jun 5, 2024

@mallenexpensify Well the issue was created before April 5th so isn't the default still be $500 ?

https://expensify.slack.com/archives/C02NK2DQWUX/p1715196549664219?thread_ts=1715190852.677769&cid=C02NK2DQWUX

@mallenexpensify
Copy link
Contributor

Thanks for the link @ishpaul777 , for this instance, you are correct. I commented on that thread to state this shouldn't be the practice for any future price updates.

In the future, when we have price changes or, if there are other instances where we're deciding what the price should be, we shouldn't always default to the created date. We have closed issues from when we paid $2k for a job, if we reopened one today, we'd want the default price to be $250. Same goes for old issues where we haven't marked external yet, also for one's that needed reliable reproduction steps before making external.

@mallenexpensify mallenexpensify added Internal Requires API changes or must be handled by Expensify staff Bug Something is broken. Auto assigns a BugZero manager. labels Jun 5, 2024
Copy link

melvin-bot bot commented Jun 5, 2024

Current assignee @mallenexpensify is eligible for the Bug assigner, not assigning anyone new.

@mallenexpensify
Copy link
Contributor

@ishpaul777 can you please accept the job and reply here once you have?
https://www.upwork.com/ab/applicants/1798501240605888788/job-details

@ishpaul777
Copy link
Contributor

Accepted 😄

@melvin-bot melvin-bot bot added the Overdue label Jun 10, 2024
@mallenexpensify
Copy link
Contributor

Contributor+: @ishpaul777 paid $500 via Upwork.

Thanks all!

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 Engineering Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item.
Projects
Status: Done
Development

No branches or pull requests

8 participants