Add checks for editing APPROVED reports #33633

youssef-lr · 2023-12-26T22:31:46Z

Details

Fixed Issues

$ #29914
PROPOSAL:

Tests

Verify that no errors appear in the JS console

IOUs (P2P requests)

As User A, request money from User B.
Make sure you're able to edit its all details as User A.
Log in as User B, make sure you can't edit its details.
Settle the request elsewhere as User B.
Log in as User A, make sure you can't edit the request now that it's been settled.

Expense reports (request made in a workspace)

Create a workspace as the Admin and invite User A.
Log in as User A and request money from the workspace chat using the 'Scan' tab.
Make sure you can edit all the details, and go ahead and edit all of them.
Log in as the Admin, make sure you can edit all the details of the request, except the Receipt.
Settle the request. Make sure you cannot edit the Amount/Currency, Merchant, Date, and Receipt.
Log in as User A, make sure you can't edit the request anymore.

Expense reports - Collect policy

We will need three accounts here, an admin, the approver, and the submitter.

As the submitter, create a request in the workspace chat using 'Scan' and submit it.
Still as the submitter, make sure you can edit all the request details while it's processing, and go ahead and edit all of them
Log in as the approver, make sure you can edit all the details except the receipt.
Approve the request, make sure you cannot edit the Amount/Currency, Merchant, Date, and Receipt.
Log in as the admin, verify the same as Step 4.
Pay the request as the admin, repeat verifications from Step 4.
Log in as the submitter now, make sure you can't edit any details from the request.

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2023-12-26T22:31:51Z

@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

youssef-lr · 2023-12-26T22:32:50Z

@mananjadhav can you please give me three brand new emails so I can set you up in a Collect test policy, something like:

manan+submitter, manan+approver, manan+admin.

youssef-lr · 2023-12-28T08:28:19Z

Friendly bump @mananjadhav, this issue is tied to our roadmap, if we can treat it with urgency it'd be great 🙏

mananjadhav · 2023-12-28T08:35:13Z

Ohh I missed this. I can take a look today.

mananjadhav · 2023-12-28T15:39:57Z

@youssef-lr While I start with the review, can you fix the Typescript error? and work on the author checklist?

youssef-lr · 2023-12-29T14:21:41Z

@mananjadhav I'm on it, but you can test everything meanwhile, it shouldn't be a blocker

youssef-lr · 2023-12-29T14:21:55Z

bump on this please

mananjadhav · 2023-12-29T14:34:22Z

I started and I'll take 2-3 hours.

youssef-lr · 2023-12-29T14:37:03Z

I'm honestly not sure why TypeScript is showing that error, the same line is used here as well...do you have any clues?

mananjadhav · 2023-12-29T16:37:00Z

I am still away from keyboard and I am not sure if I can get to it for next few hours. I've asked to reassign if someone can get it earlier.

mananjadhav · 2023-12-29T16:53:23Z

I'm honestly not sure why TypeScript is showing that error, the same line is used here as well...do you have any clues?

I am also not sure, but I checked the other method. When I hover over reportAction, I see the type as ReportActionBase & OriginalMessageIOU. This is despite reportAction being defined as OnyxEntry<ReportAction>. I didn't get time to further investigate. Sorry.

c3024 · 2023-12-29T17:13:43Z

src/libs/ReportUtils.ts

+        return true;
+    }
+
+    const reportID = reportAction?.originalMessage?.IOUReportID ?? 0;


At the other two places, where we are using this reportAction?.originalMessage?.IOUReportID reportAction is narrowed to CONST.REPORT.ACTIONS.TYPE.IOU. If we narrow it down here as well typescript will stop complaining.

Suggested change

const reportID = reportAction?.originalMessage?.IOUReportID ?? 0;

const reportID = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? reportAction?.originalMessage?.IOUReportID : 0;

Great catch, thanks! I'm going to cast instead because we know for sure the type of the action here

c3024 · 2023-12-29T17:22:34Z

In production, once the request is settled, the fields are not pressable at all for the requestor and there are no chevrons.

With this PR, these fields are pressable and there are chevrons but nothing happens on press.

I think the production design is the correct design?

youssef-lr · 2023-12-29T17:24:37Z

Looks like a bug in my PR, will look into it.

youssef-lr · 2023-12-29T17:38:12Z

I'm not able to reproduce it @c3024, I think just navigate away and back from the report and they should get disabled, this is an issue with Pusher updates I think

c3024 · 2023-12-29T17:43:30Z

Yes, refreshing the page fixes it. useMemo is the issue. The settled status green checkmark appears fine even without refreshing or navigating to another page and returning so I don't think it is due to the Pusher update.

youssef-lr · 2023-12-29T17:44:44Z

hmm, I will look into. Wouldn't Pusher update the dependencies of useMemo?

c3024 · 2023-12-29T17:47:50Z

I think it should but I just checked with removing useMemo in all canEditX fields and it worked fine.

src/libs/ReportUtils.ts

Co-authored-by: Vit Horacek <[email protected]>

mountiny

Thank you!

youssef-lr · 2024-01-05T15:42:26Z

src/components/ReportActionItem/MoneyRequestView.js


    // A flag for verifying that the current report is a sub-report of a workspace chat
-    const isPolicyExpenseChat = useMemo(() => ReportUtils.isPolicyExpenseChat(ReportUtils.getRootParentReport(report)), [report]);
+    // if the policy of the report is either Collect or Control, then this report must be tied to workspace chat
+    const isPolicyExpenseChat = ReportUtils.isGroupPolicy(report);


@mountiny woops this doesn't take into account Free policies, I'll revert this back to testing the policy is not personal

Can you also rename it to isPaidGroupPolicy?

And create a new one name isGroupPolicy checking for free too? it can be done in a follow up too

yeah that sounds good

done, though I just found out we have the same method in PolicyUtils

I think we're creating a lot of redundant helper methods we should probably some day clean things up :D

mountiny · 2024-01-05T17:09:49Z

src/libs/ReportUtils.ts

-function isDraftExpenseReport(report: OnyxEntry<Report>): boolean {
-    return isExpenseReport(report) && report?.stateNum === CONST.REPORT.STATE_NUM.OPEN && report?.statusNum === CONST.REPORT.STATUS.OPEN;
+function isDraftExpenseReport(report: OnyxEntry<Report> | EmptyObject): boolean {
+    return report?.stateNum === CONST.REPORT.STATE_NUM.OPEN && report?.statusNum === CONST.REPORT.STATUS.OPEN;


Can you please bring this back? I think that there is code which relies on the check for expense report, afaik for you changes it will work too, only expense reports can be in open state so we should keep it in

woops sorry it was a mistake

src/libs/ReportUtils.ts

mountiny

Thanks!

melvin-bot · 2024-01-05T17:57:03Z

@youssef-lr looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

youssef-lr · 2024-01-05T17:57:37Z

Tests passed

OSBotify · 2024-01-05T18:01:14Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-01-09T05:37:12Z

🚀 Deployed to staging by https://github.com/youssef-lr in version: 1.4.23-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-01-10T06:04:27Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.23-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

youssef-lr added 3 commits December 25, 2023 02:56

Add support for editing approved reports

ba930e4

Cleanup

bcb8dc4

Fix bugs

c41b554

youssef-lr self-assigned this Dec 26, 2023

youssef-lr requested a review from a team as a code owner December 26, 2023 22:31

melvin-bot bot requested review from mananjadhav and removed request for a team December 26, 2023 22:31

Lint

0459190

youssef-lr mentioned this pull request Dec 29, 2023

[HOLD for payment 2024-01-17] TU - Fix admin and approver editing request details on paid plan requests. #29914

Closed

6 tasks

Merge branch 'main' into youssef_edit_approveed_reports

394c82b

c3024 reviewed Dec 29, 2023

View reviewed changes

youssef-lr added 2 commits December 29, 2023 18:21

Fix typescript commplaining

d273fc6

Add safety check

7ba5146

youssef-lr force-pushed the youssef_edit_approveed_reports branch from f66717a to ef32e99 Compare January 4, 2024 22:53

Cleanup

7de81d4

youssef-lr force-pushed the youssef_edit_approveed_reports branch from ef32e99 to 7de81d4 Compare January 4, 2024 22:58

Remove unneeded check

4123a31

mountiny requested changes Jan 5, 2024

View reviewed changes

src/libs/ReportUtils.ts Outdated Show resolved Hide resolved

src/libs/ReportUtils.ts Outdated Show resolved Hide resolved

src/libs/ReportUtils.ts Show resolved Hide resolved

src/libs/ReportUtils.ts Show resolved Hide resolved

youssef-lr and others added 3 commits January 5, 2024 16:15

Update src/libs/ReportUtils.ts

a130cb3

Co-authored-by: Vit Horacek <[email protected]>

use isDraftExpenseReport

b6ba955

Move variables into the scope where they're needed

1707e03

mountiny previously approved these changes Jan 5, 2024

View reviewed changes

youssef-lr commented Jan 5, 2024

View reviewed changes

Rename methods

8491e86

youssef-lr dismissed mountiny’s stale review via 8491e86 January 5, 2024 16:24

mountiny requested changes Jan 5, 2024

View reviewed changes

Add code removed by mistake

3525c5e

mountiny requested changes Jan 5, 2024

View reviewed changes

src/libs/ReportUtils.ts Show resolved Hide resolved

src/libs/ReportUtils.ts Show resolved Hide resolved

Make code clearer

316b718

mountiny approved these changes Jan 5, 2024

View reviewed changes

youssef-lr merged commit e8ec48b into main Jan 5, 2024

youssef-lr deleted the youssef_edit_approveed_reports branch January 5, 2024 17:56

melvin-bot bot added the Emergency label Jan 5, 2024

youssef-lr removed the Emergency label Jan 5, 2024

github-actions bot mentioned this pull request Jan 9, 2024

Deploy Checklist: New Expensify 2024-01-09 #34121

Closed

87 tasks

unidev727 mentioned this pull request Jan 9, 2024

IOU - An administrator cannot edit an unpaid IOU created by an employee #34181

Closed

6 tasks

dummy-1111 mentioned this pull request Jan 9, 2024

[HOLD for payment 2024-01-18] [HOLD for payment 2024-01-17] Scan - No red dot for Amount and Merchant when viewing failed scan request as Admin #34179

Closed

6 tasks

bernhardoj mentioned this pull request Jan 12, 2024

[$500] Request money - Deleted request shows empty details when the details page has messages #34425

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks for editing APPROVED reports #33633

Add checks for editing APPROVED reports #33633

youssef-lr commented Dec 26, 2023 •

edited

Loading

melvin-bot bot commented Dec 26, 2023

youssef-lr commented Dec 26, 2023

youssef-lr commented Dec 28, 2023

mananjadhav commented Dec 28, 2023

mananjadhav commented Dec 28, 2023

youssef-lr commented Dec 29, 2023

youssef-lr commented Dec 29, 2023

mananjadhav commented Dec 29, 2023

youssef-lr commented Dec 29, 2023

mananjadhav commented Dec 29, 2023

mananjadhav commented Dec 29, 2023

c3024 Dec 29, 2023

youssef-lr Dec 29, 2023

c3024 commented Dec 29, 2023 •

edited

Loading

youssef-lr commented Dec 29, 2023

youssef-lr commented Dec 29, 2023

c3024 commented Dec 29, 2023 •

edited

Loading

youssef-lr commented Dec 29, 2023

c3024 commented Dec 29, 2023 •

edited

Loading

mountiny left a comment

youssef-lr Jan 5, 2024

mountiny Jan 5, 2024

youssef-lr Jan 5, 2024

youssef-lr Jan 5, 2024

youssef-lr Jan 5, 2024

mountiny Jan 5, 2024

youssef-lr Jan 5, 2024

mountiny left a comment

melvin-bot bot commented Jan 5, 2024

youssef-lr commented Jan 5, 2024

OSBotify commented Jan 5, 2024

OSBotify commented Jan 9, 2024

OSBotify commented Jan 10, 2024

	const reportID = reportAction?.originalMessage?.IOUReportID ?? 0;
	const reportID = reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU ? reportAction?.originalMessage?.IOUReportID : 0;

Add checks for editing APPROVED reports #33633

Add checks for editing APPROVED reports #33633

Conversation

youssef-lr commented Dec 26, 2023 • edited Loading

Details

Fixed Issues

Tests

IOUs (P2P requests)

Expense reports (request made in a workspace)

Expense reports - Collect policy

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 26, 2023

youssef-lr commented Dec 26, 2023

youssef-lr commented Dec 28, 2023

mananjadhav commented Dec 28, 2023

mananjadhav commented Dec 28, 2023

youssef-lr commented Dec 29, 2023

youssef-lr commented Dec 29, 2023

mananjadhav commented Dec 29, 2023

youssef-lr commented Dec 29, 2023

mananjadhav commented Dec 29, 2023

mananjadhav commented Dec 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c3024 commented Dec 29, 2023 • edited Loading

youssef-lr commented Dec 29, 2023

youssef-lr commented Dec 29, 2023

c3024 commented Dec 29, 2023 • edited Loading

youssef-lr commented Dec 29, 2023

c3024 commented Dec 29, 2023 • edited Loading

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Jan 5, 2024

youssef-lr commented Jan 5, 2024

OSBotify commented Jan 5, 2024

OSBotify commented Jan 9, 2024

OSBotify commented Jan 10, 2024

youssef-lr commented Dec 26, 2023 •

edited

Loading

c3024 commented Dec 29, 2023 •

edited

Loading

c3024 commented Dec 29, 2023 •

edited

Loading

c3024 commented Dec 29, 2023 •

edited

Loading