Fix problem #8085
Fix problem #8085
Conversation
WalkthroughThis pull request updates the user registration process. In the backend, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ManagementUserController
Client->>Controller: POST /saveUser (JSON payload)
alt Registration Successful
Controller-->>Client: 200 OK, { "message": "Success" }
else User Already Exists
Controller-->>Client: 400 Bad Request, { "message": "User with this email is already registered" }
else Other Exception
Controller-->>Client: 500 Internal Server Error, { "message": "Internal error" }
end
sequenceDiagram
participant User
participant UI as buttonsAJAX.js
participant Controller as ManagementUserController
User->>UI: Clicks Submit Button
UI->>UI: Validate Form via jQuery
alt Form Invalid
UI-->>User: Highlight Validation Errors
else Form Valid
UI->>Controller: Send AJAX POST (JSON payload)
Controller-->>UI: Return HTTP Response
UI->>User: Alert with Response Message & Reload Page on Success
end
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
core/src/main/resources/static/management/user/buttonsAJAX.js (1)
507-508: Localize user-facing messages.The alert message is hardcoded in Ukrainian. Use a localization system for better maintainability and internationalization support.
- alert("Максимальна кількість фільтрів 3. Видаліть фільтр для створення нового.") + alert(i18n.t('filters.max_limit_reached'))
🧹 Nitpick comments (3)
core/src/test/java/greencity/webcontroller/ManagementUserControllerTest.java (1)
183-193: Consider adding test for duplicate email scenario.Since the PR adds checks for duplicate emails, it would be valuable to add a test case that verifies this scenario. This ensures the error handling works as expected.
Here's a suggested test method:
@Test void saveUserTestWithDuplicateEmail() throws Exception { UserManagementDto dto = ModelUtils.getUserManagementDto(); when(restClient.managementRegisterUser(dto)) .thenThrow(new RuntimeException("User with this email already exists")); mockMvc.perform(post(MANAGEMENT_USER_LINK + "/register") .contentType(MediaType.APPLICATION_JSON) .content(new ObjectMapper().writeValueAsString(dto))) .andExpect(status().isConflict()) .andExpect(jsonPath("$.message").value("User with this email already exists")); }core/src/main/resources/static/management/user/buttonsAJAX.js (2)
242-249: Form validation looks good, but consider using a form validation library.The form validation implementation is correct, but for better maintainability and consistency, consider using a form validation library like jQuery Validate.
- if (!form[0].checkValidity()) { - form.addClass("was-validated"); - return; - } + form.validate({ + submitHandler: function(form) { + // Your AJAX submission code here + }, + invalidHandler: function(event, validator) { + $(form).addClass("was-validated"); + } + });
186-186: Remove debug console.log statements.Debug statements should not be present in production code.
- console.log(checkedCh)- console.log("some");Also applies to: 499-499
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/greencity/webcontroller/ManagementUserController.java(4 hunks)core/src/main/resources/static/management/user/buttonsAJAX.js(1 hunks)core/src/test/java/greencity/webcontroller/ManagementUserControllerTest.java(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)
🔇 Additional comments (4)
core/src/main/java/greencity/webcontroller/ManagementUserController.java (2)
60-60: LGTM! Good practice using a constant for message key.The constant helps maintain consistency across response structures.
104-114: Excellent error handling implementation!The implementation properly handles:
- Success case with 200 OK
- Duplicate email case with 400 Bad Request
- Unexpected errors with 500 Internal Server Error
Good practices:
- Using proper HTTP status codes
- Structured error responses
- Specific error messages for better user experience
service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
58-58: LGTM! Clear and descriptive error message.The error message is user-friendly and follows the existing pattern in the file.
core/src/test/java/greencity/webcontroller/ManagementUserControllerTest.java (1)
187-190: LGTM! Clean transition to JSON-based API.The changes correctly reflect the shift from form-based to JSON-based API, with proper content type and response status expectations.
| $.ajax({ | ||
| url: form.attr('action'), | ||
| type: 'POST', | ||
| contentType: 'application/json', | ||
| data: JSON.stringify(Object.fromEntries(new FormData(form[0]))), | ||
| success: function (response) { | ||
| alert(response.message); | ||
| window.location.reload(); | ||
| }, | ||
| error: function (xhr) { | ||
| if (xhr.status === 400) { | ||
| try { | ||
| const response = JSON.parse(xhr.responseText); | ||
| let errorMessage = response.find(e => e.name === "email")?.message || "Невідома помилка."; | ||
| alert("Помилка: " + errorMessage); | ||
| } catch (e) { | ||
| alert("Не вдалося розібрати відповідь сервера."); | ||
| } | ||
| } else { | ||
| alert("Помилка сервера. Статус: " + xhr.status); | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Security and UX improvements needed in error handling.
The current implementation has several areas for improvement:
- Using
alert()for error messages is not user-friendly - Error messages are hardcoded in Ukrainian
- Direct exposure of server error messages could leak sensitive information
Consider this improved implementation:
$.ajax({
url: form.attr('action'),
type: 'POST',
contentType: 'application/json',
data: JSON.stringify(Object.fromEntries(new FormData(form[0]))),
success: function (response) {
- alert(response.message);
+ showToast('success', response.message);
window.location.reload();
},
error: function (xhr) {
if (xhr.status === 400) {
try {
const response = JSON.parse(xhr.responseText);
- let errorMessage = response.find(e => e.name === "email")?.message || "Невідома помилка.";
- alert("Помилка: " + errorMessage);
+ let errorMessage = response.find(e => e.name === "email")?.message || i18n.t('errors.unknown');
+ showToast('error', i18n.t('errors.prefix') + errorMessage);
} catch (e) {
- alert("Не вдалося розібрати відповідь сервера.");
+ showToast('error', i18n.t('errors.parse_failed'));
}
} else {
- alert("Помилка сервера. Статус: " + xhr.status);
+ showToast('error', i18n.t('errors.server', { status: xhr.status }));
}
}
});
+ function showToast(type, message) {
+ // Implementation using a toast library like toastr
+ toastr[type](message);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $.ajax({ | |
| url: form.attr('action'), | |
| type: 'POST', | |
| contentType: 'application/json', | |
| data: JSON.stringify(Object.fromEntries(new FormData(form[0]))), | |
| success: function (response) { | |
| alert(response.message); | |
| window.location.reload(); | |
| }, | |
| error: function (xhr) { | |
| if (xhr.status === 400) { | |
| try { | |
| const response = JSON.parse(xhr.responseText); | |
| let errorMessage = response.find(e => e.name === "email")?.message || "Невідома помилка."; | |
| alert("Помилка: " + errorMessage); | |
| } catch (e) { | |
| alert("Не вдалося розібрати відповідь сервера."); | |
| } | |
| } else { | |
| alert("Помилка сервера. Статус: " + xhr.status); | |
| } | |
| } | |
| }); | |
| }); | |
| $.ajax({ | |
| url: form.attr('action'), | |
| type: 'POST', | |
| contentType: 'application/json', | |
| data: JSON.stringify(Object.fromEntries(new FormData(form[0]))), | |
| success: function (response) { | |
| showToast('success', response.message); | |
| window.location.reload(); | |
| }, | |
| error: function (xhr) { | |
| if (xhr.status === 400) { | |
| try { | |
| const response = JSON.parse(xhr.responseText); | |
| let errorMessage = response.find(e => e.name === "email")?.message || i18n.t('errors.unknown'); | |
| showToast('error', i18n.t('errors.prefix') + errorMessage); | |
| } catch (e) { | |
| showToast('error', i18n.t('errors.parse_failed')); | |
| } | |
| } else { | |
| showToast('error', i18n.t('errors.server', { status: xhr.status })); | |
| } | |
| } | |
| }); | |
| }); | |
| function showToast(type, message) { | |
| // Implementation using a toast library like toastr | |
| toastr[type](message); | |
| } |
|
Please increase your code coverage |
|
Add checks if user with this email already exist
Summary by CodeRabbit
New Features
Bug Fixes