handling old/stale account requests

[manuel1280]

Checklist:

• I have performed a self-review of my own code,
• I have commented my code, particularly in hard-to-understand areas,
• I have made corresponding changes to the documentation,
• I have added tests that prove my fix is effective or that my feature works,
• New and existing unit tests pass locally with my changes ("bundle exec rake"),
• Title includes "WIP" if work is in progress.

Resolves #4514

Description

This new button allows admin users to close old and stale Account requests without sending emails or notifications. this new feature is relevant to clean up data easily. my approach for the solution was:

• I reused the existing button 'Reject' and its modal by a dynamic code in javascript
• unnecessary code was removed and reactors were made
• following the current pattern of no-rest actions, the 'close' action was created
• create test files

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

running these files

• account_request_spec.rb
• account_requests_controller_spec.rb
• manually testing

[awwaiid]

Looks really great -- one very minor request to change the type of db update to ! method so it'll raise an exception as needed.

[awwaiid]

Suggested change

	update(status: 'admin_closed', rejection_reason: reason)
	update!(status: 'admin_closed', rejection_reason: reason)

Use an ! method like the others here so that it'll raise an exception if anything goes wrong

[manuel1280]

done!

[awwaiid]

Ah -- I also ran all of the tests and you can see that a browser (system) spec failed, please take a look at that as well.

[cielf]

Minor issue: During functional testing, I noted that the rejection reason has an asterisk on it, which indicates that it's mandatory, but I was able to complete the close without providing the reason.
Can you make the field mandatory, on close, please?

[manuel1280]

@cielf Hi, the Change is done, I made some code changes to make the validation easier

[cielf]

Functionality looks good now.

[dorner]

Had some further notes.

[dorner]

Sorry - changing this to update! means that this method will never return false - it'll raise an error if it can't save. That means you need to handle this error and return the message to the user rather than rely on true/false returns from this method.

[manuel1280]

I changed, what do you think?

[dorner]

You should test where the update errors out (e.g. introduce a validation error). That would have caught the mistake above 😄

[dorner]

Can we combine the expectations here with the request specs, so we don't need to do the same actions twice? We can just remove the request specs.

[manuel1280]

I Couldn't test the validation errors in this spec because the button to "Close" is only displayed on the open requests, therefore I tested the validation on the controller spec (above), also I removed the duplicated tests in that controller to avoid the same action twice

[dorner]

Looks good on my end. @cielf it's probably worth it to do one more test for function.

If I understand correctly, this requested change wasn't actually going to work correctly -- and @dorner has approved the final version.

[github-actions]

@manuel1280: Your PR handling old/stale account requests is part of today's Human Essentials production release: 2024.09.15.
Thank you very much for your contribution!