[HOLD for payment 2023-10-02] [$1000] Add automated tests for DeleteMoneyRequest

[luacmartins]

Problem

We added the DeleteMoneyRequest command here, but didn't include any automated tests for it.

Why is this important

Automated tests are the first line of defense against bugs!

Solution

Add tests to IOUTests.js

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0126b69171c61f91af
• Upwork Job ID: 1694018944195133440
• Last Price Increase: 2023-08-22
• Automatic offers:
• aimane-chnaif | Reviewer | 26306942
• rayane-djouah | Contributor | 26306944

[rayane-d]

@luacmartins, I would like to work on this issue if it goes external. I believe it doesn't require a proposal since it seems to be a straightforward task. Please let me know your thoughts.

[rayane-d]

Proposal

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

The current implementation of the deletemoneyrequest function in iou.js does not have automated tests. This lack of testing makes it difficult to ensure the functionality of the deletemoneyrequest function and increases the risk of introducing bugs or regressions when making changes to the codebase.

What is the root cause of that problem?

New Feature.

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

To address this problem, I propose adding automated tests for the deletemoneyrequest function in the ioutests.js file. The automated tests will validate the behavior of the function under different scenarios, ensuring its correctness and reliability. The tests will cover both positive and negative cases, including edge cases and boundary conditions.

Suggested test cases (we can adjust them as necessary):

• Money Request Deletion:
  It should delete a money request successfully.

• IOU Report Deletion:
  It should delete the IOU report when there are no visible comments left in the report.
  It should not delete the IOU report when there are visible comments left.

• Transaction Thread Deletion:
  It should delete the transaction thread if there are no visible comments in the thread.
  It should not delete the transaction thread if there are visible comments.

• Update Money Request Preview:
  It should update the moneyRequestPreview to show [Deleted request] when appropriate.

• IOU Report and Preview Update:
  It should update IOU report and reportPreview with new totals and messages if the IOU report isn't deleted.

• Navigation:
  It should navigate the user correctly based on the resources that were deleted and the view they are in.

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @isabelastisser (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[luacmartins]

@rayane-djouah thanks for the interest! Could you list the test cases you'd like to add?

[studentofcoding]

Hey @luacmartins could you share where's IOUTests.js located? or perhaps it's on the different name on the codebase? thanks!

[rayane-d]

@luacmartins thank you for your consideration, here is the list of test cases that I think we should add:

• Money Request Deletion:
  It should delete a money request successfully.

• IOU Report Deletion:
  It should delete the IOU report when there are no visible comments left in the report.
  It should not delete the IOU report when there are visible comments left.

• Transaction Thread Deletion:
  It should delete the transaction thread if there are no visible comments in the thread.
  It should not delete the transaction thread if there are visible comments.

• Update Money Request Preview:
  It should update the moneyRequestPreview to show [Deleted request] when appropriate.

• IOU Report and Preview Update:
  It should update IOU report and reportPreview with new totals and messages if the IOU report isn't deleted.

• Navigation:
  It should navigate the user correctly based on the resources that were deleted and the view they are in.

please let me know if I should add / remove / adjust any test case.

[luacmartins]

@studentofcoding https://github.com/Expensify/App/blob/8d5ae3a22a952677f8c48ce8a9c288f51a5eb8d0/tests/actions/IOUTest.js

@rayane-djouah test cases look good. I'll assign you to the issue. Feel free to get started on a PR.

[melvin-bot]

📣 @aimane-chnaif 🎉 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]

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

[rayane-d]

Thank you for the review. I will raise a PR ASAP

[melvin-bot]

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

[conorpendergrast]

@bfitzexpensify I'm on parental leave; re-assigning! This doesn't seem like to needs your work until payment

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @rayane-djouah got assigned: 2023-08-24 15:29:31 Z
• when the PR got merged: 2023-09-21 00:51:10 UTC
• days elapsed: 19

On to the next one 🚀

[melvin-bot]

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

[melvin-bot]

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

• [No QA] Add automated tests for delete money request #26029

If no regressions arise, payment will be issued on 2023-10-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @rayane-djouah requires payment offer (Contributor)
• @aimane-chnaif requires payment offer (Reviewer)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@aimane-chnaif] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[aimane-chnaif]

This doesn't require BZ checklist.

[rayane-d]

@luacmartins can the bounty amount for this issue be adjusted in accordance with its complexity? Thanks.

[bfitzexpensify]

@aimane-chnaif please complete the BZ checklist when you get a moment and I'll finalise payment for the C+ role - thanks!

[aimane-chnaif]

This doesn't require BZ checklist as commented here
This issue itself is regression test.

[bfitzexpensify]

Oops, missed that comment. Cool, ok, all paid out and we're done here. Thanks everyone

[rayane-d]

Bump regarding this comment @luacmartins @bfitzexpensify

[bfitzexpensify]

We only adjust bounties in very rare cases where the issue ended up being significantly different and more complex. I don't see that in this issue, and so the payment will remain at the price it was listed at.