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

Fixed #14935 - improvements and more tests around user deletion #14937

Merged
merged 22 commits into from
Jun 22, 2024

Conversation

snipe
Copy link
Owner

@snipe snipe commented Jun 22, 2024

This adds a bunch more tests around user deletion, and back ports a fix that I had to hotfix as well regarding a typo on user deletion and a bad path to the error message if you're trying to delete a user that is already deleted. While I was in there, I also updated some of the docbloc return signatures.

Copy link

what-the-diff bot commented Jun 22, 2024

PR Summary

  • New Models Added in User API Controller
    Two new models, Accessory and Consumable, are now imported in the UsersController.php file for the app's API. This will provide more comprehensive user-related information within the app.

  • Return Types Updated in User API Controller
    The return types of multiple methods in the UsersController.php file have been changed to either arrays or JsonResponse. This makes the API more flexible and enables our servers to reply using structured JSON data, improving the clarity and usability of the responses.

  • Changes in the Main User Controller
    Extra libraries for logging and storage are added to UsersController.php. Also, similar to the API counterpart, the destroy method's return type is updated. This can provide additional useful insights while debugging issues, and facilitate file-related operations.

  • Enhancements in Delete User Request
    The DeleteUserRequest.php file experienced several changes, notably transitioning the authorize method to utilize the Gate class, which adds an extra layer of authorization. Validation for whether a user has already been deleted has also been added alongside custom error messages to promote clarity for users.

  • User Factory Update
    A new 'deleted user' state has been crafted in UserFactory.php, allowing tests to simulate this scenario in a simplified way.

  • Expansion of API-based User Deletion Tests
    New tests to validate error handling for non-existent or previously deleted users are added in DeleteUserTest.php. In addition, more precise tests are established for permission validation during user deletion.

  • UI-based User Deletion Tests Expansion
    New tests have been added to DeleteUserTest.php, including scenarios for users deleting other users, as well as handling non-existent or already-deleted users. Modifications have also been done to test the permissions to delete a user interface.

Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bunch of test additions, and I'm definitely glad to have them. I'd like to dig around the one 'skipped' test a little, but this feels like we're on the right track. Thank you!

app/Http/Controllers/Api/UsersController.php Outdated Show resolved Hide resolved
app/Http/Requests/DeleteUserRequest.php Outdated Show resolved Hide resolved
tests/Feature/Users/Api/DeleteUserTest.php Outdated Show resolved Hide resolved
@snipe snipe changed the title WIP - improvements and more tests around user deletion Fixed #14935 - improvements and more tests around user deletion Jun 22, 2024
@snipe snipe merged commit 25fcf52 into develop Jun 22, 2024
9 checks passed
@snipe snipe deleted the fixes/user_improvements branch June 22, 2024 20:41
@snipe snipe removed the restore label Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants