[HOLD for payment 2024-07-22] [Unapprove in NewDot - Collect] Show Unapprove btn, add Confirm modal, call API endpoint

Beamanator commented 3 months ago

Tracking issue: https://github.com/Expensify/Expensify/issues/384891

Note: This issue can't be MERGED until https://github.com/Expensify/Expensify/issues/406171 is finished. But it can be started while that other issue is WIP 🙏

Design doc sections:

Goals:

Show the Unapprove button to the correct people at the correct time in the correct place
When user clicks the button, show new ConfirmModal IFF report has been exported to connected accounting package
- Note: I probably need to add more details about when this needs to get shown
Call new API endpoint, UnapproveExpenseReport
- Unapproving is Offline Pattern B

Note: For the "show warning modal if report has already been exported to accounting package" stuff, don't implement that here! That will be covered in https://github.com/Expensify/App/issues/44884

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @isabelastisser

melvin-bot[bot] commented 3 months ago

Current assignee @rushatgabhane is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @isabelastisser (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 3 months 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 3 months ago

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

Beamanator commented 3 months ago

@rushatgabhane is assigned to complete this job 👍

isabelastisser commented 3 months ago

@rushatgabhane, can you please provide an update? Thanks!

rushatgabhane commented 3 months ago

PR above ^

We're waiting for us to tell whether a report is exported to accounting https://github.com/Expensify/App/pull/44229#discussion_r1650294292

Beamanator commented 3 months ago

FYI I've got a few more backend changes I'm making today to hopefully this this App PR ready for testing mid-week

Beamanator commented 3 months ago

FYI backend changes have all been merged, but it will probably take another day (or some time later today) before it gets to staging

Beamanator commented 3 months ago

@rushatgabhane backend changes shouldddd all be on Staging! Your PR can be tested now in staging! (not technically "off hold" till the next deploy, which may happen Monday)

Beamanator commented 3 months ago

Note: added this to the OP:

Note: For the "show warning modal if report has already been exported to accounting package" stuff, don't implement that here! That will be covered in https://github.com/Expensify/App/issues/44884

Julesssss commented 2 months ago

hey @Beamanator, here's a server error that I think is related to these changes. I removed the blocker label as this seems to be a new App feature.

Throw ExpException - 8ba5fe883b2f7d19c56045fcd8330f36 ~~ message: '403 Incorrect report type' exceptionMessage: 'Auth UnapproveExpenseReport returned an error' exceptionFile: '/git/releases/expensify.com/1fe9c8e/lib/Auth.php' exceptionLine: '126' exceptionCode: '403'

melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

blimpich commented 2 months ago

Follow up issue related to this that came up as a deploy blocker (demoted because edge case): https://github.com/Expensify/App/issues/44991.

melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

jayeshmangwani commented 2 months ago

@isabelastisser Can you please assign me to the issue, so that I can keep track it on K2?

melvin-bot[bot] commented 2 months ago

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

https://github.com/Expensify/App/pull/44229

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

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

@rushatgabhane requires payment through NewDot Manual Requests
@jayeshmangwani requires payment through NewDot Manual Requests

melvin-bot[bot] commented 2 months ago

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:

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

isabelastisser commented 2 months ago

@rushatgabhane, please complete the BZ list. Thanks!

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @sakluger (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

isabelastisser commented 2 months ago

Payment summary:

@rushatgabhane requires payment through NewDot Manual Requests $250 C+ review
@jayeshmangwani requires payment through NewDot Manual Requests $250 Contributor

I will be OOO tomorrow and next week, so I am reassigning this until I return on July 29. Thanks, @sakluger!

Status: Payment is due on July 22.

jayeshmangwani commented 2 months ago

The Test steps from the PR will be sufficient for the regression test.

Regression Test Proposal

Go to a control policy as an admin.
Make yourself the approver.
Submit an expense to the policy.
Approve the expense and go to the expense.
Click on the report header.
Verify that we can see the "Unapprove" button.
Verify that after clicking "Unapprove" we can see the "unapproved $x" message in the chat.

Connect to an accounting integration

Connect to Xero / QuickBooks / Sage / NetSuite.
Submit an expense to the policy.
Approve the expense and go to the expense.
Click the report header.
Verify that we can see the "Unapprove" button.
Click "Unapprove" and verify a modal shows a warning that you're connected to an accounting integration.
Verify that after clicking "Unapprove" we can see an "unapproved $x" message in the chat.

Do we agree 👍 or 👎

melvin-bot[bot] commented 2 months ago

Payment Summary

[Upwork Job]()

Reviewer: @rushatgabhane owed $250 via NewDot
Reviewer: @jayeshmangwani owed $250 via NewDot

BugZero Checklist (@sakluger)

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

JmillsExpensify commented 2 months ago

$250 approved for @rushatgabhane

sakluger commented 2 months ago

Looks good! Closing 👍

jayeshmangwani commented 2 months ago

Requested $250

JmillsExpensify commented 2 months ago

$250 approved for @jayeshmangwani

Expensify / App