fix: review improvements #34

Author: lcanobbio

backend/src/main/java/ch/puzzle/pcts/controller/RoleController.java

@@ -72,7 +72,7 @@ public ResponseEntity<RoleDto> updateRole(@PathVariable Long id, @RequestBody Ro
             @ApiResponse(responseCode = "204", description = "Role deleted successfully", content = @Content),
             @ApiResponse(responseCode = "404", description = "Role not found", content = @Content) })
     @DeleteMapping("{id}")
-    public ResponseEntity deleteRole(@PathVariable Long id) {
+    public ResponseEntity<Void> deleteRole(@PathVariable Long id) {
         service.delete(id);
         return ResponseEntity.status(204).build();
     }

backend/src/main/java/ch/puzzle/pcts/model/role/Role.java

@@ -9,8 +9,7 @@
 @SQLRestriction("deleted_at IS NULL")
 public class Role {
     @Id
-    @GeneratedValue(strategy = GenerationType.AUTO, generator = "sequence_role")
-    @SequenceGenerator(name = "sequence_role", allocationSize = 1)
+    @GeneratedValue(strategy = GenerationType.IDENTITY)
     private Long id;
 
     private String name;

backend/src/main/java/ch/puzzle/pcts/service/business/RoleBusinessService.java

@@ -44,7 +44,6 @@ public Role update(Long id, Role role) {
 
     public void delete(Long id) {
         validationService.validateOnDelete(id);
-        Role deletedRole = this.getById(id);
         persistenceService.delete(id);
     }
 }

backend/src/main/resources/db/dev-data-migration/afterMigrate__0_0_initialData.sql

@@ -11,12 +11,12 @@ SELECT setval('sequence_example', (SELECT MAX(id) FROM example));
 
 TRUNCATE TABLE role CASCADE;
 
-INSERT INTO role (id, name, deleted_at, is_management)
+INSERT INTO role (name, is_management)
 VALUES
-    (1, 'Administrator', '1970-01-01 00:00:00', TRUE),
-    (2, 'Manager', null, TRUE),
-    (3, 'Team Lead', null, TRUE),
-    (4, 'Developer', null, FALSE),
-    (5, 'Intern', null, FALSE);
-
-SELECT setval('sequence_role', (SELECT MAX(id) FROM role));
+    ('Administrator', TRUE),
+    ('Manager', TRUE),
+    ('Team Lead', TRUE),
+    ('Developer', FALSE),
+    ('Intern', FALSE),
+    ('Extern', FALSE),
+    ('Consultant', FALSE);

backend/src/main/resources/db/migration/V0_0_2__role.sql

@@ -1,8 +1,7 @@
-CREATE SEQUENCE IF NOT EXISTS sequence_role;
 CREATE TABLE IF NOT EXISTS role
 (
-    id              BIGINT NOT NUll PRIMARY KEY,
+    id              BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
     name            TEXT NOT NULL,
-    deleted_at      TIMESTAMP,
+    deleted_at      TIMESTAMP DEFAULT NULL,
     is_management   BOOLEAN NOT NULL
 );

backend/src/test/java/ch/puzzle/pcts/controller/RoleControllerIT.java

@@ -85,7 +85,12 @@ void shouldGetRoleById() throws Exception {
         BDDMockito.given(service.getById(anyLong())).willReturn(role);
         BDDMockito.given(mapper.toDto(any(Role.class))).willReturn(expectedDto);
 
-        mvc.perform(get(BASEURL + "/1").with(SecurityMockMvcRequestPostProcessors.csrf())).andExpect(status().isOk());
+        mvc
+                .perform(get(BASEURL + "/1").with(SecurityMockMvcRequestPostProcessors.csrf()))
+                .andExpect(status().isOk())
+                .andExpect(jsonPath("$.id").isNumber())
+                .andExpect(jsonPath("$.isManagement").value(false))
+                .andExpect(jsonPath("$.name").value("Role 1"));
 
         verify(service, times(1)).getById(eq(1L));
         verify(mapper, times(1)).toDto(any(Role.class));
@@ -139,18 +144,14 @@ void shouldUpdateRole() throws Exception {
     @Test
     void shouldDeleteRole() throws Exception {
         BDDMockito.willDoNothing().given(service).delete(anyLong());
-        BDDMockito.given(mapper.toDto(any(Role.class))).willReturn(expectedDto);
 
         mvc
                 .perform(delete(BASEURL + "/" + id)
                         .contentType(MediaType.APPLICATION_JSON)
                         .with(SecurityMockMvcRequestPostProcessors.csrf()))
                 .andExpect(status().is(204))
-                .andExpect(jsonPath("$.id").isNumber())
-                .andExpect(jsonPath("$.isManagement").value(false))
-                .andExpect(jsonPath("$.name").value("Role 1"));
+                .andExpect(jsonPath("$").doesNotExist());
 
         verify(service, times(1)).delete(any(Long.class));
-        verify(mapper, times(1)).toDto(any(Role.class));
     }
 }

backend/src/test/java/ch/puzzle/pcts/service/business/RoleBusinessServiceTest.java

@@ -1,13 +1,15 @@
 package ch.puzzle.pcts.service.business;
 
-import static org.junit.jupiter.api.Assertions.assertArrayEquals;
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import ch.puzzle.pcts.exception.PCTSException;
+import ch.puzzle.pcts.model.error.ErrorKey;
 import ch.puzzle.pcts.model.role.Role;
 import ch.puzzle.pcts.service.persistence.RolePersistenceService;
 import ch.puzzle.pcts.service.validation.RoleValidationService;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import org.junit.jupiter.api.BeforeEach;
@@ -45,6 +47,18 @@ void shouldGetById() {
         verify(persistenceService).getById(1L);
     }
 
+    @DisplayName("Should throw exception")
+    @Test
+    void shouldThrowException() {
+        when(persistenceService.getById(1L)).thenReturn(Optional.empty());
+
+        PCTSException exception = assertThrows(PCTSException.class, () -> businessService.getById(1L));
+
+        assertEquals("Role with id: " + 1 + " does not exist.", exception.getReason());
+        assertEquals(ErrorKey.NOT_FOUND, exception.getErrorKey());
+        verify(persistenceService).getById(1L);
+    }
+
     @DisplayName("Should get all roles")
     @Test
     void shouldGetAll() {
@@ -58,6 +72,16 @@ void shouldGetAll() {
         verify(persistenceService).getAll();
     }
 
+    @DisplayName("Should get empty list")
+    @Test
+    void shouldGetEmptyList() {
+        when(persistenceService.getAll()).thenReturn(new ArrayList<>());
+
+        List<Role> result = businessService.getAll();
+
+        assertEquals(0, result.size());
+    }
+
     @DisplayName("Should create role")
     @Test
     void shouldCreate() {
@@ -89,13 +113,10 @@ void shouldUpdate() {
     @Test
     void shouldDelete() {
         Long id = 1L;
-        Role role = new Role(id, "Role1", false);
-        when(persistenceService.getById(id)).thenReturn(Optional.of(role));
 
         businessService.delete(id);
 
         verify(validationService).validateOnDelete(id);
-        verify(persistenceService).getById(id);
         verify(persistenceService).delete(id);
     }
 }

backend/src/test/java/ch/puzzle/pcts/service/persistence/RolePersistenceServiceIT.java

@@ -70,11 +70,11 @@ void shouldGetAllRoles() {
     void shouldCreate() {
         Role role = new Role(null, "Role 3", false);
 
-        persistenceService.create(role);
-        Optional<Role> result = persistenceService.getById(3L);
+        Role result = persistenceService.create(role);
 
-        assertThat(result.isPresent()).isTrue();
-        assertThat(role.getId()).isEqualTo(3L);
+        assertThat(result.getId()).isEqualTo(3L);
+        assertThat(result.getName()).isEqualTo(role.getName());
+        assertThat(result.getIsManagement()).isEqualTo(role.getIsManagement());
     }
 
     @DisplayName("Should update role")

backend/src/test/resources/db/test-data-migration/afterMigrate__0_0_initialData.sql

@@ -8,11 +8,10 @@ SELECT setval('sequence_example', (SELECT MAX(id) FROM example));
 
 TRUNCATE TABLE role CASCADE;
 
-INSERT INTO role (id, name, deleted_at, is_management)
+INSERT INTO role (name, deleted_at, is_management)
 VALUES
-    (1, 'Role 1', '1970-01-01 00:00:00', TRUE),
-    (2, 'Role 2', null, false);
+    ('Role 1', '1970-01-01 00:00:00', TRUE),
+    ('Role 2', null,  FALSE);
 
-SELECT setval('sequence_role', (SELECT MAX(id) FROM role));