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

feat: build admin lock/unlock account flow #171

Conversation

UditThakkar
Copy link
Collaborator

@UditThakkar UditThakkar commented Apr 2, 2025

This pull request introduces a new REST API endpoint /admin/toggleLockStatus that allows adminsto toggle the lock status of a user account.

Key changes include:

  • Creation of the toggleLockStatus POST endpoint under the /admin path.
  • Implementation of the endpoint to receive a LockAccountDto containing the user's email.
  • Integration with the UserService to perform the logic of toggling the account lock status.
  • Implementation of input validation for the LockAccountDto to ensure the email is provided.
  • Implementation of proper error handling for scenarios such as user not found and invalid input, returning appropriate HTTP status codes and JSON responses.

Link to the issue: #32

feat: added test cases
@UditThakkar UditThakkar requested a review from devondragon April 2, 2025 15:51
@UditThakkar UditThakkar linked an issue Apr 2, 2025 that may be closed by this pull request
@UditThakkar UditThakkar changed the title feat: build admin lock account flow feat: build admin lock/unlock account flow Apr 2, 2025
@devondragon devondragon requested a review from Copilot April 2, 2025 18:20
@devondragon devondragon added the enhancement New feature or request label Apr 2, 2025
@devondragon devondragon self-assigned this Apr 2, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements a new admin endpoint to toggle the lock status of a user account. Key changes include adding a new LockAccountDto and related test support classes, integrating the lock toggle functionality into UserService and AdminApi, and updating security configuration to include a custom authentication failure handler.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/test/java/com/digitalsanctuary/spring/user/api/provider/holder/ApiTestArgumentsHolder.java Added support for LockAccountDto in API test arguments.
src/test/java/com/digitalsanctuary/spring/user/api/provider/ApiTestLockAccountArgumentsProvider.java Provided new test cases for lock account flow.
src/test/java/com/digitalsanctuary/spring/user/api/data/DataStatus.java Extended DataStatus enum with NOT_FOUND status.
src/test/java/com/digitalsanctuary/spring/user/api/data/ApiTestData.java Added new methods for lock account test responses.
src/test/java/com/digitalsanctuary/spring/user/api/AdminApiTest.java Introduced tests for toggle lock status but with an endpoint mismatch.
src/main/java/com/digitalsanctuary/spring/user/util/ResponseUtil.java Created utility methods for error and success JSON responses.
src/main/java/com/digitalsanctuary/spring/user/service/UserService.java Implemented the toggleLockStatus method for toggling user lock state.
src/main/java/com/digitalsanctuary/spring/user/security/* Updated security configuration and failure handling.
src/main/java/com/digitalsanctuary/spring/user/api/AdminApi.java Added a new admin endpoint for toggling user lock status.

new String[]{"Account Locked"}, null
);
}
public static Response lockAccountFailry() {
Copy link
Preview

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

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

The method name 'lockAccountFailry' appears to be misspelled. Consider renaming it to 'lockAccountFailure' for clarity.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@ParameterizedTest
@ArgumentsSource(ApiTestLockAccountArgumentsProvider.class)
public void toggleLockStatusOfUser(ApiTestArgumentsHolder argumentsHolder) throws Exception {
ResultActions action = perform(MockMvcRequestBuilders.post(URL + "/lock").contentType(MediaType.APPLICATION_FORM_URLENCODED)
Copy link
Preview

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

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

The test is calling the '/lock' endpoint but the implemented endpoint is '/toggleLockStatus'. Update the test to use the correct endpoint.

Suggested change
ResultActions action = perform(MockMvcRequestBuilders.post(URL + "/lock").contentType(MediaType.APPLICATION_FORM_URLENCODED)
ResultActions action = perform(MockMvcRequestBuilders.post(URL + "/toggleLockStatus").contentType(MediaType.APPLICATION_FORM_URLENCODED)

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@devondragon
Copy link
Owner

High level comment: I don't love the lock API or service methods being toggles. I think there should be lockAccount and unlockAccount. I shouldn't have to query the current value, and then call the API to change it, hoping it hasn't changed in the mean time. I should be able to have largely idempotent clear actions.

@UditThakkar
Copy link
Collaborator Author

High level comment: I don't love the lock API or service methods being toggles. I think there should be lockAccount and unlockAccount. I shouldn't have to query the current value, and then call the API to change it, hoping it hasn't changed in the mean time. I should be able to have largely idempotent clear actions.

Thanks for the feedback! That makes sense I actually worked on a project where we had a similar feature with a "Lock Access" toggle in the admin panel. I see the benefit of having separate endpoints for clarity and idempotency. I’ll update the API to include distinct lockAccount and unlockAccount endpoints.

@UditThakkar
Copy link
Collaborator Author

I have updated the code, please help to check @devondragon

@devondragon devondragon requested a review from Copilot April 3, 2025 21:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

new ApiTestArgumentsHolder(
ApiTestData.getEmptyLockAccountDto(),
DataStatus.INVALID,
ApiTestData.invalidBodyLockAccountFailry()
Copy link
Preview

Copilot AI Apr 3, 2025

Choose a reason for hiding this comment

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

Consider correcting the spelling 'Failry' to 'Failure' in the method call 'invalidBodyLockAccountFailry()'.

Suggested change
ApiTestData.invalidBodyLockAccountFailry()
ApiTestData.invalidBodyLockAccountFailure()

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

new String[]{"Account Locked"}, null
);
}
public static Response lockAccountFailry() {
Copy link
Preview

Copilot AI Apr 3, 2025

Choose a reason for hiding this comment

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

Consider correcting the spelling 'Failry' to 'Failure' in 'lockAccountFailry()'.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

new String[]{"User not found"}, null
);
}
public static Response invalidBodyLockAccountFailry() {
Copy link
Preview

Copilot AI Apr 3, 2025

Choose a reason for hiding this comment

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

Consider correcting the spelling 'Failry' to 'Failure' in 'invalidBodyLockAccountFailry()'.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Repository owner deleted a comment from Copilot bot Apr 3, 2025
@devondragon devondragon merged commit 3eb7801 into devondragon:issue-32-Build-Admin-Lock-Account-Flow Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Build Admin Lock Account Flow
2 participants