Skip to content

Fix problem #8085

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.

Sign up for GitHub

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

Open
VasylyshynDmytro wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Fix problem #8085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcf04cd
Fix problem
VasylyshynDmytro Jan 28, 2025
3715ab1
Fix sonar issue
VasylyshynDmytro Feb 6, 2025
7e5cde5
Add tests
VasylyshynDmytro Feb 6, 2025
e16f852
remove not used import
VasylyshynDmytro Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions core/src/main/java/greencity/webcontroller/ManagementUserController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import greencity.annotations.CurrentUser;
import greencity.client.RestClient;
import greencity.constant.ErrorMessage;
import greencity.dto.PageableAdvancedDto;
import greencity.dto.PageableDetailedDto;
import greencity.dto.user.UserFilterDto;
Expand Down Expand Up @@ -43,6 +44,7 @@
import org.springframework.web.bind.annotation.PutMapping;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.bind.annotation.PatchMapping;
import org.springframework.web.client.HttpClientErrorException;
import static greencity.dto.genericresponse.GenericResponseDto.buildGenericResponseDto;

@Validated
Expand All @@ -55,6 +57,7 @@ public class ManagementUserController {
private final UserService userService;
private final HabitAssignService habitAssignService;
private final FilterService filterService;
public static final String MESSAGE_KEY = "message";

/**
* Method that returns management page with all {@link UserVO}.
Expand Down Expand Up @@ -98,9 +101,20 @@ public String getAllUsers(@ModelAttribute UserFilterDto request, @CurrentUser Us
* @author Vasyl Zhovnir
*/
@PostMapping("/register")
public String saveUser(@Valid UserManagementDto userDto) {
restClient.managementRegisterUser(userDto);
return "redirect:/management/users";
public ResponseEntity<Map<String, String>> saveUser(@Valid @RequestBody UserManagementDto userDto) {
try {
restClient.managementRegisterUser(userDto);
return ResponseEntity.ok().body(Map.of(MESSAGE_KEY, "User successfully added!"));
} catch (HttpClientErrorException e) {
if (e.getStatusCode() == HttpStatus.BAD_REQUEST) {
return ResponseEntity.status(HttpStatus.BAD_REQUEST)
.body(Map.of("name", "email", MESSAGE_KEY, ErrorMessage.USER_WITH_EMAIL_EXIST));
}
throw e;
} catch (Exception e) {
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
.body(Map.of(MESSAGE_KEY, HttpStatus.INTERNAL_SERVER_ERROR.toString()));
}
}

/**
Expand Down
37 changes: 31 additions & 6 deletions core/src/main/resources/static/management/user/buttonsAJAX.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,38 @@ $(document).ready(function () {

// Submit button in addUserModal
$('#submitAddBtn').on('click', function (event) {
let form = document.getElementById("addUserForm");
if (form.checkValidity() === false) {
event.preventDefault();
event.stopPropagation();
event.preventDefault();

let form = $('#addUserForm');
if (!form[0].checkValidity()) {
form.addClass("was-validated");
return;
}
form.classList.add("was-validated");
})

$.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);
}
}
});
});
Comment on lines +251 to +274
Copy link

@coderabbitai coderabbitai bot Feb 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security and UX improvements needed in error handling.

The current implementation has several areas for improvement:

  1. Using alert() for error messages is not user-friendly
  2. Error messages are hardcoded in Ukrainian
  3. 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.

Suggested change
$.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);
}


// Edit user button (popup)
$('td .edit.eBtn').on('click', function (event) {
Expand Down
36 changes: 32 additions & 4 deletions core/src/test/java/greencity/webcontroller/ManagementUserControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Sort;
import org.springframework.data.web.PageableHandlerMethodArgumentResolver;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
import org.springframework.web.client.HttpClientErrorException;

import java.util.Collections;
import java.util.List;
import java.util.Map;
Expand All @@ -52,6 +55,7 @@
import static greencity.TestConst.USER_ID;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
Expand Down Expand Up @@ -184,14 +188,38 @@ void deactivateUser() throws Exception {
void saveUserTest() throws Exception {
UserManagementDto dto = ModelUtils.getUserManagementDto();

mockMvc.perform(post(MANAGEMENT_USER_LINK + "/register").contentType(MediaType.APPLICATION_FORM_URLENCODED)
.param("id", dto.getId().toString()).param("name", dto.getName()).param("email", dto.getEmail())
.param("userCredo", dto.getUserCredo()).param("role", dto.getRole().toString())
.param("userStatus", dto.getUserStatus().toString())).andExpect(status().is3xxRedirection());
mockMvc.perform(post(MANAGEMENT_USER_LINK + "/register")
.contentType(MediaType.APPLICATION_JSON)
.content(new ObjectMapper().writeValueAsString(dto)))
.andExpect(status().is2xxSuccessful());

verify(restClient).managementRegisterUser(dto);
}

@Test
void saveUserTest_UserAlreadyExists() throws Exception {
UserManagementDto dto = ModelUtils.getUserManagementDto();

doThrow(new HttpClientErrorException(HttpStatus.BAD_REQUEST))
.when(restClient).managementRegisterUser(dto);
mockMvc.perform(post(MANAGEMENT_USER_LINK + "/register")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(dto)))
.andExpect(status().is4xxClientError());
}

@Test
void saveUserTest_InternalServerError() throws Exception {
UserManagementDto dto = ModelUtils.getUserManagementDto();

doThrow(new RuntimeException()).when(restClient).managementRegisterUser(dto);

mockMvc.perform(post(MANAGEMENT_USER_LINK + "/register")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(dto)))
.andExpect(status().isInternalServerError());
}

@Test
void updateUserTest() throws Exception {
UserManagementDto userManagementDto = ModelUtils.getUserManagementDto();
Expand Down
1 change: 1 addition & 0 deletions service-api/src/main/java/greencity/constant/ErrorMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class ErrorMessage {
public static final String HABIT_HAS_BEEN_ALREADY_ENROLLED = "You can enroll habit only once a day";
public static final String HABIT_ALREADY_ACQUIRED = "You have already acquired habit with id: ";
public static final String HABIT_IS_NOT_ENROLLED_ON_CURRENT_DATE = "Habit is not enrolled on ";
public static final String USER_WITH_EMAIL_EXIST = "User with this email is already registered";
public static final String HABIT_ASSIGN_NOT_FOUND_WITH_CURRENT_USER_ID_AND_HABIT_ID =
"There is no habit assign for current user and such habit with id: ";
public static final String HABIT_ASSIGN_NOT_FOUND_WITH_CURRENT_USER_ID_AND_HABIT_ASSIGN_ID =
Expand Down
Loading