From 7717b407d315512ebaed68d311d3bebd9f98392e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 5 Apr 2023 15:41:33 +0100 Subject: [PATCH 01/24] Add role repository --- .../pt/up/fe/ni/website/backend/repository/RoleRepository.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt index 1898b9f7..c59bdf7e 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt @@ -5,4 +5,6 @@ import org.springframework.stereotype.Repository import pt.up.fe.ni.website.backend.model.Role @Repository -interface RoleRepository : CrudRepository +interface RoleRepository : CrudRepository { + fun findByName(name: String): Role? +} From 7773353f176aa745c980787ba087a14da22db3d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 5 Apr 2023 15:42:48 +0100 Subject: [PATCH 02/24] Add RoleService and logic related to role modification --- .../website/backend/service/ErrorMessages.kt | 8 ++ .../ni/website/backend/service/RoleService.kt | 102 +++++++++++++++--- 2 files changed, 98 insertions(+), 12 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt index 290b1bd8..ab5da93c 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt @@ -17,6 +17,8 @@ object ErrorMessages { const val generationAlreadyExists = "generation already exists" + const val roleAlreadyExists = "role already exists" + fun postNotFound(postId: Long): String = "post not found with id $postId" fun postNotFound(postSlug: String): String = "post not found with slug $postSlug" @@ -38,4 +40,10 @@ object ErrorMessages { fun generationNotFound(year: String): String = "generation not found with year $year" fun emailNotFound(email: String): String = "account not found with email $email" + + fun roleNotFound(id: Long): String = "role not found with id $id" + + fun userAlreadyHasRole(roleId: Long, userId: Long): String = "user $userId already has role $roleId" + + fun userNotInRole(roleId: Long, userId: Long): String = "user $userId doesn't have role $roleId" } diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index bf74ab31..12832144 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -1,46 +1,124 @@ package pt.up.fe.ni.website.backend.service +import jakarta.transaction.Transactional +import org.springframework.data.repository.findByIdOrNull import org.springframework.stereotype.Service +import pt.up.fe.ni.website.backend.dto.entity.RoleDto import pt.up.fe.ni.website.backend.model.Activity import pt.up.fe.ni.website.backend.model.PerActivityRole import pt.up.fe.ni.website.backend.model.Role -import pt.up.fe.ni.website.backend.model.permissions.Permission import pt.up.fe.ni.website.backend.model.permissions.Permissions +import pt.up.fe.ni.website.backend.repository.AccountRepository +import pt.up.fe.ni.website.backend.repository.ActivityRepository +import pt.up.fe.ni.website.backend.repository.GenerationRepository import pt.up.fe.ni.website.backend.repository.PerActivityRoleRepository import pt.up.fe.ni.website.backend.repository.RoleRepository @Service +@Transactional class RoleService( private val roleRepository: RoleRepository, - private val perActivityRoleRepository: PerActivityRoleRepository + private val perActivityRoleRepository: PerActivityRoleRepository, + private val generationRepository: GenerationRepository, + private val accountRepository: AccountRepository, + private val activityRepository: ActivityRepository ) { - fun grantPermissionToRole(role: Role, permission: Permission) { - role.permissions.add(permission) + fun getRole(roleId: Long): Role { + val role = roleRepository.findById(roleId).orElseThrow { + throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + } + return role + } + fun getAllRoles(): List = roleRepository.findAll().toList() + fun grantPermissionToRole(roleId: Long, permissions: Permissions) { + val role = roleRepository.findById(roleId).orElseThrow { + throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + } + role.permissions.addAll(permissions) roleRepository.save(role) } - fun revokePermissionFromRole(role: Role, permission: Permission) { - role.permissions.remove(permission) + fun revokePermissionFromRole(roleId: Long, permissions: Permissions) { + val role = roleRepository.findById(roleId).orElseThrow { + throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + } + role.permissions.removeAll(permissions) roleRepository.save(role) } - fun grantPermissionToRoleOnActivity(role: Role, activity: Activity, permission: Permission) { + fun grantPermissionToRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { + val activity = activityRepository.findById(activityId).orElseThrow { + throw NoSuchElementException(ErrorMessages.activityNotFound(activityId)) + } + val role = roleRepository.findById(roleId).orElseThrow { + throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + } val foundActivity = activity.associatedRoles .find { it.activity == activity } ?: PerActivityRole(Permissions()) foundActivity.role = role foundActivity.activity = activity - foundActivity.role = role - foundActivity.permissions.add(permission) + foundActivity.permissions.addAll(permissions) perActivityRoleRepository.save(foundActivity) } - fun revokePermissionFromRoleOnActivity(role: Role, activity: Activity, permission: Permission) { + fun revokePermissionFromRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { + val activity = activityRepository.findById(activityId).orElseThrow { + throw NoSuchElementException(ErrorMessages.activityNotFound(activityId)) + } + val role = roleRepository.findById(roleId).orElseThrow { + throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + } val foundActivity = activity.associatedRoles .find { it.role == role } ?: return - - foundActivity.permissions.remove(permission) + foundActivity.permissions.removeAll(permissions) perActivityRoleRepository.save(foundActivity) } + + fun createNewRole(dto: RoleDto): Role { + if (roleRepository.findByName(dto.name) != null) { + throw IllegalArgumentException(ErrorMessages.roleAlreadyExists) + } + val role = dto.create() + val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc() + ?: throw IllegalArgumentException(ErrorMessages.noGenerations) + roleRepository.save(role) + latestGeneration.roles.add(role) + generationRepository.save(latestGeneration) + return role + } + + fun deleteRole(roleId: Long) { + val role = roleRepository.findByIdOrNull(roleId) + ?: throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc()!! + latestGeneration.roles.remove(role) + generationRepository.save(latestGeneration) + roleRepository.deleteById(roleId) + } + + fun addUserToRole(roleId: Long, userId: Long) { + val role = roleRepository.findByIdOrNull(roleId) + ?: throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + val account = accountRepository.findByIdOrNull(userId) + ?: throw NoSuchElementException(ErrorMessages.accountNotFound(userId)) + role.accounts.find { it.id == account.id }.let { + if (it != null) throw NoSuchElementException(ErrorMessages.userAlreadyHasRole(roleId, userId)) + } + role.accounts.add(account) + roleRepository.save(role) + } + + fun removeUserFromRole(roleId: Long, userId: Long) { + val role = roleRepository.findByIdOrNull(roleId) + ?: throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + val account = accountRepository.findByIdOrNull(userId) + ?: throw NoSuchElementException(ErrorMessages.accountNotFound(userId)) + role.accounts.find { it.id == account.id }.let { + if (it == null) throw NoSuchElementException(ErrorMessages.userNotInRole(roleId, userId)) + } + role.accounts.remove(account) + roleRepository.save(role) + } } From 5e0a429a4db8429dc580e483cc1d1852371e162a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 5 Apr 2023 15:43:06 +0100 Subject: [PATCH 03/24] Add role controller and tests --- .../backend/controller/RoleController.kt | 75 ++++ .../backend/controller/RoleControllerTest.kt | 400 ++++++++++++++++++ 2 files changed, 475 insertions(+) create mode 100644 src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt create mode 100644 src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt new file mode 100644 index 00000000..1cefbabe --- /dev/null +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -0,0 +1,75 @@ +package pt.up.fe.ni.website.backend.controller + +import org.springframework.web.bind.annotation.DeleteMapping +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.PathVariable +import org.springframework.web.bind.annotation.PostMapping +import org.springframework.web.bind.annotation.RequestBody +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.RestController +import pt.up.fe.ni.website.backend.dto.entity.RoleDto +import pt.up.fe.ni.website.backend.model.permissions.Permissions +import pt.up.fe.ni.website.backend.service.RoleService + +@RestController +@RequestMapping("/roles") +class RoleController(private val roleService: RoleService) { + @GetMapping + fun getAllRoles() = roleService.getAllRoles() + + @GetMapping("/{id}") + fun getRole(@PathVariable id: Long) = roleService.getRole(id) + + @PostMapping("/new") + fun createNewRole(@RequestBody dto: RoleDto) = roleService.createNewRole(dto) + + @DeleteMapping("/{id}") + fun deleteRole(@PathVariable id: Long): Map { + roleService.deleteRole(id) + return emptyMap() + } + + @PostMapping("/{id}/grant") + fun grantPermissionRole(@PathVariable id: Long, @RequestBody permissions: Permissions): Map { + roleService.grantPermissionToRole(id, permissions) + return emptyMap() + } + + @PostMapping("/{id}/revoke") + fun revokePermissionRole(@PathVariable id: Long, @RequestBody permissions: Permissions): Map { + roleService.revokePermissionFromRole(id, permissions) + return emptyMap() + } + + @PostMapping("/{id}/users") + fun addUserToRole(@PathVariable id: Long, @RequestBody userId: Long): Map { + roleService.addUserToRole(id, userId) + return emptyMap() + } + + @DeleteMapping("/{id}/users") + fun removeUserFromRole(@PathVariable id: Long, @RequestBody userId: Long): Map { + roleService.removeUserFromRole(id, userId) + return emptyMap() + } + + @PostMapping("/{id}/activities") + fun addPermissionToPerActivityRole( + @PathVariable id: Long, + @RequestBody activityId: Long, + @RequestBody permissions: Permissions + ): Map { + roleService.grantPermissionToRoleOnActivity(id, activityId, permissions) + return emptyMap() + } + + @DeleteMapping("/{id}/activities") + fun revokePermissionToPerActivityRole( + @PathVariable id: Long, + @RequestBody activityId: Long, + @RequestBody permissions: Permissions + ): Map { + roleService.revokePermissionFromRoleOnActivity(id, activityId, permissions) + return emptyMap() + } +} diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt new file mode 100644 index 00000000..4917c116 --- /dev/null +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -0,0 +1,400 @@ +package pt.up.fe.ni.website.backend.controller + +import com.fasterxml.jackson.databind.ObjectMapper +import jakarta.transaction.Transactional +import java.util.* +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Test +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.data.repository.findByIdOrNull +import org.springframework.http.MediaType +import org.springframework.test.web.servlet.MockMvc +import org.springframework.test.web.servlet.delete +import org.springframework.test.web.servlet.get +import org.springframework.test.web.servlet.post +import pt.up.fe.ni.website.backend.model.Account +import pt.up.fe.ni.website.backend.model.CustomWebsite +import pt.up.fe.ni.website.backend.model.Generation +import pt.up.fe.ni.website.backend.model.Role +import pt.up.fe.ni.website.backend.model.permissions.Permission +import pt.up.fe.ni.website.backend.model.permissions.Permissions +import pt.up.fe.ni.website.backend.repository.AccountRepository +import pt.up.fe.ni.website.backend.repository.GenerationRepository +import pt.up.fe.ni.website.backend.repository.RoleRepository +import pt.up.fe.ni.website.backend.utils.TestUtils +import pt.up.fe.ni.website.backend.utils.annotations.ControllerTest +import pt.up.fe.ni.website.backend.utils.annotations.NestedTest + +@ControllerTest +@Transactional +internal class RoleControllerTest @Autowired constructor( + val mockMvc: MockMvc, + val objectMapper: ObjectMapper, + val roleRepository: RoleRepository, + val accountRepository: AccountRepository, + val generationRepository: GenerationRepository +) { + val testGeneration = Generation( + "22-23" + ) + + val testRole = Role( + "TestRole", + Permissions(listOf(Permission.CREATE_ACTIVITY)), + false + ) + val otherTestRole = Role( + "Coordenador de Projetos", + Permissions(listOf(Permission.EDIT_ACCOUNT)), + false + ) + + val testAccount = Account( + "TestAccount", + "something@pog.com", + "test_password", + "This is a test account", + TestUtils.createDate(2003, Calendar.NOVEMBER, 13), + "https://test-photo.com", + "https://linkedin.com", + "https://github.com", + listOf( + CustomWebsite("https://test-website.com", "https://test-website.com/logo.png") + ), + mutableListOf() + ) + + @NestedTest + @DisplayName("GET /roles") + inner class GetAllRoles { + + val roles = listOf( + testRole, + Role( + "El Presidant", + Permissions(listOf(Permission.SUPERUSER)), + true + ) + ) + + @BeforeEach + fun addRoles() { + for (role in roles) { + roleRepository.save(role) + } + } + + @Test + fun `should return all roles`() { + mockMvc.get("/roles").andExpect { + status { isOk() } + content { contentType(MediaType.APPLICATION_JSON) } + content { json(objectMapper.writeValueAsString(roles)) } + } + } + } + + @NestedTest + @DisplayName("GET /roles/{id}") + inner class GetSpecificRole { + @BeforeEach + fun addRole() { + roleRepository.save(testRole) + } + + @Test + fun `should return testRole when provided by its id`() { + mockMvc.get("/roles/${testRole.id}").andExpect { + status { isOk() } + content { contentType(MediaType.APPLICATION_JSON) } + content { json(objectMapper.writeValueAsString(testRole)) } + } + } + + @Test + fun `should return error on invalid roleID`() { + mockMvc.get("/roles/4020").andExpect { + status { isNotFound() } + content { contentType(MediaType.APPLICATION_JSON) } + jsonPath("$.errors.length()") { value(1) } + jsonPath("$.errors.[0].message") { value("role not found with id 4020") } + } + } + } + + @NestedTest + inner class CreateNewRole { + + @BeforeEach + fun addRole() { + generationRepository.save(testGeneration) + roleRepository.save(testRole) + } + + @Test + fun `should add new role`() { + mockMvc.post("/roles/new") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString( + mapOf( + "name" to otherTestRole.name, + "permissions" to otherTestRole.permissions.map { it.bit }, + "isSection" to otherTestRole.isSection, + "associatedActivities" to otherTestRole.associatedActivities, + "accounts" to otherTestRole.accounts + ) + ) + }.andExpect { + status { isOk() } + content { contentType(MediaType.APPLICATION_JSON) } + jsonPath("$.name") { value(otherTestRole.name) } + jsonPath("$.id") { value(2) } // this must be hardcoded + jsonPath("$.permissions.length()") { value(otherTestRole.permissions.size) } + jsonPath("$.isSection") { value(otherTestRole.isSection) } + jsonPath("$.associatedActivities") { value(otherTestRole.associatedActivities) } + } + assert(roleRepository.existsById(2)) + assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 1) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles[0].id!!.compareTo(2) == 0) + } + + @Test + fun `shouldn't add role with same name`() { + mockMvc.post("/roles/new") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString( + mapOf( + "name" to testRole.name, + "permissions" to testRole.permissions.map { it.bit }, + "isSection" to testRole.isSection, + "associatedActivities" to testRole.associatedActivities, + "accounts" to testRole.accounts + ) + ) + }.andExpect { + status { isUnprocessableEntity() } + content { contentType(MediaType.APPLICATION_JSON) } + jsonPath("$.errors.length()") { value(1) } + jsonPath("$.errors.[0].message") { value("role already exists") } + } + assert(roleRepository.findByName(testRole.name) != null) + assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 1) + } + } + + @NestedTest + inner class DeleteRole { + @BeforeEach + fun addRole() { + roleRepository.save(testRole) + generationRepository.save(testGeneration) + generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.add(testRole) + } + + @Test + fun `should remove role with correct id`() { + mockMvc.delete("/roles/${testRole.id}").andExpect { + status { isOk() } + } + assert(roleRepository.findByName(testRole.name) == null) + assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 0) + } + + @Test + fun `should not remove role id that does not exist`() { + mockMvc.delete("/roles/1234").andExpect { + status { isNotFound() } + content { contentType(MediaType.APPLICATION_JSON) } + jsonPath("$.errors.length()") { value(1) } + jsonPath("$.errors.[0].message") { value("role not found with id 1234") } + } + } + } + + @NestedTest + inner class GrantPermissionRole { + + @BeforeEach + fun addRole() { + roleRepository.save(testRole) + } + + @Test + fun `should grant permission to role that exists`() { + mockMvc.post("/roles/${testRole.id}/grant") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString( + Permissions(listOf(Permission.SUPERUSER)) + ) + }.andExpect { + status { isOk() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) + } + + @Test + fun `should grant permission to role that does not exists`() { + mockMvc.post("/roles/1234/grant") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString( + Permissions(listOf(Permission.SUPERUSER)) + ) + }.andExpect { + status { isNotFound() } + } + } + } + + @NestedTest + inner class RevokePermissionRole { + + @BeforeEach + fun addRole() { + roleRepository.save(testRole) + } + + @Test + fun `should revoke permission to role that exists`() { + mockMvc.post("/roles/${testRole.id}/revoke") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString( + Permissions(listOf(Permission.SUPERUSER)) + ) + }.andExpect { + status { isOk() } + } + assert(!roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) + } + + @Test + fun `should revoke permission to role that does not exists`() { + mockMvc.post("/roles/1234/revoke") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString( + Permissions(listOf(Permission.SUPERUSER)) + ) + }.andExpect { + status { isNotFound() } + } + } + } + + @NestedTest + inner class AddUserToRole { + @BeforeEach + fun addRoleAndUser() { + roleRepository.save(testRole) + accountRepository.save(testAccount) + } + + @Test + fun `should add an existing account to an existing role`() { + mockMvc.post("/roles/${testRole.id}/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(testAccount.id) + }.andExpect { + status { isOk() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) + } + + @Test + fun `should not add an non existing account to an existing role`() { + mockMvc.post("/roles/${testRole.id}/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(1234) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) + } + + @Test + fun `should not add an non existing account to an non existing role`() { + mockMvc.post("/roles/1234/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(1234) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) + } + + @Test + fun `should not add an existing account to an non existing role`() { + mockMvc.post("/roles/1234/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(testAccount.id) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) + } + } + + @NestedTest + inner class RemoveUserFromRole { + @BeforeEach + fun addRoleAndUser() { + accountRepository.save(testAccount) + roleRepository.save(testRole) + testRole.accounts.add(testAccount) + roleRepository.save(testRole) + } + + @AfterEach + fun removeRolesAndUser() { + testRole.accounts.remove(testAccount) + roleRepository.save(testRole) + } + + @Test + fun `should remove an existing account to an existing role`() { + mockMvc.delete("/roles/${testRole.id}/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(testAccount.id) + }.andExpect { + status { isOk() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) + } + + @Test + fun `should not remove an non existing account to an existing role`() { + mockMvc.delete("/roles/${testRole.id}/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(1234) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) + } + + @Test + fun `should not remove an non existing account to an non existing role`() { + mockMvc.delete("/roles/1234/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(1234) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) + } + + @Test + fun `should not remove an existing account to an non existing role`() { + mockMvc.delete("/roles/1234/users") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(testAccount.id) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) + } + } +} From c1cd18256ef5dd3bd6368a19a5cd44c5912e1ffa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 12 Apr 2023 13:56:34 +0100 Subject: [PATCH 04/24] Add more tests and refactor some endpoints --- .../backend/controller/RoleController.kt | 8 +- .../ni/website/backend/service/RoleService.kt | 7 + .../backend/controller/RoleControllerTest.kt | 137 +++++++++++++++++- 3 files changed, 147 insertions(+), 5 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 1cefbabe..650aa325 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -53,20 +53,20 @@ class RoleController(private val roleService: RoleService) { return emptyMap() } - @PostMapping("/{id}/activities") + @PostMapping("/{id}/activities/{activityId}/permissions") fun addPermissionToPerActivityRole( @PathVariable id: Long, - @RequestBody activityId: Long, + @PathVariable activityId: Long, @RequestBody permissions: Permissions ): Map { roleService.grantPermissionToRoleOnActivity(id, activityId, permissions) return emptyMap() } - @DeleteMapping("/{id}/activities") + @DeleteMapping("/{id}/activities/{activityId}/permissions") fun revokePermissionToPerActivityRole( @PathVariable id: Long, - @RequestBody activityId: Long, + @PathVariable activityId: Long, @RequestBody permissions: Permissions ): Map { roleService.revokePermissionFromRoleOnActivity(id, activityId, permissions) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 12832144..58fe5496 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -61,6 +61,11 @@ class RoleService( foundActivity.permissions.addAll(permissions) perActivityRoleRepository.save(foundActivity) + if (activity.associatedRoles.find { it.activity == activity } == null) { + role.associatedActivities.add(foundActivity) + } + activityRepository.save(activity) + roleRepository.save(role) } fun revokePermissionFromRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { @@ -74,6 +79,8 @@ class RoleService( .find { it.role == role } ?: return foundActivity.permissions.removeAll(permissions) perActivityRoleRepository.save(foundActivity) + activityRepository.save(activity) + } fun createNewRole(dto: RoleDto): Role { diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 4917c116..b7c27242 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -17,11 +17,14 @@ import org.springframework.test.web.servlet.post import pt.up.fe.ni.website.backend.model.Account import pt.up.fe.ni.website.backend.model.CustomWebsite import pt.up.fe.ni.website.backend.model.Generation +import pt.up.fe.ni.website.backend.model.PerActivityRole +import pt.up.fe.ni.website.backend.model.Project import pt.up.fe.ni.website.backend.model.Role import pt.up.fe.ni.website.backend.model.permissions.Permission import pt.up.fe.ni.website.backend.model.permissions.Permissions import pt.up.fe.ni.website.backend.repository.AccountRepository import pt.up.fe.ni.website.backend.repository.GenerationRepository +import pt.up.fe.ni.website.backend.repository.ProjectRepository import pt.up.fe.ni.website.backend.repository.RoleRepository import pt.up.fe.ni.website.backend.utils.TestUtils import pt.up.fe.ni.website.backend.utils.annotations.ControllerTest @@ -34,7 +37,8 @@ internal class RoleControllerTest @Autowired constructor( val objectMapper: ObjectMapper, val roleRepository: RoleRepository, val accountRepository: AccountRepository, - val generationRepository: GenerationRepository + val generationRepository: GenerationRepository, + val projectRepository: ProjectRepository ) { val testGeneration = Generation( "22-23" @@ -51,6 +55,11 @@ internal class RoleControllerTest @Autowired constructor( false ) + val testProject = Project( + "UNI", + "Melhor app" + ) + val testAccount = Account( "TestAccount", "something@pog.com", @@ -397,4 +406,130 @@ internal class RoleControllerTest @Autowired constructor( assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } } + + @NestedTest + inner class AddPermissionToRoleActivity{ + @BeforeEach + fun addAll(){ + roleRepository.save(testRole) + projectRepository.save(testProject) + } + + @Test + fun `should add permission to role activity and create PerActivityRole`(){ + mockMvc.post("/roles/${testRole.id}/activities/${testProject.id}/permissions"){ + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isOk() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 1) + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities[0].permissions.contains(Permission.EDIT_ACTIVITY)) + } + @Test + fun `shouldn't add permission to role activity if roleId is invalid`(){ + mockMvc.post("/roles/1234/activities/${testProject.id}/permissions"){ + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) + } + + @Test + fun `shouldn't add permission to role activity if activityId is invalid`(){ + mockMvc.post("/roles/${testRole.id}/activities/1234/permissions"){ + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) + } + + @Test + fun `shouldn't add permission to role activity if activityId is invalid and roleId is invalid`(){ + mockMvc.post("/roles/${testRole.id}/activities/1234/permissions"){ + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isNotFound() } + } + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) + } + + + } + + @NestedTest + inner class RemovePermissionFromPerRoleActivity { + @BeforeEach + fun addAll() { + projectRepository.save(testProject) + roleRepository.save(testRole) + var perActivityRole = PerActivityRole(Permissions(listOf(Permission.EDIT_ACTIVITY))) + perActivityRole.activity = testProject + perActivityRole.role = testRole + testProject.associatedRoles.add(perActivityRole) + projectRepository.save(testProject) + roleRepository.save(testRole) + } + + @AfterEach + fun removeAll() { + testProject.associatedRoles.removeAt(0) + projectRepository.save(testProject) + } + + @Test + fun `should remove an existing role activity permission`() { + mockMvc.delete("/roles/${testRole.id}/activities/${testProject.id}/permissions") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isOk() } + } + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + assert(!projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) + + } + + @Test + fun `should not remove an existing role activity permission on a non existing role`() { + mockMvc.delete("/roles/1234/activities/${testProject.id}/permissions") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isNotFound() } + } + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) + } + @Test + fun `should not remove an existing role activity permission on a non existing activity`() { + mockMvc.delete("/roles/${testRole.id}/activities/1234/permissions") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isNotFound() } + } + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) + + } + + @Test + fun `should not remove an existing role activity permission on a non existing activity and non existing role`() { + mockMvc.delete("/roles/1234/activities/1234/permissions") { + contentType = MediaType.APPLICATION_JSON + content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + }.andExpect { + status { isNotFound() } + } + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) + + } + } } From 4840c1b7318b6ae66054413d9db000dd671d07ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 12 Apr 2023 16:33:21 +0100 Subject: [PATCH 05/24] Change endpoints and create permissionsDto to keep API more semantic --- .../backend/controller/RoleController.kt | 27 +++++--- .../backend/dto/permissions/PermissionsDto.kt | 8 +++ .../backend/controller/RoleControllerTest.kt | 61 ++++++++++--------- 3 files changed, 58 insertions(+), 38 deletions(-) create mode 100644 src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 650aa325..874e16e6 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -8,6 +8,8 @@ import org.springframework.web.bind.annotation.RequestBody import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RestController import pt.up.fe.ni.website.backend.dto.entity.RoleDto +import pt.up.fe.ni.website.backend.dto.permissions.PermissionsDto +import pt.up.fe.ni.website.backend.model.permissions.Permission import pt.up.fe.ni.website.backend.model.permissions.Permissions import pt.up.fe.ni.website.backend.service.RoleService @@ -30,14 +32,17 @@ class RoleController(private val roleService: RoleService) { } @PostMapping("/{id}/grant") - fun grantPermissionRole(@PathVariable id: Long, @RequestBody permissions: Permissions): Map { - roleService.grantPermissionToRole(id, permissions) + fun grantPermissionRole(@PathVariable id: Long, + @RequestBody permissionsDto: PermissionsDto + ): Map { + roleService.grantPermissionToRole(id, Permissions(permissionsDto.permissions)) return emptyMap() } @PostMapping("/{id}/revoke") - fun revokePermissionRole(@PathVariable id: Long, @RequestBody permissions: Permissions): Map { - roleService.revokePermissionFromRole(id, permissions) + fun revokePermissionRole(@PathVariable id: Long, @RequestBody permissionsDto: PermissionsDto + ): Map { + roleService.revokePermissionFromRole(id, Permissions(permissionsDto.permissions)) return emptyMap() } @@ -53,23 +58,25 @@ class RoleController(private val roleService: RoleService) { return emptyMap() } - @PostMapping("/{id}/activities/{activityId}/permissions") + @PostMapping("/{id}/activity/{activityId}/permissions") fun addPermissionToPerActivityRole( @PathVariable id: Long, @PathVariable activityId: Long, - @RequestBody permissions: Permissions + @RequestBody permissionsDto: PermissionsDto ): Map { - roleService.grantPermissionToRoleOnActivity(id, activityId, permissions) + roleService.grantPermissionToRoleOnActivity(id, activityId, + Permissions(permissionsDto.permissions)) return emptyMap() } - @DeleteMapping("/{id}/activities/{activityId}/permissions") + @DeleteMapping("/{id}/activity/{activityId}/permissions") fun revokePermissionToPerActivityRole( @PathVariable id: Long, @PathVariable activityId: Long, - @RequestBody permissions: Permissions + @RequestBody permissionsDto: PermissionsDto ): Map { - roleService.revokePermissionFromRoleOnActivity(id, activityId, permissions) + roleService.revokePermissionFromRoleOnActivity(id, activityId, + Permissions(permissionsDto.permissions)) return emptyMap() } } diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt new file mode 100644 index 00000000..dd73046e --- /dev/null +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt @@ -0,0 +1,8 @@ +package pt.up.fe.ni.website.backend.dto.permissions + +import pt.up.fe.ni.website.backend.model.permissions.Permissions + + +data class PermissionsDto( + val permissions : Permissions +) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index b7c27242..81dee265 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -237,9 +237,9 @@ internal class RoleControllerTest @Autowired constructor( fun `should grant permission to role that exists`() { mockMvc.post("/roles/${testRole.id}/grant") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString( - Permissions(listOf(Permission.SUPERUSER)) - ) + content = objectMapper.writeValueAsString(mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + )) }.andExpect { status { isOk() } } @@ -250,9 +250,9 @@ internal class RoleControllerTest @Autowired constructor( fun `should grant permission to role that does not exists`() { mockMvc.post("/roles/1234/grant") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString( - Permissions(listOf(Permission.SUPERUSER)) - ) + content = objectMapper.writeValueAsString(mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + )) }.andExpect { status { isNotFound() } } @@ -271,9 +271,9 @@ internal class RoleControllerTest @Autowired constructor( fun `should revoke permission to role that exists`() { mockMvc.post("/roles/${testRole.id}/revoke") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString( - Permissions(listOf(Permission.SUPERUSER)) - ) + content = objectMapper.writeValueAsString(mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + )) }.andExpect { status { isOk() } } @@ -284,9 +284,9 @@ internal class RoleControllerTest @Autowired constructor( fun `should revoke permission to role that does not exists`() { mockMvc.post("/roles/1234/revoke") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString( - Permissions(listOf(Permission.SUPERUSER)) - ) + content = objectMapper.writeValueAsString(mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + )) }.andExpect { status { isNotFound() } } @@ -417,9 +417,9 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should add permission to role activity and create PerActivityRole`(){ - mockMvc.post("/roles/${testRole.id}/activities/${testProject.id}/permissions"){ + mockMvc.post("/roles/${testRole.id}/activity/${testProject.id}/permissions"){ contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) }.andExpect { status { isOk() } } @@ -428,9 +428,9 @@ internal class RoleControllerTest @Autowired constructor( } @Test fun `shouldn't add permission to role activity if roleId is invalid`(){ - mockMvc.post("/roles/1234/activities/${testProject.id}/permissions"){ + mockMvc.post("/roles/1234/activity/${testProject.id}/permissions"){ contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) }.andExpect { status { isNotFound() } } @@ -439,9 +439,9 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if activityId is invalid`(){ - mockMvc.post("/roles/${testRole.id}/activities/1234/permissions"){ + mockMvc.post("/roles/${testRole.id}/activity/1234/permissions"){ contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) }.andExpect { status { isNotFound() } } @@ -450,9 +450,10 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if activityId is invalid and roleId is invalid`(){ - mockMvc.post("/roles/${testRole.id}/activities/1234/permissions"){ + mockMvc.post("/roles/${testRole.id}/activity/1234/permissions"){ contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + }.andExpect { status { isNotFound() } } @@ -484,9 +485,10 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove an existing role activity permission`() { - mockMvc.delete("/roles/${testRole.id}/activities/${testProject.id}/permissions") { + mockMvc.delete("/roles/${testRole.id}/activity/${testProject.id}/permissions") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + }.andExpect { status { isOk() } } @@ -497,9 +499,10 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing role`() { - mockMvc.delete("/roles/1234/activities/${testProject.id}/permissions") { + mockMvc.delete("/roles/1234/activity/${testProject.id}/permissions") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + }.andExpect { status { isNotFound() } } @@ -508,9 +511,10 @@ internal class RoleControllerTest @Autowired constructor( } @Test fun `should not remove an existing role activity permission on a non existing activity`() { - mockMvc.delete("/roles/${testRole.id}/activities/1234/permissions") { + mockMvc.delete("/roles/${testRole.id}/activity/1234/permissions") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + }.andExpect { status { isNotFound() } } @@ -521,9 +525,10 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing activity and non existing role`() { - mockMvc.delete("/roles/1234/activities/1234/permissions") { + mockMvc.delete("/roles/1234/activity/1234/permissions") { contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(Permissions(listOf(Permission.EDIT_ACTIVITY))) + content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + }.andExpect { status { isNotFound() } } From a17fe90cdf14db37dc519f4d47352471687e406b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 26 Apr 2023 16:09:38 +0100 Subject: [PATCH 06/24] Add documentation and refactor tests --- .../backend/controller/RoleController.kt | 24 +- .../backend/dto/permissions/PermissionsDto.kt | 3 +- .../ni/website/backend/service/RoleService.kt | 1 - .../backend/controller/RoleControllerTest.kt | 598 ++++++++++++------ .../backend/utils/documentation/Tag.kt | 3 +- .../model/PayloadPermissions.kt | 14 + .../payloadschemas/model/PayloadRoles.kt | 22 + 7 files changed, 444 insertions(+), 221 deletions(-) create mode 100644 src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadPermissions.kt create mode 100644 src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 874e16e6..20ca2315 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -9,7 +9,6 @@ import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RestController import pt.up.fe.ni.website.backend.dto.entity.RoleDto import pt.up.fe.ni.website.backend.dto.permissions.PermissionsDto -import pt.up.fe.ni.website.backend.model.permissions.Permission import pt.up.fe.ni.website.backend.model.permissions.Permissions import pt.up.fe.ni.website.backend.service.RoleService @@ -32,15 +31,18 @@ class RoleController(private val roleService: RoleService) { } @PostMapping("/{id}/grant") - fun grantPermissionRole(@PathVariable id: Long, - @RequestBody permissionsDto: PermissionsDto + fun grantPermissionRole( + @PathVariable id: Long, + @RequestBody permissionsDto: PermissionsDto ): Map { roleService.grantPermissionToRole(id, Permissions(permissionsDto.permissions)) return emptyMap() } @PostMapping("/{id}/revoke") - fun revokePermissionRole(@PathVariable id: Long, @RequestBody permissionsDto: PermissionsDto + fun revokePermissionRole( + @PathVariable id: Long, + @RequestBody permissionsDto: PermissionsDto ): Map { roleService.revokePermissionFromRole(id, Permissions(permissionsDto.permissions)) return emptyMap() @@ -64,8 +66,11 @@ class RoleController(private val roleService: RoleService) { @PathVariable activityId: Long, @RequestBody permissionsDto: PermissionsDto ): Map { - roleService.grantPermissionToRoleOnActivity(id, activityId, - Permissions(permissionsDto.permissions)) + roleService.grantPermissionToRoleOnActivity( + id, + activityId, + Permissions(permissionsDto.permissions) + ) return emptyMap() } @@ -75,8 +80,11 @@ class RoleController(private val roleService: RoleService) { @PathVariable activityId: Long, @RequestBody permissionsDto: PermissionsDto ): Map { - roleService.revokePermissionFromRoleOnActivity(id, activityId, - Permissions(permissionsDto.permissions)) + roleService.revokePermissionFromRoleOnActivity( + id, + activityId, + Permissions(permissionsDto.permissions) + ) return emptyMap() } } diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt index dd73046e..3f2c3235 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt @@ -2,7 +2,6 @@ package pt.up.fe.ni.website.backend.dto.permissions import pt.up.fe.ni.website.backend.model.permissions.Permissions - data class PermissionsDto( - val permissions : Permissions + val permissions: Permissions ) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 58fe5496..de258bd8 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -80,7 +80,6 @@ class RoleService( foundActivity.permissions.removeAll(permissions) perActivityRoleRepository.save(foundActivity) activityRepository.save(activity) - } fun createNewRole(dto: RoleDto): Role { diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 81dee265..27637ae5 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -10,10 +10,18 @@ import org.junit.jupiter.api.Test import org.springframework.beans.factory.annotation.Autowired import org.springframework.data.repository.findByIdOrNull import org.springframework.http.MediaType +import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.delete +import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.get +import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.post +import org.springframework.restdocs.payload.JsonFieldType import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.delete import org.springframework.test.web.servlet.get import org.springframework.test.web.servlet.post +import org.springframework.test.web.servlet.result.MockMvcResultMatchers.content +import org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath +import org.springframework.test.web.servlet.result.MockMvcResultMatchers.status +import org.springframework.web.servlet.function.RequestPredicates.contentType import pt.up.fe.ni.website.backend.model.Account import pt.up.fe.ni.website.backend.model.CustomWebsite import pt.up.fe.ni.website.backend.model.Generation @@ -29,6 +37,24 @@ import pt.up.fe.ni.website.backend.repository.RoleRepository import pt.up.fe.ni.website.backend.utils.TestUtils import pt.up.fe.ni.website.backend.utils.annotations.ControllerTest import pt.up.fe.ni.website.backend.utils.annotations.NestedTest +import pt.up.fe.ni.website.backend.utils.documentation.Tag +import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadAccount +import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadPermissions +import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadRoles +import pt.up.fe.ni.website.backend.utils.documentation.utils.DocumentedJSONField +import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocument +import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocumentEmptyObjectResponse +import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocumentErrorResponse +import pt.up.fe.ni.website.backend.utils.documentation.utils.ModelDocumentation + +class AccountRoleDocumentation : ModelDocumentation( + Tag.ROLES.name.lowercase() + "-accounts", + Tag.ROLES, + mutableListOf( + DocumentedJSONField("[]", "Array that only contains ONE account id", JsonFieldType.ARRAY) + ) + +) @ControllerTest @Transactional @@ -44,6 +70,12 @@ internal class RoleControllerTest @Autowired constructor( "22-23" ) + val documentationRoles = PayloadRoles() + + val documentationAccount = PayloadAccount() + + val documentationPermissions = PayloadPermissions() + val testRole = Role( "TestRole", Permissions(listOf(Permission.CREATE_ACTIVITY)), @@ -97,11 +129,15 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should return all roles`() { - mockMvc.get("/roles").andExpect { - status { isOk() } - content { contentType(MediaType.APPLICATION_JSON) } - content { json(objectMapper.writeValueAsString(roles)) } - } + mockMvc.perform(get("/roles")).andExpectAll( + status().isOk, + content().contentType(MediaType.APPLICATION_JSON), + content().json(objectMapper.writeValueAsString(roles)) + ).andDocument( + documentationRoles.getModelDocumentationArray(), + "Returns all existing roles", + "This endpoint returns all existing roles in the database for ease of use." + ) } } @@ -115,21 +151,25 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should return testRole when provided by its id`() { - mockMvc.get("/roles/${testRole.id}").andExpect { - status { isOk() } - content { contentType(MediaType.APPLICATION_JSON) } - content { json(objectMapper.writeValueAsString(testRole)) } - } + mockMvc.perform(get("/roles/${testRole.id}")).andExpectAll( + status().isOk, + content().contentType(MediaType.APPLICATION_JSON), + content().json(objectMapper.writeValueAsString(testRole)) + ).andDocument( + documentationRoles, + "Returns a specific role", + "Returns a summary brief of a specific role, which makes getting one role way more efficient. " + ) } @Test fun `should return error on invalid roleID`() { - mockMvc.get("/roles/4020").andExpect { - status { isNotFound() } - content { contentType(MediaType.APPLICATION_JSON) } - jsonPath("$.errors.length()") { value(1) } - jsonPath("$.errors.[0].message") { value("role not found with id 4020") } - } + mockMvc.perform(get("/roles/4020")).andExpectAll( + status().isNotFound(), + content().contentType(MediaType.APPLICATION_JSON), + jsonPath("$.errors.length()").value(1), + jsonPath("$.errors.[0].message").value("role not found with id 4020") + ).andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) } } @@ -140,58 +180,80 @@ internal class RoleControllerTest @Autowired constructor( fun addRole() { generationRepository.save(testGeneration) roleRepository.save(testRole) + testGeneration.roles.add(testRole) + roleRepository.save(testRole) + generationRepository.save(testGeneration) + } + + @AfterEach + fun removeRole() { + testGeneration.roles.remove(testRole) + testGeneration.roles.remove(otherTestRole) + roleRepository.save(testRole) + generationRepository.save(testGeneration) } @Test fun `should add new role`() { - mockMvc.post("/roles/new") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString( - mapOf( - "name" to otherTestRole.name, - "permissions" to otherTestRole.permissions.map { it.bit }, - "isSection" to otherTestRole.isSection, - "associatedActivities" to otherTestRole.associatedActivities, - "accounts" to otherTestRole.accounts + mockMvc.perform( + post("/roles/new") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "name" to otherTestRole.name, + "permissions" to otherTestRole.permissions.map { it.bit }.toList(), + "isSection" to otherTestRole.isSection, + "associatedActivities" to otherTestRole.associatedActivities, + "accounts" to otherTestRole.accounts + ) + ) ) - ) - }.andExpect { - status { isOk() } - content { contentType(MediaType.APPLICATION_JSON) } - jsonPath("$.name") { value(otherTestRole.name) } - jsonPath("$.id") { value(2) } // this must be hardcoded - jsonPath("$.permissions.length()") { value(otherTestRole.permissions.size) } - jsonPath("$.isSection") { value(otherTestRole.isSection) } - jsonPath("$.associatedActivities") { value(otherTestRole.associatedActivities) } - } + ).andExpectAll( + status().isOk, + content().contentType(MediaType.APPLICATION_JSON), + jsonPath("$.name").value(otherTestRole.name), + jsonPath("$.id").value(2), // this must be hardcoded + jsonPath("$.permissions.length()").value(otherTestRole.permissions.size), + jsonPath("$.isSection").value(otherTestRole.isSection), + jsonPath("$.associatedActivities").value(otherTestRole.associatedActivities) + ).andDocument( + documentationRoles, + "This creates a role, returning the complete role information", + "It will only create the role if it doesn't exist any other role with the same name" + ) assert(roleRepository.existsById(2)) assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) - assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 1) - assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles[0].id!!.compareTo(2) == 0) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 2) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles[1].id!!.compareTo(2) == 0) } @Test fun `shouldn't add role with same name`() { - mockMvc.post("/roles/new") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString( - mapOf( - "name" to testRole.name, - "permissions" to testRole.permissions.map { it.bit }, - "isSection" to testRole.isSection, - "associatedActivities" to testRole.associatedActivities, - "accounts" to testRole.accounts + mockMvc.perform( + post("/roles/new") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "name" to testRole.name, + "permissions" to otherTestRole.permissions.map { it.bit }.toList(), + "isSection" to testRole.isSection, + "associatedActivities" to testRole.associatedActivities, + "accounts" to testRole.accounts + ) + ) ) - ) - }.andExpect { - status { isUnprocessableEntity() } - content { contentType(MediaType.APPLICATION_JSON) } - jsonPath("$.errors.length()") { value(1) } - jsonPath("$.errors.[0].message") { value("role already exists") } - } + ).andExpectAll( + status().isUnprocessableEntity, + content().contentType(MediaType.APPLICATION_JSON), + jsonPath("$.errors.length()").value(1), + jsonPath("$.errors.[0].message").value("role already exists") + ) + .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) assert(roleRepository.findByName(testRole.name) != null) assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) - assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 1) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 2) } } @@ -206,9 +268,13 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove role with correct id`() { - mockMvc.delete("/roles/${testRole.id}").andExpect { - status { isOk() } - } + mockMvc.perform(delete("/roles/${testRole.id}")).andExpectAll( + status().isOk + ).andDocumentEmptyObjectResponse( + documentationRoles, + "Removes the role by its id", + "The id must exist in order to remove it correctly" + ) assert(roleRepository.findByName(testRole.name) == null) assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 0) @@ -216,12 +282,13 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove role id that does not exist`() { - mockMvc.delete("/roles/1234").andExpect { - status { isNotFound() } - content { contentType(MediaType.APPLICATION_JSON) } - jsonPath("$.errors.length()") { value(1) } - jsonPath("$.errors.[0].message") { value("role not found with id 1234") } - } + mockMvc.perform(delete("/roles/1234")).andExpectAll( + status().isNotFound(), + content().contentType(MediaType.APPLICATION_JSON), + jsonPath("$.errors.length()").value(1), + jsonPath("$.errors.[0].message").value("role not found with id 1234") + ) + .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) } } @@ -235,27 +302,42 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should grant permission to role that exists`() { - mockMvc.post("/roles/${testRole.id}/grant") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf( - "permissions" to Permissions(listOf(Permission.SUPERUSER)) - )) - }.andExpect { - status { isOk() } - } + mockMvc.perform( + post("/roles/${testRole.id}/grant") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + ) + ) + ) + ).andExpectAll( + status().isOk + ) + .andDocumentEmptyObjectResponse( + documentationPermissions, + "Adds a set of permissions to a role by ID", + "This doesn't check if the permission is already added, the role ID must be valid" + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) } @Test fun `should grant permission to role that does not exists`() { - mockMvc.post("/roles/1234/grant") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf( - "permissions" to Permissions(listOf(Permission.SUPERUSER)) - )) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + post("/roles/1234/grant") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + ) + ) + ) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) } } @@ -269,27 +351,43 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should revoke permission to role that exists`() { - mockMvc.post("/roles/${testRole.id}/revoke") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf( - "permissions" to Permissions(listOf(Permission.SUPERUSER)) - )) - }.andExpect { - status { isOk() } - } + mockMvc.perform( + post("/roles/${testRole.id}/revoke") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + ) + ) + ) + ) + .andExpectAll( + status().isOk + ).andDocumentEmptyObjectResponse( + documentationPermissions, + "Revokes permission by role ID", + "Revokes permissions, it doesn't check if the permissions to be revoked are already off." + ) assert(!roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) } @Test fun `should revoke permission to role that does not exists`() { - mockMvc.post("/roles/1234/revoke") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf( - "permissions" to Permissions(listOf(Permission.SUPERUSER)) - )) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + post("/roles/1234/revoke") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "permissions" to Permissions(listOf(Permission.SUPERUSER)) + ) + ) + ) + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) + .andExpectAll( + status().isNotFound() + ) } } @@ -303,45 +401,53 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should add an existing account to an existing role`() { - mockMvc.post("/roles/${testRole.id}/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(testAccount.id) - }.andExpect { - status { isOk() } - } + mockMvc.perform( + post("/roles/${testRole.id}/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(testAccount.id)) + ).andExpectAll( + status().isOk + ).andDocumentEmptyObjectResponse( + documentationAccount, + "Add an account to a role by its IDs", + "It will return an error if the user is already in the role or if the role doesn't exist" + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @Test fun `should not add an non existing account to an existing role`() { - mockMvc.post("/roles/${testRole.id}/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(1234) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + post("/roles/${testRole.id}/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(1234)) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @Test fun `should not add an non existing account to an non existing role`() { - mockMvc.post("/roles/1234/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(1234) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + post("/roles/1234/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(1234)) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @Test fun `should not add an existing account to an non existing role`() { - mockMvc.post("/roles/1234/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(testAccount.id) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + post("/roles/1234/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(testAccount.id)) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } } @@ -364,103 +470,138 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove an existing account to an existing role`() { - mockMvc.delete("/roles/${testRole.id}/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(testAccount.id) - }.andExpect { - status { isOk() } - } + mockMvc.perform( + delete("/roles/${testRole.id}/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(testAccount.id)) + ).andExpectAll( + status().isOk + ).andDocumentEmptyObjectResponse( + documentationAccount, + "Removes the account on the role by ID", + "It only works if the user is in role and role exists" + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @Test fun `should not remove an non existing account to an existing role`() { - mockMvc.delete("/roles/${testRole.id}/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(1234) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + delete("/roles/${testRole.id}/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(1234)) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @Test fun `should not remove an non existing account to an non existing role`() { - mockMvc.delete("/roles/1234/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(1234) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + delete("/roles/1234/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(1234)) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @Test fun `should not remove an existing account to an non existing role`() { - mockMvc.delete("/roles/1234/users") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(testAccount.id) - }.andExpect { - status { isNotFound() } - } + mockMvc.perform( + delete("/roles/1234/users") + .contentType(MediaType.APPLICATION_JSON) + .content(objectMapper.writeValueAsString(testAccount.id)) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } } @NestedTest - inner class AddPermissionToRoleActivity{ + inner class AddPermissionToRoleActivity { @BeforeEach - fun addAll(){ + fun addAll() { roleRepository.save(testRole) projectRepository.save(testProject) } @Test - fun `should add permission to role activity and create PerActivityRole`(){ - mockMvc.post("/roles/${testRole.id}/activity/${testProject.id}/permissions"){ - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) - }.andExpect { - status { isOk() } - } + fun `should add permission to role activity and create PerActivityRole`() { + mockMvc.perform( + post("/roles/${testRole.id}/activity/${testProject.id}/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) + ).andExpectAll( + status().isOk + ).andDocumentEmptyObjectResponse( + documentationPermissions, + "Adds an permission to an role activity", + "It will create a PerRoleActivity if it doesn't exist" + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 1) - assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities[0].permissions.contains(Permission.EDIT_ACTIVITY)) + assert( + roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities[0].permissions.contains( + Permission.EDIT_ACTIVITY + ) + ) } + @Test - fun `shouldn't add permission to role activity if roleId is invalid`(){ - mockMvc.post("/roles/1234/activity/${testProject.id}/permissions"){ - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) - }.andExpect { - status { isNotFound() } - } + fun `shouldn't add permission to role activity if roleId is invalid`() { + mockMvc.perform( + post("/roles/1234/activity/${testProject.id}/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @Test - fun `shouldn't add permission to role activity if activityId is invalid`(){ - mockMvc.post("/roles/${testRole.id}/activity/1234/permissions"){ - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) - }.andExpect { - status { isNotFound() } - } + fun `shouldn't add permission to role activity if activityId is invalid`() { + mockMvc.perform( + post("/roles/${testRole.id}/activity/1234/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @Test - fun `shouldn't add permission to role activity if activityId is invalid and roleId is invalid`(){ - mockMvc.post("/roles/${testRole.id}/activity/1234/permissions"){ - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + fun `shouldn't add permission to role activity if activityId is invalid and roleId is invalid`() { + mockMvc.perform( + post("/roles/${testRole.id}/activity/1234/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) - }.andExpect { - status { isNotFound() } - } + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } - - } @NestedTest @@ -469,7 +610,7 @@ internal class RoleControllerTest @Autowired constructor( fun addAll() { projectRepository.save(testProject) roleRepository.save(testRole) - var perActivityRole = PerActivityRole(Permissions(listOf(Permission.EDIT_ACTIVITY))) + val perActivityRole = PerActivityRole(Permissions(listOf(Permission.EDIT_ACTIVITY))) perActivityRole.activity = testProject perActivityRole.role = testRole testProject.associatedRoles.add(perActivityRole) @@ -485,56 +626,95 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove an existing role activity permission`() { - mockMvc.delete("/roles/${testRole.id}/activity/${testProject.id}/permissions") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + mockMvc.perform( + delete("/roles/${testRole.id}/activity/${testProject.id}/permissions") + .contentType(MediaType.APPLICATION_JSON).contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) - }.andExpect { - status { isOk() } - } + ).andExpectAll( + status().isOk + ).andDocumentEmptyObjectResponse( + documentationPermissions, + "Removes an PerRoleActivity permission by ID", + "It will not create a PerRoleActivity if it doesn't exist, it will simply return, " + + "doesn't check if the permissions already are revoked..." + ) assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) - assert(!projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) - + assert( + !projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + Permission.EDIT_ACTIVITY + ) + ) } @Test fun `should not remove an existing role activity permission on a non existing role`() { - mockMvc.delete("/roles/1234/activity/${testProject.id}/permissions") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + mockMvc.perform( + delete("/roles/1234/activity/${testProject.id}/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) - }.andExpect { - status { isNotFound() } - } + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) + assert( + projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + Permission.EDIT_ACTIVITY + ) + ) } + @Test fun `should not remove an existing role activity permission on a non existing activity`() { - mockMvc.delete("/roles/${testRole.id}/activity/1234/permissions") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + mockMvc.perform( + delete("/roles/${testRole.id}/activity/1234/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) - }.andExpect { - status { isNotFound() } - } + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) - + assert( + projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + Permission.EDIT_ACTIVITY + ) + ) } @Test - fun `should not remove an existing role activity permission on a non existing activity and non existing role`() { - mockMvc.delete("/roles/1234/activity/1234/permissions") { - contentType = MediaType.APPLICATION_JSON - content = objectMapper.writeValueAsString(mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY)))) + fun `should not remove an existing role activity perm on a non existing activity and non existing role`() { + mockMvc.perform( + delete("/roles/1234/activity/1234/permissions") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) + ) + ) - }.andExpect { - status { isNotFound() } - } + ).andExpectAll( + status().isNotFound() + ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains(Permission.EDIT_ACTIVITY)) - + assert( + projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + Permission.EDIT_ACTIVITY + ) + ) } } } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/Tag.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/Tag.kt index 3354c7f7..f7c3e8f1 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/Tag.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/Tag.kt @@ -8,5 +8,6 @@ enum class Tag(override val fullName: String) : ITag { EVENT("Events"), GENERATION("Generations"), POST("Posts"), - PROJECT("Projects") + PROJECT("Projects"), + ROLES("Role Management") } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadPermissions.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadPermissions.kt new file mode 100644 index 00000000..abb6ca1d --- /dev/null +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadPermissions.kt @@ -0,0 +1,14 @@ +package pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model + +import org.springframework.restdocs.payload.JsonFieldType +import pt.up.fe.ni.website.backend.utils.documentation.Tag +import pt.up.fe.ni.website.backend.utils.documentation.utils.DocumentedJSONField +import pt.up.fe.ni.website.backend.utils.documentation.utils.ModelDocumentation + +class PayloadPermissions : ModelDocumentation( + Tag.ROLES.name.lowercase(), + Tag.ROLES, + mutableListOf( + DocumentedJSONField("permissions", "Permissions array", JsonFieldType.ARRAY) + ) +) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt new file mode 100644 index 00000000..f588bb50 --- /dev/null +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt @@ -0,0 +1,22 @@ +package pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model + +import org.springframework.restdocs.payload.JsonFieldType +import pt.up.fe.ni.website.backend.utils.documentation.Tag +import pt.up.fe.ni.website.backend.utils.documentation.utils.DocumentedJSONField +import pt.up.fe.ni.website.backend.utils.documentation.utils.ModelDocumentation + +class PayloadRoles : ModelDocumentation( + Tag.ROLES.name.lowercase(), + Tag.ROLES, + mutableListOf( + DocumentedJSONField("name", "Name of the role", JsonFieldType.STRING), + DocumentedJSONField("permissions", "Permissions in the role, as an integer", JsonFieldType.ARRAY), + DocumentedJSONField("isSection", "Role is section", JsonFieldType.BOOLEAN), + DocumentedJSONField("id", "Internal ID of role", JsonFieldType.NUMBER), + DocumentedJSONField( + "associatedActivities", + "List of activities that are associated with this role", + JsonFieldType.ARRAY + ) + ) +) From a6d64c37fc03b8949c3dbc3e5b4dbb81c999c92f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 21 Jun 2023 10:42:46 +0100 Subject: [PATCH 07/24] [no ci] Apply suggestions from code review Co-authored-by: BrunoRosendo --- .../ni/website/backend/controller/RoleController.kt | 12 ++++++------ .../up/fe/ni/website/backend/service/RoleService.kt | 4 ++-- .../payloadschemas/model/PayloadRoles.kt | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 20ca2315..fb9d0095 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -21,7 +21,7 @@ class RoleController(private val roleService: RoleService) { @GetMapping("/{id}") fun getRole(@PathVariable id: Long) = roleService.getRole(id) - @PostMapping("/new") + @PostMapping fun createNewRole(@RequestBody dto: RoleDto) = roleService.createNewRole(dto) @DeleteMapping("/{id}") @@ -31,7 +31,7 @@ class RoleController(private val roleService: RoleService) { } @PostMapping("/{id}/grant") - fun grantPermissionRole( + fun grantPermissionToRole( @PathVariable id: Long, @RequestBody permissionsDto: PermissionsDto ): Map { @@ -40,7 +40,7 @@ class RoleController(private val roleService: RoleService) { } @PostMapping("/{id}/revoke") - fun revokePermissionRole( + fun revokePermissionFromRole( @PathVariable id: Long, @RequestBody permissionsDto: PermissionsDto ): Map { @@ -60,7 +60,7 @@ class RoleController(private val roleService: RoleService) { return emptyMap() } - @PostMapping("/{id}/activity/{activityId}/permissions") + @PostMapping("/{id}/activities/{activityId}/permissions") fun addPermissionToPerActivityRole( @PathVariable id: Long, @PathVariable activityId: Long, @@ -74,8 +74,8 @@ class RoleController(private val roleService: RoleService) { return emptyMap() } - @DeleteMapping("/{id}/activity/{activityId}/permissions") - fun revokePermissionToPerActivityRole( + @DeleteMapping("/{id}/activities/{activityId}/permissions") + fun revokePermissionFromPerActivityRole( @PathVariable id: Long, @PathVariable activityId: Long, @RequestBody permissionsDto: PermissionsDto diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index de258bd8..4d50310f 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -24,7 +24,7 @@ class RoleService( private val activityRepository: ActivityRepository ) { - fun getRole(roleId: Long): Role { + fun getRoleById(roleId: Long): Role { val role = roleRepository.findById(roleId).orElseThrow { throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) } @@ -54,7 +54,7 @@ class RoleService( val role = roleRepository.findById(roleId).orElseThrow { throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) } - val foundActivity = activity.associatedRoles + val foundPerActivityRole = activity.associatedRoles .find { it.activity == activity } ?: PerActivityRole(Permissions()) foundActivity.role = role foundActivity.activity = activity diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt index f588bb50..e738e1e8 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt @@ -11,7 +11,7 @@ class PayloadRoles : ModelDocumentation( mutableListOf( DocumentedJSONField("name", "Name of the role", JsonFieldType.STRING), DocumentedJSONField("permissions", "Permissions in the role, as an integer", JsonFieldType.ARRAY), - DocumentedJSONField("isSection", "Role is section", JsonFieldType.BOOLEAN), + DocumentedJSONField("isSection", "Whether the role should be displayed as a section", JsonFieldType.BOOLEAN), DocumentedJSONField("id", "Internal ID of role", JsonFieldType.NUMBER), DocumentedJSONField( "associatedActivities", From 50faa4e408524f06ef1b13401258eb5ff2c33357 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 21 Jun 2023 10:45:59 +0100 Subject: [PATCH 08/24] [no ci] Apply test files suggestions from code review Co-authored-by: BrunoRosendo --- .../backend/controller/RoleControllerTest.kt | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 27637ae5..ed4ffdd0 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -158,7 +158,7 @@ internal class RoleControllerTest @Autowired constructor( ).andDocument( documentationRoles, "Returns a specific role", - "Returns a summary brief of a specific role, which makes getting one role way more efficient. " + "Returns a summary brief of a specific role, which makes getting one role way more efficient." ) } @@ -220,7 +220,7 @@ internal class RoleControllerTest @Autowired constructor( ).andDocument( documentationRoles, "This creates a role, returning the complete role information", - "It will only create the role if it doesn't exist any other role with the same name" + "It will only create the role if it no other role with the same name exists in that generation" ) assert(roleRepository.existsById(2)) assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) @@ -281,7 +281,7 @@ internal class RoleControllerTest @Autowired constructor( } @Test - fun `should not remove role id that does not exist`() { + fun `should not remove role if id does not exist`() { mockMvc.perform(delete("/roles/1234")).andExpectAll( status().isNotFound(), content().contentType(MediaType.APPLICATION_JSON), @@ -293,7 +293,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest - inner class GrantPermissionRole { + inner class GrantPermissionToRole { @BeforeEach fun addRole() { @@ -324,7 +324,7 @@ internal class RoleControllerTest @Autowired constructor( } @Test - fun `should grant permission to role that does not exists`() { + fun `should not grant permission to role that does not exists`() { mockMvc.perform( post("/roles/1234/grant") .contentType(MediaType.APPLICATION_JSON) @@ -342,7 +342,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest - inner class RevokePermissionRole { + inner class RevokePermissionFromRole { @BeforeEach fun addRole() { @@ -350,7 +350,7 @@ internal class RoleControllerTest @Autowired constructor( } @Test - fun `should revoke permission to role that exists`() { + fun `should revoke permission from role that exists`() { mockMvc.perform( post("/roles/${testRole.id}/revoke") .contentType(MediaType.APPLICATION_JSON) @@ -367,13 +367,13 @@ internal class RoleControllerTest @Autowired constructor( ).andDocumentEmptyObjectResponse( documentationPermissions, "Revokes permission by role ID", - "Revokes permissions, it doesn't check if the permissions to be revoked are already off." + "Revokes permissions, it doesn't check if the role contains them." ) assert(!roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) } @Test - fun `should revoke permission to role that does not exists`() { + fun `should not revoke permission from role that does not exist`() { mockMvc.perform( post("/roles/1234/revoke") .contentType(MediaType.APPLICATION_JSON) @@ -410,7 +410,7 @@ internal class RoleControllerTest @Autowired constructor( ).andDocumentEmptyObjectResponse( documentationAccount, "Add an account to a role by its IDs", - "It will return an error if the user is already in the role or if the role doesn't exist" + "It will return an error if the user already has the role or if it doesn't exist" ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -469,7 +469,7 @@ internal class RoleControllerTest @Autowired constructor( } @Test - fun `should remove an existing account to an existing role`() { + fun `should remove an existing account from an existing role`() { mockMvc.perform( delete("/roles/${testRole.id}/users") .contentType(MediaType.APPLICATION_JSON) @@ -478,14 +478,14 @@ internal class RoleControllerTest @Autowired constructor( status().isOk ).andDocumentEmptyObjectResponse( documentationAccount, - "Removes the account on the role by ID", - "It only works if the user is in role and role exists" + "Removes the account from the role by ID", + "It only works if the user has the role and it exists" ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @Test - fun `should not remove an non existing account to an existing role`() { + fun `should not remove a non existing account from an existing role`() { mockMvc.perform( delete("/roles/${testRole.id}/users") .contentType(MediaType.APPLICATION_JSON) @@ -641,7 +641,7 @@ internal class RoleControllerTest @Autowired constructor( documentationPermissions, "Removes an PerRoleActivity permission by ID", "It will not create a PerRoleActivity if it doesn't exist, it will simply return, " + - "doesn't check if the permissions already are revoked..." + "doesn't check if the permissions are already revoked..." ) assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) assert( From 09ee6c45f5c579341b997f05f1e9081a54def8a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Tue, 27 Jun 2023 23:35:19 +0100 Subject: [PATCH 09/24] first improvements to readability and consistency --- .../backend/controller/RoleController.kt | 17 +++-- .../backend/dto/permissions/UserIdDto.kt | 5 ++ .../ni/website/backend/service/RoleService.kt | 63 ++++++---------- .../backend/controller/RoleControllerTest.kt | 74 +++++++++---------- 4 files changed, 73 insertions(+), 86 deletions(-) create mode 100644 src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index fb9d0095..c8a0d895 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -9,6 +9,7 @@ import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RestController import pt.up.fe.ni.website.backend.dto.entity.RoleDto import pt.up.fe.ni.website.backend.dto.permissions.PermissionsDto +import pt.up.fe.ni.website.backend.dto.permissions.UserIdDto import pt.up.fe.ni.website.backend.model.permissions.Permissions import pt.up.fe.ni.website.backend.service.RoleService @@ -30,33 +31,33 @@ class RoleController(private val roleService: RoleService) { return emptyMap() } - @PostMapping("/{id}/grant") + @PostMapping("/{id}/permissions") fun grantPermissionToRole( @PathVariable id: Long, @RequestBody permissionsDto: PermissionsDto ): Map { - roleService.grantPermissionToRole(id, Permissions(permissionsDto.permissions)) + roleService.grantPermissionToRole(id, permissionsDto.permissions) return emptyMap() } - @PostMapping("/{id}/revoke") + @DeleteMapping("/{id}/permissions") fun revokePermissionFromRole( @PathVariable id: Long, @RequestBody permissionsDto: PermissionsDto ): Map { - roleService.revokePermissionFromRole(id, Permissions(permissionsDto.permissions)) + roleService.revokePermissionFromRole(id, permissionsDto.permissions) return emptyMap() } @PostMapping("/{id}/users") - fun addUserToRole(@PathVariable id: Long, @RequestBody userId: Long): Map { - roleService.addUserToRole(id, userId) + fun addUserToRole(@PathVariable id: Long, @RequestBody userIdDto: UserIdDto): Map { + roleService.addUserToRole(id, userIdDto.userId) return emptyMap() } @DeleteMapping("/{id}/users") - fun removeUserFromRole(@PathVariable id: Long, @RequestBody userId: Long): Map { - roleService.removeUserFromRole(id, userId) + fun removeUserFromRole(@PathVariable id: Long, @RequestBody userIdDto: UserIdDto): Map { + roleService.removeUserFromRole(id, userIdDto.userId) return emptyMap() } diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt new file mode 100644 index 00000000..c3ec905d --- /dev/null +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt @@ -0,0 +1,5 @@ +package pt.up.fe.ni.website.backend.dto.permissions + +data class UserIdDto ( + val userId: Long +) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 4d50310f..eefa95cf 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -13,6 +13,7 @@ import pt.up.fe.ni.website.backend.repository.ActivityRepository import pt.up.fe.ni.website.backend.repository.GenerationRepository import pt.up.fe.ni.website.backend.repository.PerActivityRoleRepository import pt.up.fe.ni.website.backend.repository.RoleRepository +import pt.up.fe.ni.website.backend.service.activity.ActivityService @Service @Transactional @@ -20,11 +21,11 @@ class RoleService( private val roleRepository: RoleRepository, private val perActivityRoleRepository: PerActivityRoleRepository, private val generationRepository: GenerationRepository, - private val accountRepository: AccountRepository, - private val activityRepository: ActivityRepository + private val accountService: AccountService, + private val activityService: ActivityService, ) { - fun getRoleById(roleId: Long): Role { + fun getRole(roleId: Long): Role { val role = roleRepository.findById(roleId).orElseThrow { throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) } @@ -32,54 +33,42 @@ class RoleService( } fun getAllRoles(): List = roleRepository.findAll().toList() fun grantPermissionToRole(roleId: Long, permissions: Permissions) { - val role = roleRepository.findById(roleId).orElseThrow { - throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) - } + val role = getRole(roleId) role.permissions.addAll(permissions) roleRepository.save(role) } fun revokePermissionFromRole(roleId: Long, permissions: Permissions) { - val role = roleRepository.findById(roleId).orElseThrow { - throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) - } + val role = getRole(roleId) role.permissions.removeAll(permissions) roleRepository.save(role) } fun grantPermissionToRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { - val activity = activityRepository.findById(activityId).orElseThrow { - throw NoSuchElementException(ErrorMessages.activityNotFound(activityId)) - } - val role = roleRepository.findById(roleId).orElseThrow { - throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) - } + val activity = activityService.getActivityById(activityId) + val role = getRole(roleId) val foundPerActivityRole = activity.associatedRoles .find { it.activity == activity } ?: PerActivityRole(Permissions()) - foundActivity.role = role - foundActivity.activity = activity + foundPerActivityRole.role = role + foundPerActivityRole.activity = activity - foundActivity.permissions.addAll(permissions) - perActivityRoleRepository.save(foundActivity) + foundPerActivityRole.permissions.addAll(permissions) + perActivityRoleRepository.save(foundPerActivityRole) if (activity.associatedRoles.find { it.activity == activity } == null) { - role.associatedActivities.add(foundActivity) + role.associatedActivities.add(foundPerActivityRole) } - activityRepository.save(activity) + //activityRepository.save(activity) roleRepository.save(role) } fun revokePermissionFromRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { - val activity = activityRepository.findById(activityId).orElseThrow { - throw NoSuchElementException(ErrorMessages.activityNotFound(activityId)) - } - val role = roleRepository.findById(roleId).orElseThrow { - throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) - } + val activity = activityService.getActivityById(activityId) + val role = getRole(roleId) val foundActivity = activity.associatedRoles .find { it.role == role } ?: return foundActivity.permissions.removeAll(permissions) perActivityRoleRepository.save(foundActivity) - activityRepository.save(activity) + //activityRepository.save(activity) } fun createNewRole(dto: RoleDto): Role { @@ -87,6 +76,7 @@ class RoleService( throw IllegalArgumentException(ErrorMessages.roleAlreadyExists) } val role = dto.create() + val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc() ?: throw IllegalArgumentException(ErrorMessages.noGenerations) roleRepository.save(role) @@ -96,19 +86,16 @@ class RoleService( } fun deleteRole(roleId: Long) { - val role = roleRepository.findByIdOrNull(roleId) - ?: throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) + val role = getRole(roleId) val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc()!! latestGeneration.roles.remove(role) generationRepository.save(latestGeneration) - roleRepository.deleteById(roleId) + roleRepository.delete(role) } fun addUserToRole(roleId: Long, userId: Long) { - val role = roleRepository.findByIdOrNull(roleId) - ?: throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) - val account = accountRepository.findByIdOrNull(userId) - ?: throw NoSuchElementException(ErrorMessages.accountNotFound(userId)) + val role = getRole(roleId) + val account = accountService.getAccountById(userId) role.accounts.find { it.id == account.id }.let { if (it != null) throw NoSuchElementException(ErrorMessages.userAlreadyHasRole(roleId, userId)) } @@ -117,10 +104,8 @@ class RoleService( } fun removeUserFromRole(roleId: Long, userId: Long) { - val role = roleRepository.findByIdOrNull(roleId) - ?: throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) - val account = accountRepository.findByIdOrNull(userId) - ?: throw NoSuchElementException(ErrorMessages.accountNotFound(userId)) + val role = getRole(roleId) + val account = accountService.getAccountById(userId) role.accounts.find { it.id == account.id }.let { if (it == null) throw NoSuchElementException(ErrorMessages.userNotInRole(roleId, userId)) } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index ed4ffdd0..0ef00f7e 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -181,22 +181,13 @@ internal class RoleControllerTest @Autowired constructor( generationRepository.save(testGeneration) roleRepository.save(testRole) testGeneration.roles.add(testRole) - roleRepository.save(testRole) - generationRepository.save(testGeneration) - } - - @AfterEach - fun removeRole() { - testGeneration.roles.remove(testRole) - testGeneration.roles.remove(otherTestRole) - roleRepository.save(testRole) generationRepository.save(testGeneration) } @Test fun `should add new role`() { mockMvc.perform( - post("/roles/new") + post("/roles") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -222,7 +213,7 @@ internal class RoleControllerTest @Autowired constructor( "This creates a role, returning the complete role information", "It will only create the role if it no other role with the same name exists in that generation" ) - assert(roleRepository.existsById(2)) + assert(roleRepository.count().toInt() == 2) assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 2) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles[1].id!!.compareTo(2) == 0) @@ -231,7 +222,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add role with same name`() { mockMvc.perform( - post("/roles/new") + post("/roles") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -245,14 +236,12 @@ internal class RoleControllerTest @Autowired constructor( ) ) ).andExpectAll( - status().isUnprocessableEntity, + status().isBadRequest, content().contentType(MediaType.APPLICATION_JSON), jsonPath("$.errors.length()").value(1), - jsonPath("$.errors.[0].message").value("role already exists") ) .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) assert(roleRepository.findByName(testRole.name) != null) - assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 2) } } @@ -266,6 +255,12 @@ internal class RoleControllerTest @Autowired constructor( generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.add(testRole) } + @AfterEach + fun removeRole(){ + roleRepository.delete(testRole); + testGeneration.roles.remove(testRole) + } + @Test fun `should remove role with correct id`() { mockMvc.perform(delete("/roles/${testRole.id}")).andExpectAll( @@ -281,6 +276,7 @@ internal class RoleControllerTest @Autowired constructor( } @Test + @Transactional fun `should not remove role if id does not exist`() { mockMvc.perform(delete("/roles/1234")).andExpectAll( status().isNotFound(), @@ -303,7 +299,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should grant permission to role that exists`() { mockMvc.perform( - post("/roles/${testRole.id}/grant") + post("/roles/${testRole.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -326,7 +322,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not grant permission to role that does not exists`() { mockMvc.perform( - post("/roles/1234/grant") + post("/roles/1234/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -352,7 +348,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should revoke permission from role that exists`() { mockMvc.perform( - post("/roles/${testRole.id}/revoke") + delete("/roles/${testRole.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -375,7 +371,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not revoke permission from role that does not exist`() { mockMvc.perform( - post("/roles/1234/revoke") + delete("/roles/1234/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -384,10 +380,10 @@ internal class RoleControllerTest @Autowired constructor( ) ) ) + ).andExpectAll( + status().isNotFound() ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) - .andExpectAll( - status().isNotFound() - ) + } } @@ -404,7 +400,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( post("/roles/${testRole.id}/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(testAccount.id)) + .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( @@ -420,7 +416,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( post("/roles/${testRole.id}/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(1234)) + .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) @@ -432,7 +428,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( post("/roles/1234/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(1234)) + .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) @@ -444,7 +440,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( post("/roles/1234/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(testAccount.id)) + .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) @@ -473,7 +469,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( delete("/roles/${testRole.id}/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(testAccount.id)) + .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( @@ -489,7 +485,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( delete("/roles/${testRole.id}/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(1234)) + .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) @@ -501,7 +497,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( delete("/roles/1234/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(1234)) + .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) @@ -513,7 +509,7 @@ internal class RoleControllerTest @Autowired constructor( mockMvc.perform( delete("/roles/1234/users") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(testAccount.id)) + .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) @@ -532,7 +528,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should add permission to role activity and create PerActivityRole`() { mockMvc.perform( - post("/roles/${testRole.id}/activity/${testProject.id}/permissions") + post("/roles/${testRole.id}/activities/${testProject.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -557,7 +553,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if roleId is invalid`() { mockMvc.perform( - post("/roles/1234/activity/${testProject.id}/permissions") + post("/roles/1234/activities/${testProject.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -573,7 +569,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if activityId is invalid`() { mockMvc.perform( - post("/roles/${testRole.id}/activity/1234/permissions") + post("/roles/${testRole.id}/activities/1234/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -589,7 +585,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if activityId is invalid and roleId is invalid`() { mockMvc.perform( - post("/roles/${testRole.id}/activity/1234/permissions") + post("/roles/${testRole.id}/activities/1234/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -608,8 +604,8 @@ internal class RoleControllerTest @Autowired constructor( inner class RemovePermissionFromPerRoleActivity { @BeforeEach fun addAll() { - projectRepository.save(testProject) roleRepository.save(testRole) + projectRepository.save(testProject) val perActivityRole = PerActivityRole(Permissions(listOf(Permission.EDIT_ACTIVITY))) perActivityRole.activity = testProject perActivityRole.role = testRole @@ -627,7 +623,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove an existing role activity permission`() { mockMvc.perform( - delete("/roles/${testRole.id}/activity/${testProject.id}/permissions") + delete("/roles/${testRole.id}/activities/${testProject.id}/permissions") .contentType(MediaType.APPLICATION_JSON).contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -654,7 +650,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing role`() { mockMvc.perform( - delete("/roles/1234/activity/${testProject.id}/permissions") + delete("/roles/1234/activities/${testProject.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -676,7 +672,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing activity`() { mockMvc.perform( - delete("/roles/${testRole.id}/activity/1234/permissions") + delete("/roles/${testRole.id}/activities/1234/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -698,7 +694,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity perm on a non existing activity and non existing role`() { mockMvc.perform( - delete("/roles/1234/activity/1234/permissions") + delete("/roles/1234/activities/1234/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( From 28b41184e455cf905d8edbdc0ba0a1e6c1e63118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 5 Jul 2023 14:35:21 +0100 Subject: [PATCH 10/24] make tests run and remove ordercolumn --- .../backend/config/security/AuthConfig.kt | 1 + .../backend/controller/RoleController.kt | 5 +--- .../backend/dto/permissions/UserIdDto.kt | 2 +- .../fe/ni/website/backend/model/Generation.kt | 1 - .../ni/website/backend/service/RoleService.kt | 18 +++++------ .../backend/controller/RoleControllerTest.kt | 30 +++++++++++-------- 6 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt index 7f1939a8..e9b941d1 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt @@ -30,6 +30,7 @@ class AuthConfig( ) { @Bean fun securityFilterChain(http: HttpSecurity): SecurityFilterChain { + http.headers().frameOptions().disable() return http.csrf { csrf -> csrf.disable() }.cors().and() .oauth2ResourceServer().jwt() .jwtAuthenticationConverter(rolesConverter()) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index c8a0d895..5e94e94b 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -26,10 +26,7 @@ class RoleController(private val roleService: RoleService) { fun createNewRole(@RequestBody dto: RoleDto) = roleService.createNewRole(dto) @DeleteMapping("/{id}") - fun deleteRole(@PathVariable id: Long): Map { - roleService.deleteRole(id) - return emptyMap() - } + fun deleteRole(@PathVariable id: Long) = roleService.deleteRole(id) @PostMapping("/{id}/permissions") fun grantPermissionToRole( diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt index c3ec905d..e1cb3fde 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt @@ -1,5 +1,5 @@ package pt.up.fe.ni.website.backend.dto.permissions -data class UserIdDto ( +data class UserIdDto( val userId: Long ) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt index bdf5232e..2ce82ba7 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt @@ -25,7 +25,6 @@ class Generation( val id: Long? = null ) { @OneToMany(cascade = [CascadeType.ALL], fetch = FetchType.EAGER, mappedBy = "generation") - @OrderColumn @JsonManagedReference @field:NoDuplicateRoles val roles: MutableList<@Valid Role> = mutableListOf() diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index eefa95cf..97db02c5 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -1,15 +1,11 @@ package pt.up.fe.ni.website.backend.service import jakarta.transaction.Transactional -import org.springframework.data.repository.findByIdOrNull import org.springframework.stereotype.Service import pt.up.fe.ni.website.backend.dto.entity.RoleDto -import pt.up.fe.ni.website.backend.model.Activity import pt.up.fe.ni.website.backend.model.PerActivityRole import pt.up.fe.ni.website.backend.model.Role import pt.up.fe.ni.website.backend.model.permissions.Permissions -import pt.up.fe.ni.website.backend.repository.AccountRepository -import pt.up.fe.ni.website.backend.repository.ActivityRepository import pt.up.fe.ni.website.backend.repository.GenerationRepository import pt.up.fe.ni.website.backend.repository.PerActivityRoleRepository import pt.up.fe.ni.website.backend.repository.RoleRepository @@ -22,7 +18,7 @@ class RoleService( private val perActivityRoleRepository: PerActivityRoleRepository, private val generationRepository: GenerationRepository, private val accountService: AccountService, - private val activityService: ActivityService, + private val activityService: ActivityService ) { fun getRole(roleId: Long): Role { @@ -57,7 +53,7 @@ class RoleService( if (activity.associatedRoles.find { it.activity == activity } == null) { role.associatedActivities.add(foundPerActivityRole) } - //activityRepository.save(activity) + // activityRepository.save(activity) roleRepository.save(role) } @@ -68,7 +64,7 @@ class RoleService( .find { it.role == role } ?: return foundActivity.permissions.removeAll(permissions) perActivityRoleRepository.save(foundActivity) - //activityRepository.save(activity) + // activityRepository.save(activity) } fun createNewRole(dto: RoleDto): Role { @@ -79,6 +75,9 @@ class RoleService( val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc() ?: throw IllegalArgumentException(ErrorMessages.noGenerations) + + // need to set it from both sides due to testing transaction + role.generation = latestGeneration roleRepository.save(role) latestGeneration.roles.add(role) generationRepository.save(latestGeneration) @@ -87,9 +86,8 @@ class RoleService( fun deleteRole(roleId: Long) { val role = getRole(roleId) - val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc()!! - latestGeneration.roles.remove(role) - generationRepository.save(latestGeneration) + role.generation.roles.remove(role) + generationRepository.save(role.generation) roleRepository.delete(role) } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 0ef00f7e..591f1b0f 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -180,7 +180,9 @@ internal class RoleControllerTest @Autowired constructor( fun addRole() { generationRepository.save(testGeneration) roleRepository.save(testRole) - testGeneration.roles.add(testRole) + if (testGeneration.roles.size == 0) { + testGeneration.roles.add(testRole) + } generationRepository.save(testGeneration) } @@ -236,9 +238,9 @@ internal class RoleControllerTest @Autowired constructor( ) ) ).andExpectAll( - status().isBadRequest, + status().isUnprocessableEntity, content().contentType(MediaType.APPLICATION_JSON), - jsonPath("$.errors.length()").value(1), + jsonPath("$.errors.length()").value(1) ) .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) assert(roleRepository.findByName(testRole.name) != null) @@ -248,30 +250,33 @@ internal class RoleControllerTest @Autowired constructor( @NestedTest inner class DeleteRole { + lateinit var generation1: Generation + lateinit var role: Role + @BeforeEach fun addRole() { - roleRepository.save(testRole) - generationRepository.save(testGeneration) - generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.add(testRole) + generation1 = Generation("22-23") + role = Role("test-role-1", Permissions(), true) + generation1.roles.add(role) + generationRepository.save(generation1) + role.generation = generation1 } @AfterEach - fun removeRole(){ - roleRepository.delete(testRole); - testGeneration.roles.remove(testRole) + fun removeRole() { + generationRepository.deleteAll() } @Test fun `should remove role with correct id`() { - mockMvc.perform(delete("/roles/${testRole.id}")).andExpectAll( + mockMvc.perform(delete("/roles/${role.id}")).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( documentationRoles, "Removes the role by its id", "The id must exist in order to remove it correctly" ) - assert(roleRepository.findByName(testRole.name) == null) - assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) + assert(roleRepository.findByName(role.name) == null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 0) } @@ -383,7 +388,6 @@ internal class RoleControllerTest @Autowired constructor( ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) - } } From 81faf12f098c0e70b18e9b79c7a9ad5fc732c1ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 5 Jul 2023 14:49:40 +0100 Subject: [PATCH 11/24] fix linting --- src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt index 2ce82ba7..d87580ac 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/model/Generation.kt @@ -9,7 +9,6 @@ import jakarta.persistence.FetchType import jakarta.persistence.GeneratedValue import jakarta.persistence.Id import jakarta.persistence.OneToMany -import jakarta.persistence.OrderColumn import jakarta.validation.Valid import pt.up.fe.ni.website.backend.utils.validation.NoDuplicateRoles import pt.up.fe.ni.website.backend.utils.validation.SchoolYear From b01bb25a30c549ae5a46a829b5875bed515c7d3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 12 Jul 2023 14:15:46 +0100 Subject: [PATCH 12/24] fix docs --- .../backend/controller/RoleControllerTest.kt | 96 ++++++++++--------- 1 file changed, 52 insertions(+), 44 deletions(-) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 591f1b0f..bfaf844a 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -13,7 +13,6 @@ import org.springframework.http.MediaType import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.delete import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.get import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.post -import org.springframework.restdocs.payload.JsonFieldType import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.delete import org.springframework.test.web.servlet.get @@ -37,24 +36,12 @@ import pt.up.fe.ni.website.backend.repository.RoleRepository import pt.up.fe.ni.website.backend.utils.TestUtils import pt.up.fe.ni.website.backend.utils.annotations.ControllerTest import pt.up.fe.ni.website.backend.utils.annotations.NestedTest -import pt.up.fe.ni.website.backend.utils.documentation.Tag import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadAccount import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadPermissions import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadRoles -import pt.up.fe.ni.website.backend.utils.documentation.utils.DocumentedJSONField import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocument import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocumentEmptyObjectResponse import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocumentErrorResponse -import pt.up.fe.ni.website.backend.utils.documentation.utils.ModelDocumentation - -class AccountRoleDocumentation : ModelDocumentation( - Tag.ROLES.name.lowercase() + "-accounts", - Tag.ROLES, - mutableListOf( - DocumentedJSONField("[]", "Array that only contains ONE account id", JsonFieldType.ARRAY) - ) - -) @ControllerTest @Transactional @@ -87,7 +74,7 @@ internal class RoleControllerTest @Autowired constructor( false ) - val testProject = Project( + val project = Project( "UNI", "Melhor app" ) @@ -174,6 +161,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("POST /roles/{id}") inner class CreateNewRole { @BeforeEach @@ -249,6 +237,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("DELETE /roles/{id}") inner class DeleteRole { lateinit var generation1: Generation lateinit var role: Role @@ -294,6 +283,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("POST /roles/{id}/permissions") inner class GrantPermissionToRole { @BeforeEach @@ -343,6 +333,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("DELETE /roles/{id}/permissions") inner class RevokePermissionFromRole { @BeforeEach @@ -392,6 +383,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("POST /roles/{id}/users") inner class AddUserToRole { @BeforeEach fun addRoleAndUser() { @@ -453,6 +445,7 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("DELETE /roles/{id}/users") inner class RemoveUserFromRole { @BeforeEach fun addRoleAndUser() { @@ -516,23 +509,27 @@ internal class RoleControllerTest @Autowired constructor( .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationAccount, + hasRequestPayload = true + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } } @NestedTest + @DisplayName("POST /roles/{id}/activities/{activityId}/permissions") inner class AddPermissionToRoleActivity { @BeforeEach fun addAll() { roleRepository.save(testRole) - projectRepository.save(testProject) + projectRepository.save(project) } @Test fun `should add permission to role activity and create PerActivityRole`() { mockMvc.perform( - post("/roles/${testRole.id}/activities/${testProject.id}/permissions") + post("/roles/${testRole.id}/activities/${project.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -543,7 +540,7 @@ internal class RoleControllerTest @Autowired constructor( status().isOk ).andDocumentEmptyObjectResponse( documentationPermissions, - "Adds an permission to an role activity", + "Adds an permission to a role activity", "It will create a PerRoleActivity if it doesn't exist" ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 1) @@ -557,7 +554,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if roleId is invalid`() { mockMvc.perform( - post("/roles/1234/activities/${testProject.id}/permissions") + post("/roles/1234/activities/${project.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -566,7 +563,10 @@ internal class RoleControllerTest @Autowired constructor( ) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = true + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @@ -582,7 +582,10 @@ internal class RoleControllerTest @Autowired constructor( ) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = true + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @@ -605,29 +608,28 @@ internal class RoleControllerTest @Autowired constructor( } @NestedTest + @DisplayName("DELETE /roles/{id}/activities/{activityId}/permissions") inner class RemovePermissionFromPerRoleActivity { + + private lateinit var project: Project + @BeforeEach fun addAll() { + project = Project("test project", "test") roleRepository.save(testRole) - projectRepository.save(testProject) + projectRepository.save(project) val perActivityRole = PerActivityRole(Permissions(listOf(Permission.EDIT_ACTIVITY))) - perActivityRole.activity = testProject + perActivityRole.activity = project perActivityRole.role = testRole - testProject.associatedRoles.add(perActivityRole) - projectRepository.save(testProject) + project.associatedRoles.add(perActivityRole) + projectRepository.save(project) roleRepository.save(testRole) } - @AfterEach - fun removeAll() { - testProject.associatedRoles.removeAt(0) - projectRepository.save(testProject) - } - @Test fun `should remove an existing role activity permission`() { mockMvc.perform( - delete("/roles/${testRole.id}/activities/${testProject.id}/permissions") + delete("/roles/${testRole.id}/activities/${project.id}/permissions") .contentType(MediaType.APPLICATION_JSON).contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -643,9 +645,9 @@ internal class RoleControllerTest @Autowired constructor( "It will not create a PerRoleActivity if it doesn't exist, it will simply return, " + "doesn't check if the permissions are already revoked..." ) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( - !projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + !projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles[0].permissions.contains( Permission.EDIT_ACTIVITY ) ) @@ -654,7 +656,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing role`() { mockMvc.perform( - delete("/roles/1234/activities/${testProject.id}/permissions") + delete("/roles/1234/activities/${project.id}/permissions") .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -664,10 +666,13 @@ internal class RoleControllerTest @Autowired constructor( ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = false + ) + assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( - projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles[0].permissions.contains( Permission.EDIT_ACTIVITY ) ) @@ -687,16 +692,16 @@ internal class RoleControllerTest @Autowired constructor( ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( - projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles[0].permissions.contains( Permission.EDIT_ACTIVITY ) ) } @Test - fun `should not remove an existing role activity perm on a non existing activity and non existing role`() { + fun `should not remove an existing role permission when neither the activity and role don't exist`() { mockMvc.perform( delete("/roles/1234/activities/1234/permissions") .contentType(MediaType.APPLICATION_JSON) @@ -708,10 +713,13 @@ internal class RoleControllerTest @Autowired constructor( ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) - assert(projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles.size == 1) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = false + ) + assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( - projectRepository.findByIdOrNull(testProject.id!!)!!.associatedRoles[0].permissions.contains( + projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles[0].permissions.contains( Permission.EDIT_ACTIVITY ) ) From 95f3cabcdfc9903887e4fc1264eb7459cdf7ac52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 12 Jul 2023 15:03:17 +0100 Subject: [PATCH 13/24] fix docs again --- .../backend/controller/RoleControllerTest.kt | 199 +++++++++++++----- 1 file changed, 143 insertions(+), 56 deletions(-) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index bfaf844a..9f5a2cbb 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -1,5 +1,6 @@ package pt.up.fe.ni.website.backend.controller +import com.epages.restdocs.apispec.ResourceDocumentation.parameterWithName import com.fasterxml.jackson.databind.ObjectMapper import jakarta.transaction.Transactional import java.util.* @@ -36,7 +37,6 @@ import pt.up.fe.ni.website.backend.repository.RoleRepository import pt.up.fe.ni.website.backend.utils.TestUtils import pt.up.fe.ni.website.backend.utils.annotations.ControllerTest import pt.up.fe.ni.website.backend.utils.annotations.NestedTest -import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadAccount import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadPermissions import pt.up.fe.ni.website.backend.utils.documentation.payloadschemas.model.PayloadRoles import pt.up.fe.ni.website.backend.utils.documentation.utils.MockMVCExtension.Companion.andDocument @@ -59,8 +59,6 @@ internal class RoleControllerTest @Autowired constructor( val documentationRoles = PayloadRoles() - val documentationAccount = PayloadAccount() - val documentationPermissions = PayloadPermissions() val testRole = Role( @@ -131,6 +129,9 @@ internal class RoleControllerTest @Autowired constructor( @NestedTest @DisplayName("GET /roles/{id}") inner class GetSpecificRole { + + private val parameters = listOf(parameterWithName("id").description("ID of the role to retrieve")) + @BeforeEach fun addRole() { roleRepository.save(testRole) @@ -138,30 +139,35 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should return testRole when provided by its id`() { - mockMvc.perform(get("/roles/${testRole.id}")).andExpectAll( + mockMvc.perform(get("/roles/{id}", testRole.id)).andExpectAll( status().isOk, content().contentType(MediaType.APPLICATION_JSON), content().json(objectMapper.writeValueAsString(testRole)) ).andDocument( documentationRoles, "Returns a specific role", - "Returns a summary brief of a specific role, which makes getting one role way more efficient." + "Returns a summary brief of a specific role, which makes getting one role way more efficient.", + urlParameters = parameters ) } @Test fun `should return error on invalid roleID`() { - mockMvc.perform(get("/roles/4020")).andExpectAll( + mockMvc.perform(get("/roles/{id}", 4020)).andExpectAll( status().isNotFound(), content().contentType(MediaType.APPLICATION_JSON), jsonPath("$.errors.length()").value(1), jsonPath("$.errors.[0].message").value("role not found with id 4020") - ).andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters + ) } } @NestedTest - @DisplayName("POST /roles/{id}") + @DisplayName("POST /roles") inner class CreateNewRole { @BeforeEach @@ -202,6 +208,7 @@ internal class RoleControllerTest @Autowired constructor( documentationRoles, "This creates a role, returning the complete role information", "It will only create the role if it no other role with the same name exists in that generation" + ) assert(roleRepository.count().toInt() == 2) assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) @@ -239,6 +246,8 @@ internal class RoleControllerTest @Autowired constructor( @NestedTest @DisplayName("DELETE /roles/{id}") inner class DeleteRole { + private val parameters = listOf(parameterWithName("id").description("ID of the role to delete")) + lateinit var generation1: Generation lateinit var role: Role @@ -258,33 +267,37 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove role with correct id`() { - mockMvc.perform(delete("/roles/${role.id}")).andExpectAll( + mockMvc.perform(delete("/roles/{id}", role.id)).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( documentationRoles, "Removes the role by its id", - "The id must exist in order to remove it correctly" + "The id must exist in order to remove it correctly", + urlParameters = parameters ) assert(roleRepository.findByName(role.name) == null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 0) } @Test - @Transactional fun `should not remove role if id does not exist`() { - mockMvc.perform(delete("/roles/1234")).andExpectAll( + mockMvc.perform(delete("/roles/{id}", 1234)).andExpectAll( status().isNotFound(), content().contentType(MediaType.APPLICATION_JSON), jsonPath("$.errors.length()").value(1), jsonPath("$.errors.[0].message").value("role not found with id 1234") ) - .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) + .andDocumentErrorResponse( + documentationRoles, + urlParameters = parameters + ) } } @NestedTest @DisplayName("POST /roles/{id}/permissions") inner class GrantPermissionToRole { + private val parameters = listOf(parameterWithName("id").description("ID of the role to grant permissions to")) @BeforeEach fun addRole() { @@ -294,7 +307,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should grant permission to role that exists`() { mockMvc.perform( - post("/roles/${testRole.id}/permissions") + post("/roles/{id}/permissions", testRole.id) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -309,7 +322,9 @@ internal class RoleControllerTest @Autowired constructor( .andDocumentEmptyObjectResponse( documentationPermissions, "Adds a set of permissions to a role by ID", - "This doesn't check if the permission is already added, the role ID must be valid" + "This doesn't check if the permission is already added, the role ID must be valid", + + urlParameters = parameters ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) } @@ -317,7 +332,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not grant permission to role that does not exists`() { mockMvc.perform( - post("/roles/1234/permissions") + post("/roles/{id}/permissions", 1234) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -328,13 +343,19 @@ internal class RoleControllerTest @Autowired constructor( ) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) + ).andDocumentErrorResponse( + documentationPermissions, + urlParameters = parameters + ) } } @NestedTest @DisplayName("DELETE /roles/{id}/permissions") inner class RevokePermissionFromRole { + private val parameters = listOf( + parameterWithName("id").description("ID of the role to revoke permissions from") + ) @BeforeEach fun addRole() { @@ -344,7 +365,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should revoke permission from role that exists`() { mockMvc.perform( - delete("/roles/${testRole.id}/permissions") + delete("/roles/{id}/permissions", testRole.id) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -359,7 +380,9 @@ internal class RoleControllerTest @Autowired constructor( ).andDocumentEmptyObjectResponse( documentationPermissions, "Revokes permission by role ID", - "Revokes permissions, it doesn't check if the role contains them." + "Revokes permissions, it doesn't check if the role contains them.", + + urlParameters = parameters ) assert(!roleRepository.findByIdOrNull(testRole.id!!)!!.permissions.contains(Permission.SUPERUSER)) } @@ -367,7 +390,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not revoke permission from role that does not exist`() { mockMvc.perform( - delete("/roles/1234/permissions") + delete("/roles/{id}/permissions", 1234) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -378,13 +401,19 @@ internal class RoleControllerTest @Autowired constructor( ) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = true, + urlParameters = parameters + ) } } @NestedTest @DisplayName("POST /roles/{id}/users") inner class AddUserToRole { + private val parameters = listOf(parameterWithName("id").description("ID of the role to add an user")) + @BeforeEach fun addRoleAndUser() { roleRepository.save(testRole) @@ -394,15 +423,17 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should add an existing account to an existing role`() { mockMvc.perform( - post("/roles/${testRole.id}/users") + post("/roles/{id}/users", testRole.id) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( - documentationAccount, + documentationRoles, "Add an account to a role by its IDs", - "It will return an error if the user already has the role or if it doesn't exist" + "It will return an error if the user already has the role or if it doesn't exist", + hasRequestPayload = true, + urlParameters = parameters ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -410,36 +441,48 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not add an non existing account to an existing role`() { mockMvc.perform( - post("/roles/${testRole.id}/users") + post("/roles/{id}/users", testRole.id) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @Test fun `should not add an non existing account to an non existing role`() { mockMvc.perform( - post("/roles/1234/users") + post("/roles/{id}/users", 1234) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @Test fun `should not add an existing account to an non existing role`() { mockMvc.perform( - post("/roles/1234/users") + post("/roles/{id}/users", 1234) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } } @@ -447,6 +490,8 @@ internal class RoleControllerTest @Autowired constructor( @NestedTest @DisplayName("DELETE /roles/{id}/users") inner class RemoveUserFromRole { + private val parameters = listOf(parameterWithName("id").description("ID of the role to remove an user from")) + @BeforeEach fun addRoleAndUser() { accountRepository.save(testAccount) @@ -464,15 +509,17 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove an existing account from an existing role`() { mockMvc.perform( - delete("/roles/${testRole.id}/users") + delete("/roles/{id}/users", testRole.id) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( - documentationAccount, + documentationRoles, "Removes the account from the role by ID", - "It only works if the user has the role and it exists" + "It only works if the user has the role and it exists", + hasRequestPayload = true, + urlParameters = parameters ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size == 0) } @@ -480,38 +527,47 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove a non existing account from an existing role`() { mockMvc.perform( - delete("/roles/${testRole.id}/users") + delete("/roles/{id}/users", testRole.id) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @Test fun `should not remove an non existing account to an non existing role`() { mockMvc.perform( - delete("/roles/1234/users") + delete("/roles/{id}/users", 1234) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationAccount, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @Test fun `should not remove an existing account to an non existing role`() { mockMvc.perform( - delete("/roles/1234/users") + delete("/roles/{id}/users", 1234) .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse( - documentationAccount, - hasRequestPayload = true + documentationRoles, + hasRequestPayload = true, + urlParameters = parameters ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -520,6 +576,11 @@ internal class RoleControllerTest @Autowired constructor( @NestedTest @DisplayName("POST /roles/{id}/activities/{activityId}/permissions") inner class AddPermissionToRoleActivity { + private val parameters = listOf( + parameterWithName("id").description("ID of the role"), + parameterWithName("activityId").description("ID of the activity") + ) + @BeforeEach fun addAll() { roleRepository.save(testRole) @@ -529,7 +590,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should add permission to role activity and create PerActivityRole`() { mockMvc.perform( - post("/roles/${testRole.id}/activities/${project.id}/permissions") + post("/roles/{id}/activities/{activityId}/permissions", testRole.id, project.id) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -541,7 +602,9 @@ internal class RoleControllerTest @Autowired constructor( ).andDocumentEmptyObjectResponse( documentationPermissions, "Adds an permission to a role activity", - "It will create a PerRoleActivity if it doesn't exist" + "It will create a PerRoleActivity if it doesn't exist", + hasRequestPayload = true, + urlParameters = parameters ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 1) assert( @@ -554,7 +617,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if roleId is invalid`() { mockMvc.perform( - post("/roles/1234/activities/${project.id}/permissions") + post("/roles/{id}/activities/{activityId}/permissions", 1234, project.id) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -565,7 +628,9 @@ internal class RoleControllerTest @Autowired constructor( status().isNotFound() ).andDocumentErrorResponse( documentationPermissions, - hasRequestPayload = true + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @@ -573,7 +638,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if activityId is invalid`() { mockMvc.perform( - post("/roles/${testRole.id}/activities/1234/permissions") + post("/roles/{id}/activities/{activityId}/permissions", testRole.id, 1234) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -584,7 +649,9 @@ internal class RoleControllerTest @Autowired constructor( status().isNotFound() ).andDocumentErrorResponse( documentationPermissions, - hasRequestPayload = true + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @@ -592,7 +659,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `shouldn't add permission to role activity if activityId is invalid and roleId is invalid`() { mockMvc.perform( - post("/roles/${testRole.id}/activities/1234/permissions") + post("/roles/{id}/activities/{activityId}/permissions", 1234, 1234) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -602,7 +669,11 @@ internal class RoleControllerTest @Autowired constructor( ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = true) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = true, + urlParameters = parameters + ) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } } @@ -610,6 +681,10 @@ internal class RoleControllerTest @Autowired constructor( @NestedTest @DisplayName("DELETE /roles/{id}/activities/{activityId}/permissions") inner class RemovePermissionFromPerRoleActivity { + private val parameters = listOf( + parameterWithName("id").description("ID of the role"), + parameterWithName("activityId").description("ID of the activity") + ) private lateinit var project: Project @@ -629,7 +704,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove an existing role activity permission`() { mockMvc.perform( - delete("/roles/${testRole.id}/activities/${project.id}/permissions") + delete("/roles/{id}/activities/{activityId}/permissions", testRole.id, project.id) .contentType(MediaType.APPLICATION_JSON).contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -643,7 +718,10 @@ internal class RoleControllerTest @Autowired constructor( documentationPermissions, "Removes an PerRoleActivity permission by ID", "It will not create a PerRoleActivity if it doesn't exist, it will simply return, " + - "doesn't check if the permissions are already revoked..." + "doesn't check if the permissions are already revoked...", + hasRequestPayload = true, + urlParameters = parameters + ) assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( @@ -656,7 +734,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing role`() { mockMvc.perform( - delete("/roles/1234/activities/${project.id}/permissions") + delete("/roles/{id}/activities/{activityId}/permissions", 1234, project.id) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -668,7 +746,9 @@ internal class RoleControllerTest @Autowired constructor( status().isNotFound() ).andDocumentErrorResponse( documentationPermissions, - hasRequestPayload = false + hasRequestPayload = true, + urlParameters = parameters + ) assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( @@ -681,7 +761,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role activity permission on a non existing activity`() { mockMvc.perform( - delete("/roles/${testRole.id}/activities/1234/permissions") + delete("/roles/{id}/activities/{activityId}/permissions", testRole.id, 1234) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( @@ -691,7 +771,11 @@ internal class RoleControllerTest @Autowired constructor( ).andExpectAll( status().isNotFound() - ).andDocumentErrorResponse(documentationPermissions, hasRequestPayload = false) + ).andDocumentErrorResponse( + documentationPermissions, + hasRequestPayload = true, + urlParameters = parameters + ) assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles[0].permissions.contains( @@ -703,19 +787,22 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should not remove an existing role permission when neither the activity and role don't exist`() { mockMvc.perform( - delete("/roles/1234/activities/1234/permissions") + delete("/roles/{id}/activities/{activityId}/permissions", 1234, 1234) .contentType(MediaType.APPLICATION_JSON) .content( objectMapper.writeValueAsString( mapOf("permissions" to Permissions(listOf(Permission.EDIT_ACTIVITY))) ) + ) ).andExpectAll( status().isNotFound() ).andDocumentErrorResponse( documentationPermissions, - hasRequestPayload = false + hasRequestPayload = true, + urlParameters = parameters + ) assert(projectRepository.findByIdOrNull(project.id!!)!!.associatedRoles.size == 1) assert( From 786e581a7fc6d2d73100aa59b464c1062998df6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 12 Jul 2023 17:41:55 +0100 Subject: [PATCH 14/24] remove httpsecurity iframe bypass (due to h2 dashboard) --- .../pt/up/fe/ni/website/backend/config/security/AuthConfig.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt index e9b941d1..7f1939a8 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/config/security/AuthConfig.kt @@ -30,7 +30,6 @@ class AuthConfig( ) { @Bean fun securityFilterChain(http: HttpSecurity): SecurityFilterChain { - http.headers().frameOptions().disable() return http.csrf { csrf -> csrf.disable() }.cors().and() .oauth2ResourceServer().jwt() .jwtAuthenticationConverter(rolesConverter()) From ac1b3a73042d8ef785c6aab293f5fc00ac27ee12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Tue, 18 Jul 2023 11:39:16 +0100 Subject: [PATCH 15/24] change dto file structure --- .../pt/up/fe/ni/website/backend/controller/RoleController.kt | 4 ++-- .../kotlin/pt/up/fe/ni/website/backend/dto/auth/UserIdDto.kt | 5 +++++ .../pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt | 5 ----- .../backend/dto/{permissions => roles}/PermissionsDto.kt | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) create mode 100644 src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/UserIdDto.kt delete mode 100644 src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt rename src/main/kotlin/pt/up/fe/ni/website/backend/dto/{permissions => roles}/PermissionsDto.kt (71%) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 5e94e94b..299d2970 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -7,9 +7,9 @@ import org.springframework.web.bind.annotation.PostMapping import org.springframework.web.bind.annotation.RequestBody import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RestController +import pt.up.fe.ni.website.backend.dto.auth.UserIdDto import pt.up.fe.ni.website.backend.dto.entity.RoleDto -import pt.up.fe.ni.website.backend.dto.permissions.PermissionsDto -import pt.up.fe.ni.website.backend.dto.permissions.UserIdDto +import pt.up.fe.ni.website.backend.dto.roles.PermissionsDto import pt.up.fe.ni.website.backend.model.permissions.Permissions import pt.up.fe.ni.website.backend.service.RoleService diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/UserIdDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/UserIdDto.kt new file mode 100644 index 00000000..1c4a036f --- /dev/null +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/UserIdDto.kt @@ -0,0 +1,5 @@ +package pt.up.fe.ni.website.backend.dto.auth + +data class UserIdDto( + val userId: Long +) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt deleted file mode 100644 index e1cb3fde..00000000 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/UserIdDto.kt +++ /dev/null @@ -1,5 +0,0 @@ -package pt.up.fe.ni.website.backend.dto.permissions - -data class UserIdDto( - val userId: Long -) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/roles/PermissionsDto.kt similarity index 71% rename from src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt rename to src/main/kotlin/pt/up/fe/ni/website/backend/dto/roles/PermissionsDto.kt index 3f2c3235..a7e4ea4a 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/permissions/PermissionsDto.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/roles/PermissionsDto.kt @@ -1,4 +1,4 @@ -package pt.up.fe.ni.website.backend.dto.permissions +package pt.up.fe.ni.website.backend.dto.roles import pt.up.fe.ni.website.backend.model.permissions.Permissions From 75927202b08e66cdce76fbcfa5c1624e0144e063 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 19 Jul 2023 10:50:46 +0100 Subject: [PATCH 16/24] Make role creation accept a past generation --- .../backend/controller/RoleController.kt | 2 +- .../ni/website/backend/dto/entity/RoleDto.kt | 4 +- .../website/backend/service/ErrorMessages.kt | 3 +- .../ni/website/backend/service/RoleService.kt | 30 ++++++----- .../backend/controller/RoleControllerTest.kt | 54 ++++++++++++++++--- .../fe/ni/website/backend/utils/TestUtils.kt | 11 ++++ .../payloadschemas/model/PayloadRoles.kt | 7 +++ 7 files changed, 88 insertions(+), 23 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 299d2970..89c48bf7 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -67,7 +67,7 @@ class RoleController(private val roleService: RoleService) { roleService.grantPermissionToRoleOnActivity( id, activityId, - Permissions(permissionsDto.permissions) + permissionsDto.permissions ) return emptyMap() } diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt index dd36aa28..77e76963 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt @@ -11,5 +11,7 @@ class RoleDto( val isSection: Boolean?, val accountIds: List = emptyList(), - val associatedActivities: List + val associatedActivities: List, + + val generationId: Long? = null ) : EntityDto() diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt index ab5da93c..ca43d82c 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt @@ -17,7 +17,8 @@ object ErrorMessages { const val generationAlreadyExists = "generation already exists" - const val roleAlreadyExists = "role already exists" + fun roleAlreadyExists(roleName: String, generationYear: String): String = + "role $roleName already exists in $generationYear" fun postNotFound(postId: Long): String = "post not found with id $postId" diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 97db02c5..6c93e628 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -1,8 +1,11 @@ package pt.up.fe.ni.website.backend.service import jakarta.transaction.Transactional +import jakarta.validation.Validator import org.springframework.stereotype.Service +import pt.up.fe.ni.website.backend.config.ApplicationContextUtils import pt.up.fe.ni.website.backend.dto.entity.RoleDto +import pt.up.fe.ni.website.backend.model.Generation import pt.up.fe.ni.website.backend.model.PerActivityRole import pt.up.fe.ni.website.backend.model.Role import pt.up.fe.ni.website.backend.model.permissions.Permissions @@ -53,7 +56,6 @@ class RoleService( if (activity.associatedRoles.find { it.activity == activity } == null) { role.associatedActivities.add(foundPerActivityRole) } - // activityRepository.save(activity) roleRepository.save(role) } @@ -64,23 +66,27 @@ class RoleService( .find { it.role == role } ?: return foundActivity.permissions.removeAll(permissions) perActivityRoleRepository.save(foundActivity) - // activityRepository.save(activity) } fun createNewRole(dto: RoleDto): Role { - if (roleRepository.findByName(dto.name) != null) { - throw IllegalArgumentException(ErrorMessages.roleAlreadyExists) - } val role = dto.create() + val generation: Generation = if (dto.generationId != null) { + generationRepository.findById(dto.generationId).orElseThrow { + IllegalArgumentException(ErrorMessages.generationNotFound(dto.generationId)) + } + } else { + generationRepository.findFirstByOrderBySchoolYearDesc() + ?: throw IllegalArgumentException(ErrorMessages.noGenerations) + } - val latestGeneration = generationRepository.findFirstByOrderBySchoolYearDesc() - ?: throw IllegalArgumentException(ErrorMessages.noGenerations) - - // need to set it from both sides due to testing transaction - role.generation = latestGeneration + role.generation = generation + generation.roles.add(role) + val validator: Validator = ApplicationContextUtils.getBean(Validator::class.java) + // we can infer that if something goes wrong is with adding a new role with the same name + if (validator.validate(generation).isNotEmpty()) { + throw IllegalArgumentException(ErrorMessages.roleAlreadyExists(role.name, generation.schoolYear)) + } roleRepository.save(role) - latestGeneration.roles.add(role) - generationRepository.save(latestGeneration) return role } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 9f5a2cbb..2b9dac3c 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -57,6 +57,10 @@ internal class RoleControllerTest @Autowired constructor( "22-23" ) + val lastYearGeneration = Generation( + "21-22" + ) + val documentationRoles = PayloadRoles() val documentationPermissions = PayloadPermissions() @@ -172,12 +176,11 @@ internal class RoleControllerTest @Autowired constructor( @BeforeEach fun addRole() { + testRole.generation = testGeneration generationRepository.save(testGeneration) + generationRepository.save(lastYearGeneration) roleRepository.save(testRole) - if (testGeneration.roles.size == 0) { - testGeneration.roles.add(testRole) - } - generationRepository.save(testGeneration) + TestUtils.startNewTransaction() } @Test @@ -211,7 +214,6 @@ internal class RoleControllerTest @Autowired constructor( ) assert(roleRepository.count().toInt() == 2) - assert(generationRepository.findFirstByOrderBySchoolYearDesc() != null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 2) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles[1].id!!.compareTo(2) == 0) } @@ -238,8 +240,44 @@ internal class RoleControllerTest @Autowired constructor( jsonPath("$.errors.length()").value(1) ) .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByName(testRole.name) != null) - assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 2) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size != 2) + } + + @Test + fun `should add a role with an existing name but on a different generation`() { + mockMvc.perform( + post("/roles") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "name" to testRole.name, + "permissions" to testRole.permissions.map { it.bit }.toList(), + "isSection" to testRole.isSection, + "associatedActivities" to testRole.associatedActivities, + "accounts" to testRole.accounts, + "generationId" to lastYearGeneration.id + ) + ) + ) + ).andExpectAll( + status().isOk, + content().contentType(MediaType.APPLICATION_JSON), + jsonPath("$.name").value("TestRole"), + jsonPath("$.id").value(2), // this must be hardcoded + jsonPath("$.permissions.length()").value(1), + jsonPath("$.isSection").value(false) + ).andDocument( + documentationRoles, + "This creates a role on a specified generation, returning the complete role information", + "It will only create the role if it no other role with the same name exists in that generation", + hasRequestPayload = true + ) + TestUtils.startNewTransaction() + assert(generationRepository.findById(lastYearGeneration.id!!).orElseThrow().roles.size == 1) + assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 1) } } @@ -248,8 +286,8 @@ internal class RoleControllerTest @Autowired constructor( inner class DeleteRole { private val parameters = listOf(parameterWithName("id").description("ID of the role to delete")) - lateinit var generation1: Generation - lateinit var role: Role + private lateinit var generation1: Generation + private lateinit var role: Role @BeforeEach fun addRole() { diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/TestUtils.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/TestUtils.kt index fafc608e..4d5289f0 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/TestUtils.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/TestUtils.kt @@ -3,6 +3,7 @@ package pt.up.fe.ni.website.backend.utils import java.util.Calendar import java.util.Date import java.util.TimeZone +import org.springframework.test.context.transaction.TestTransaction class TestUtils { companion object { @@ -11,5 +12,15 @@ class TestUtils { .apply { set(year, month, day, 0, 0, 0) } .time } + + fun startNewTransaction(rollback: Boolean = false) { + if (rollback) { + TestTransaction.flagForRollback() + } else { + TestTransaction.flagForCommit() + } + TestTransaction.end() + TestTransaction.start() + } } } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt index e738e1e8..25b7fca7 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt @@ -17,6 +17,13 @@ class PayloadRoles : ModelDocumentation( "associatedActivities", "List of activities that are associated with this role", JsonFieldType.ARRAY + ), + DocumentedJSONField( + "generation", + "ID of generation that this role should be added to, this is optional and if not specified it " + + "defaults to the latest generation", + JsonFieldType.NUMBER, + optional = true ) ) ) From 90adf692e11324bc934227341570a7b7f4027d40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 19 Jul 2023 11:05:18 +0000 Subject: [PATCH 17/24] Remove orderColumn from activity, make add remove and remove user idempotent and make code cleaner --- .../fe/ni/website/backend/model/Activity.kt | 2 -- .../ni/website/backend/service/RoleService.kt | 21 ++++++------------- .../backend/controller/RoleControllerTest.kt | 8 ++++++- .../payloadschemas/model/PayloadRoles.kt | 3 ++- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/model/Activity.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/model/Activity.kt index 7e446eb1..3c6016f0 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/model/Activity.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/model/Activity.kt @@ -12,7 +12,6 @@ import jakarta.persistence.Inheritance import jakarta.persistence.InheritanceType import jakarta.persistence.JoinColumn import jakarta.persistence.OneToMany -import jakarta.persistence.OrderColumn import jakarta.validation.Valid import jakarta.validation.constraints.Size import pt.up.fe.ni.website.backend.model.constants.ActivityConstants as Constants @@ -33,7 +32,6 @@ abstract class Activity( open val teamMembers: MutableList, @OneToMany(cascade = [CascadeType.ALL], mappedBy = "activity") - @OrderColumn @JsonIgnore // TODO: Decide if we want to return perRoles (or IDs) by default open val associatedRoles: MutableList<@Valid PerActivityRole> = mutableListOf(), diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 6c93e628..4794021a 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -46,17 +46,13 @@ class RoleService( fun grantPermissionToRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { val activity = activityService.getActivityById(activityId) val role = getRole(roleId) - val foundPerActivityRole = activity.associatedRoles + val perActivityRole = activity.associatedRoles .find { it.activity == activity } ?: PerActivityRole(Permissions()) - foundPerActivityRole.role = role - foundPerActivityRole.activity = activity + perActivityRole.role = role + perActivityRole.activity = activity - foundPerActivityRole.permissions.addAll(permissions) - perActivityRoleRepository.save(foundPerActivityRole) - if (activity.associatedRoles.find { it.activity == activity } == null) { - role.associatedActivities.add(foundPerActivityRole) - } - roleRepository.save(role) + perActivityRole.permissions.addAll(permissions) + perActivityRoleRepository.save(perActivityRole) } fun revokePermissionFromRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { @@ -100,9 +96,7 @@ class RoleService( fun addUserToRole(roleId: Long, userId: Long) { val role = getRole(roleId) val account = accountService.getAccountById(userId) - role.accounts.find { it.id == account.id }.let { - if (it != null) throw NoSuchElementException(ErrorMessages.userAlreadyHasRole(roleId, userId)) - } + if (role.accounts.any { it.id == account.id }) return role.accounts.add(account) roleRepository.save(role) } @@ -110,9 +104,6 @@ class RoleService( fun removeUserFromRole(roleId: Long, userId: Long) { val role = getRole(roleId) val account = accountService.getAccountById(userId) - role.accounts.find { it.id == account.id }.let { - if (it == null) throw NoSuchElementException(ErrorMessages.userNotInRole(roleId, userId)) - } role.accounts.remove(account) roleRepository.save(role) } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 2b9dac3c..49355294 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -469,7 +469,8 @@ internal class RoleControllerTest @Autowired constructor( ).andDocumentEmptyObjectResponse( documentationRoles, "Add an account to a role by its IDs", - "It will return an error if the user already has the role or if it doesn't exist", + "It's an idempotent endpoint so it will return HTTP code 200 even if the user is already " + + "in the role", hasRequestPayload = true, urlParameters = parameters ) @@ -623,6 +624,7 @@ internal class RoleControllerTest @Autowired constructor( fun addAll() { roleRepository.save(testRole) projectRepository.save(project) + TestUtils.startNewTransaction() } @Test @@ -644,6 +646,7 @@ internal class RoleControllerTest @Autowired constructor( hasRequestPayload = true, urlParameters = parameters ) + TestUtils.startNewTransaction() assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 1) assert( roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities[0].permissions.contains( @@ -670,6 +673,7 @@ internal class RoleControllerTest @Autowired constructor( urlParameters = parameters ) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @@ -691,6 +695,7 @@ internal class RoleControllerTest @Autowired constructor( urlParameters = parameters ) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } @@ -712,6 +717,7 @@ internal class RoleControllerTest @Autowired constructor( hasRequestPayload = true, urlParameters = parameters ) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.associatedActivities.size == 0) } } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt index 25b7fca7..b81a464e 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadRoles.kt @@ -15,7 +15,8 @@ class PayloadRoles : ModelDocumentation( DocumentedJSONField("id", "Internal ID of role", JsonFieldType.NUMBER), DocumentedJSONField( "associatedActivities", - "List of activities that are associated with this role", + "List of PerActivityRoles that are associated with activities. These roles can have " + + "more permissions specific to that activity. (eg.: edit the specific event page)", JsonFieldType.ARRAY ), DocumentedJSONField( From 526b5b8aeaebc6aa63b3ab48c732785c68ee8137 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 19 Jul 2023 11:20:57 +0000 Subject: [PATCH 18/24] remove role repo .findByName --- .../pt/up/fe/ni/website/backend/repository/RoleRepository.kt | 4 +--- .../fe/ni/website/backend/controller/RoleControllerTest.kt | 5 +++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt index c59bdf7e..1898b9f7 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/repository/RoleRepository.kt @@ -5,6 +5,4 @@ import org.springframework.stereotype.Repository import pt.up.fe.ni.website.backend.model.Role @Repository -interface RoleRepository : CrudRepository { - fun findByName(name: String): Role? -} +interface RoleRepository : CrudRepository diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 49355294..e4703cea 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -241,7 +241,7 @@ internal class RoleControllerTest @Autowired constructor( ) .andDocumentErrorResponse(documentationRoles, hasRequestPayload = true) TestUtils.startNewTransaction(rollback = true) - assert(roleRepository.findByName(testRole.name) != null) + assert(roleRepository.findByIdOrNull(testRole.id!!) != null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size != 2) } @@ -305,6 +305,7 @@ internal class RoleControllerTest @Autowired constructor( @Test fun `should remove role with correct id`() { + val id: Long = role.id!! mockMvc.perform(delete("/roles/{id}", role.id)).andExpectAll( status().isOk ).andDocumentEmptyObjectResponse( @@ -313,7 +314,7 @@ internal class RoleControllerTest @Autowired constructor( "The id must exist in order to remove it correctly", urlParameters = parameters ) - assert(roleRepository.findByName(role.name) == null) + assert(roleRepository.findByIdOrNull(id) == null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 0) } From b1c05c434054f7234fae80971369400c9d62f6a8 Mon Sep 17 00:00:00 2001 From: Luis Duarte Date: Wed, 26 Jul 2023 15:16:43 +0100 Subject: [PATCH 19/24] make code suggestions --- .../backend/controller/RoleController.kt | 2 +- .../backend/service/GenerationService.kt | 14 ++++-- .../ni/website/backend/service/RoleService.kt | 43 ++++++++----------- .../controller/GenerationControllerTest.kt | 3 +- .../backend/controller/RoleControllerTest.kt | 30 +++++++------ 5 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 89c48bf7..9a5417a1 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -20,7 +20,7 @@ class RoleController(private val roleService: RoleService) { fun getAllRoles() = roleService.getAllRoles() @GetMapping("/{id}") - fun getRole(@PathVariable id: Long) = roleService.getRole(id) + fun getRole(@PathVariable id: Long) = roleService.getRoleById(id) @PostMapping fun createNewRole(@RequestBody dto: RoleDto) = roleService.createNewRole(dto) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/GenerationService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/GenerationService.kt index bcf9abbf..5cf6e985 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/GenerationService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/GenerationService.kt @@ -27,6 +27,16 @@ class GenerationService( return buildGetGenerationDto(generation) } + fun getGenerationByIdOrInferLatest(id: Long?): Generation { + return if (id != null) { + repository.findById(id).orElseThrow { + NoSuchElementException(ErrorMessages.generationNotFound(id)) + } + } else { + repository.findFirstByOrderBySchoolYearDesc() + ?: throw IllegalArgumentException(ErrorMessages.noGenerations) + } + } fun getGenerationByYear(year: String): GetGenerationDto { val generation = repository.findBySchoolYear(year) ?: throw NoSuchElementException(ErrorMessages.generationNotFound(year)) @@ -96,9 +106,7 @@ class GenerationService( roleDto.accountIds.forEach { val account = accountService.getAccountById(it) - // only owner side is needed after transaction, but it's useful to update the objects account.roles.add(role) - role.accounts.add(account) } roleDto.associatedActivities.forEachIndexed associatedLoop@{ activityRoleIdx, activityRoleDto -> @@ -106,9 +114,7 @@ class GenerationService( val activityId = activityRoleDto.activityId ?: return@associatedLoop val activity = activityService.getActivityById(activityId) - // only owner side is needed after transaction, but it's useful to update the objects perActivityRole.activity = activity - activity.associatedRoles.add(perActivityRole) } } } diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 4794021a..09b6d189 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -3,9 +3,7 @@ package pt.up.fe.ni.website.backend.service import jakarta.transaction.Transactional import jakarta.validation.Validator import org.springframework.stereotype.Service -import pt.up.fe.ni.website.backend.config.ApplicationContextUtils import pt.up.fe.ni.website.backend.dto.entity.RoleDto -import pt.up.fe.ni.website.backend.model.Generation import pt.up.fe.ni.website.backend.model.PerActivityRole import pt.up.fe.ni.website.backend.model.Role import pt.up.fe.ni.website.backend.model.permissions.Permissions @@ -19,12 +17,14 @@ import pt.up.fe.ni.website.backend.service.activity.ActivityService class RoleService( private val roleRepository: RoleRepository, private val perActivityRoleRepository: PerActivityRoleRepository, - private val generationRepository: GenerationRepository, + private val generationService: GenerationService, private val accountService: AccountService, - private val activityService: ActivityService + private val activityService: ActivityService, + private val generationRepository: GenerationRepository, + private val validator: Validator ) { - fun getRole(roleId: Long): Role { + fun getRoleById(roleId: Long): Role { val role = roleRepository.findById(roleId).orElseThrow { throw NoSuchElementException(ErrorMessages.roleNotFound(roleId)) } @@ -32,20 +32,20 @@ class RoleService( } fun getAllRoles(): List = roleRepository.findAll().toList() fun grantPermissionToRole(roleId: Long, permissions: Permissions) { - val role = getRole(roleId) + val role = getRoleById(roleId) role.permissions.addAll(permissions) roleRepository.save(role) } fun revokePermissionFromRole(roleId: Long, permissions: Permissions) { - val role = getRole(roleId) + val role = getRoleById(roleId) role.permissions.removeAll(permissions) roleRepository.save(role) } fun grantPermissionToRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { val activity = activityService.getActivityById(activityId) - val role = getRole(roleId) + val role = getRoleById(roleId) val perActivityRole = activity.associatedRoles .find { it.activity == activity } ?: PerActivityRole(Permissions()) perActivityRole.role = role @@ -57,7 +57,7 @@ class RoleService( fun revokePermissionFromRoleOnActivity(roleId: Long, activityId: Long, permissions: Permissions) { val activity = activityService.getActivityById(activityId) - val role = getRole(roleId) + val role = getRoleById(roleId) val foundActivity = activity.associatedRoles .find { it.role == role } ?: return foundActivity.permissions.removeAll(permissions) @@ -66,43 +66,34 @@ class RoleService( fun createNewRole(dto: RoleDto): Role { val role = dto.create() - val generation: Generation = if (dto.generationId != null) { - generationRepository.findById(dto.generationId).orElseThrow { - IllegalArgumentException(ErrorMessages.generationNotFound(dto.generationId)) - } - } else { - generationRepository.findFirstByOrderBySchoolYearDesc() - ?: throw IllegalArgumentException(ErrorMessages.noGenerations) - } + val generation = generationService.getGenerationByIdOrInferLatest(dto.generationId) - role.generation = generation - generation.roles.add(role) - val validator: Validator = ApplicationContextUtils.getBean(Validator::class.java) - // we can infer that if something goes wrong is with adding a new role with the same name - if (validator.validate(generation).isNotEmpty()) { + generation.roles.add(role) // just for validation and will not be persisted + if (validator.validateProperty(generation, "roles").isNotEmpty()) { throw IllegalArgumentException(ErrorMessages.roleAlreadyExists(role.name, generation.schoolYear)) } + role.generation = generation roleRepository.save(role) return role } fun deleteRole(roleId: Long) { - val role = getRole(roleId) + val role = getRoleById(roleId) role.generation.roles.remove(role) generationRepository.save(role.generation) roleRepository.delete(role) } fun addUserToRole(roleId: Long, userId: Long) { - val role = getRole(roleId) + val role = getRoleById(roleId) val account = accountService.getAccountById(userId) if (role.accounts.any { it.id == account.id }) return - role.accounts.add(account) + account.roles.add(role) roleRepository.save(role) } fun removeUserFromRole(roleId: Long, userId: Long) { - val role = getRole(roleId) + val role = getRoleById(roleId) val account = accountService.getAccountById(userId) role.accounts.remove(account) roleRepository.save(role) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt index ff3c2555..8c615b92 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt @@ -530,6 +530,7 @@ class GenerationControllerTest @Autowired constructor( @BeforeEach fun addGenerations() { initializeTestGenerations() + TestUtils.startNewTransaction() } @Test @@ -574,7 +575,7 @@ class GenerationControllerTest @Autowired constructor( jsonPath("$.roles.length()").value(1), jsonPath("$.roles[0].name").value("role1") ).andDocument(documentation, documentRequestPayload = true) - + TestUtils.startNewTransaction() val role = roleRepository.findAll().toList() .filter { it.generation.schoolYear == "20-21" } .find { it.name == "role1" } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index e4703cea..4157da6b 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -292,10 +292,11 @@ internal class RoleControllerTest @Autowired constructor( @BeforeEach fun addRole() { generation1 = Generation("22-23") - role = Role("test-role-1", Permissions(), true) - generation1.roles.add(role) generationRepository.save(generation1) + role = Role("test-role-1", Permissions(), true) role.generation = generation1 + roleRepository.save(role) + TestUtils.startNewTransaction() } @AfterEach @@ -314,6 +315,7 @@ internal class RoleControllerTest @Autowired constructor( "The id must exist in order to remove it correctly", urlParameters = parameters ) + TestUtils.startNewTransaction() assert(roleRepository.findByIdOrNull(id) == null) assert(generationRepository.findFirstByOrderBySchoolYearDesc()!!.roles.size == 0) } @@ -457,6 +459,7 @@ internal class RoleControllerTest @Autowired constructor( fun addRoleAndUser() { roleRepository.save(testRole) accountRepository.save(testAccount) + TestUtils.startNewTransaction() } @Test @@ -475,7 +478,9 @@ internal class RoleControllerTest @Autowired constructor( hasRequestPayload = true, urlParameters = parameters ) + TestUtils.startNewTransaction() assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) + assert(accountRepository.findByIdOrNull(testAccount.id!!)!!.roles.size != 0) } @Test @@ -534,16 +539,10 @@ internal class RoleControllerTest @Autowired constructor( @BeforeEach fun addRoleAndUser() { - accountRepository.save(testAccount) - roleRepository.save(testRole) - testRole.accounts.add(testAccount) - roleRepository.save(testRole) - } - - @AfterEach - fun removeRolesAndUser() { - testRole.accounts.remove(testAccount) roleRepository.save(testRole) + testAccount.roles.add(testRole) + accountRepository.save(testAccount) + TestUtils.startNewTransaction() } @Test @@ -571,12 +570,13 @@ internal class RoleControllerTest @Autowired constructor( .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( - status().isNotFound() + status().isNotFound ).andDocumentErrorResponse( documentationRoles, hasRequestPayload = true, urlParameters = parameters ) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -587,12 +587,13 @@ internal class RoleControllerTest @Autowired constructor( .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to 1234))) ).andExpectAll( - status().isNotFound() + status().isNotFound ).andDocumentErrorResponse( documentationRoles, hasRequestPayload = true, urlParameters = parameters ) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -603,12 +604,13 @@ internal class RoleControllerTest @Autowired constructor( .contentType(MediaType.APPLICATION_JSON) .content(objectMapper.writeValueAsString(mapOf("userId" to testAccount.id))) ).andExpectAll( - status().isNotFound() + status().isNotFound ).andDocumentErrorResponse( documentationRoles, hasRequestPayload = true, urlParameters = parameters ) + TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } } From 62907e4ab9b04a0cc913482365661885850c3395 Mon Sep 17 00:00:00 2001 From: Luis Duarte Date: Wed, 26 Jul 2023 15:29:03 +0100 Subject: [PATCH 20/24] remove redudant permissions call --- .../pt/up/fe/ni/website/backend/controller/RoleController.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 9a5417a1..63bdcd00 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -10,7 +10,6 @@ import org.springframework.web.bind.annotation.RestController import pt.up.fe.ni.website.backend.dto.auth.UserIdDto import pt.up.fe.ni.website.backend.dto.entity.RoleDto import pt.up.fe.ni.website.backend.dto.roles.PermissionsDto -import pt.up.fe.ni.website.backend.model.permissions.Permissions import pt.up.fe.ni.website.backend.service.RoleService @RestController @@ -81,7 +80,7 @@ class RoleController(private val roleService: RoleService) { roleService.revokePermissionFromRoleOnActivity( id, activityId, - Permissions(permissionsDto.permissions) + permissionsDto.permissions ) return emptyMap() } From 95dc712f83cc465ce30824dd9132e256ebffe29e Mon Sep 17 00:00:00 2001 From: Luis Duarte Date: Wed, 26 Jul 2023 15:53:11 +0100 Subject: [PATCH 21/24] fix tests --- .../website/backend/controller/RoleControllerTest.kt | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 4157da6b..c997823a 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -540,9 +540,14 @@ internal class RoleControllerTest @Autowired constructor( @BeforeEach fun addRoleAndUser() { roleRepository.save(testRole) - testAccount.roles.add(testRole) accountRepository.save(testAccount) - TestUtils.startNewTransaction() + testRole.accounts.add(testAccount) + roleRepository.save(testRole) + } + + @AfterEach + fun removeAssociations() { + if (testRole.accounts.size != 0) testRole.accounts.clear() } @Test @@ -576,7 +581,6 @@ internal class RoleControllerTest @Autowired constructor( hasRequestPayload = true, urlParameters = parameters ) - TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -593,7 +597,6 @@ internal class RoleControllerTest @Autowired constructor( hasRequestPayload = true, urlParameters = parameters ) - TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } @@ -610,7 +613,6 @@ internal class RoleControllerTest @Autowired constructor( hasRequestPayload = true, urlParameters = parameters ) - TestUtils.startNewTransaction(rollback = true) assert(roleRepository.findByIdOrNull(testRole.id!!)!!.accounts.size != 0) } } From 6dcb39e598268c57f3f06212fd4825aa15edb467 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 16 Aug 2023 11:30:21 +0000 Subject: [PATCH 22/24] Add role update endpoint --- .../backend/controller/RoleController.kt | 9 +- .../backend/dto/entity/GenerationDto.kt | 3 +- .../{RoleDto.kt => role/CreateRoleDto.kt} | 6 +- .../backend/dto/entity/role/UpdateRoleDto.kt | 9 ++ .../ni/website/backend/service/RoleService.kt | 18 +++- .../controller/GenerationControllerTest.kt | 12 +-- .../backend/controller/RoleControllerTest.kt | 90 +++++++++++++++++-- 7 files changed, 128 insertions(+), 19 deletions(-) rename src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/{RoleDto.kt => role/CreateRoleDto.kt} (66%) create mode 100644 src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/UpdateRoleDto.kt diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt index 63bdcd00..08731ce7 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt @@ -4,11 +4,13 @@ import org.springframework.web.bind.annotation.DeleteMapping import org.springframework.web.bind.annotation.GetMapping import org.springframework.web.bind.annotation.PathVariable import org.springframework.web.bind.annotation.PostMapping +import org.springframework.web.bind.annotation.PutMapping import org.springframework.web.bind.annotation.RequestBody import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RestController import pt.up.fe.ni.website.backend.dto.auth.UserIdDto -import pt.up.fe.ni.website.backend.dto.entity.RoleDto +import pt.up.fe.ni.website.backend.dto.entity.role.CreateRoleDto +import pt.up.fe.ni.website.backend.dto.entity.role.UpdateRoleDto import pt.up.fe.ni.website.backend.dto.roles.PermissionsDto import pt.up.fe.ni.website.backend.service.RoleService @@ -22,11 +24,14 @@ class RoleController(private val roleService: RoleService) { fun getRole(@PathVariable id: Long) = roleService.getRoleById(id) @PostMapping - fun createNewRole(@RequestBody dto: RoleDto) = roleService.createNewRole(dto) + fun createNewRole(@RequestBody dto: CreateRoleDto) = roleService.createNewRole(dto) @DeleteMapping("/{id}") fun deleteRole(@PathVariable id: Long) = roleService.deleteRole(id) + @PutMapping("/{id}") + fun updateRole(@PathVariable id: Long, @RequestBody dto: UpdateRoleDto) = roleService.updateRole(id, dto) + @PostMapping("/{id}/permissions") fun grantPermissionToRole( @PathVariable id: Long, diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/GenerationDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/GenerationDto.kt index cbc6f835..342c64d2 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/GenerationDto.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/GenerationDto.kt @@ -1,8 +1,9 @@ package pt.up.fe.ni.website.backend.dto.entity +import pt.up.fe.ni.website.backend.dto.entity.role.CreateRoleDto import pt.up.fe.ni.website.backend.model.Generation class GenerationDto( var schoolYear: String?, - val roles: List + val roles: List ) : EntityDto() diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/CreateRoleDto.kt similarity index 66% rename from src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt rename to src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/CreateRoleDto.kt index 77e76963..7fc5160e 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/RoleDto.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/CreateRoleDto.kt @@ -1,9 +1,11 @@ -package pt.up.fe.ni.website.backend.dto.entity +package pt.up.fe.ni.website.backend.dto.entity.role import com.fasterxml.jackson.annotation.JsonProperty +import pt.up.fe.ni.website.backend.dto.entity.EntityDto +import pt.up.fe.ni.website.backend.dto.entity.PerActivityRoleDto import pt.up.fe.ni.website.backend.model.Role -class RoleDto( +class CreateRoleDto( val name: String, val permissions: List, diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/UpdateRoleDto.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/UpdateRoleDto.kt new file mode 100644 index 00000000..5e8a848f --- /dev/null +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/dto/entity/role/UpdateRoleDto.kt @@ -0,0 +1,9 @@ +package pt.up.fe.ni.website.backend.dto.entity.role + +import pt.up.fe.ni.website.backend.dto.entity.EntityDto +import pt.up.fe.ni.website.backend.model.Role + +class UpdateRoleDto( + val name: String, + val isSection: Boolean? +) : EntityDto() diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 09b6d189..239fb1c3 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -3,7 +3,8 @@ package pt.up.fe.ni.website.backend.service import jakarta.transaction.Transactional import jakarta.validation.Validator import org.springframework.stereotype.Service -import pt.up.fe.ni.website.backend.dto.entity.RoleDto +import pt.up.fe.ni.website.backend.dto.entity.role.CreateRoleDto +import pt.up.fe.ni.website.backend.dto.entity.role.UpdateRoleDto import pt.up.fe.ni.website.backend.model.PerActivityRole import pt.up.fe.ni.website.backend.model.Role import pt.up.fe.ni.website.backend.model.permissions.Permissions @@ -64,7 +65,7 @@ class RoleService( perActivityRoleRepository.save(foundActivity) } - fun createNewRole(dto: RoleDto): Role { + fun createNewRole(dto: CreateRoleDto): Role { val role = dto.create() val generation = generationService.getGenerationByIdOrInferLatest(dto.generationId) @@ -98,4 +99,17 @@ class RoleService( role.accounts.remove(account) roleRepository.save(role) } + + fun updateRole(roleId: Long, dto: UpdateRoleDto): Role { + val role = getRoleById(roleId) + + // just for validation + role.name = dto.name + if (validator.validateProperty(role.generation, "roles").isNotEmpty()) { + throw IllegalArgumentException(ErrorMessages.roleAlreadyExists(role.name, role.generation.schoolYear)) + } + + dto.update(role) + return roleRepository.save(role) + } } diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt index 8c615b92..8654aea9 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/GenerationControllerTest.kt @@ -24,7 +24,7 @@ import org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPat import org.springframework.test.web.servlet.result.MockMvcResultMatchers.status import pt.up.fe.ni.website.backend.dto.entity.GenerationDto import pt.up.fe.ni.website.backend.dto.entity.PerActivityRoleDto -import pt.up.fe.ni.website.backend.dto.entity.RoleDto +import pt.up.fe.ni.website.backend.dto.entity.role.CreateRoleDto import pt.up.fe.ni.website.backend.model.Account import pt.up.fe.ni.website.backend.model.Activity import pt.up.fe.ni.website.backend.model.Event @@ -420,14 +420,14 @@ class GenerationControllerTest @Autowired constructor( val generationDtoWithRoles = GenerationDto( "20-21", listOf( - RoleDto( + CreateRoleDto( "role1", emptyList(), true, emptyList(), emptyList() ), - RoleDto( + CreateRoleDto( "role2", emptyList(), false, @@ -473,7 +473,7 @@ class GenerationControllerTest @Autowired constructor( val generationDtoWithRoles = GenerationDto( "20-21", listOf( - RoleDto( + CreateRoleDto( "role1", listOf(0, 1), true, @@ -553,7 +553,7 @@ class GenerationControllerTest @Autowired constructor( val generationDtoWithAccounts = GenerationDto( "20-21", listOf( - RoleDto( + CreateRoleDto( "role1", emptyList(), true, @@ -595,7 +595,7 @@ class GenerationControllerTest @Autowired constructor( val generationDtoWithActivities = GenerationDto( "20-21", listOf( - RoleDto( + CreateRoleDto( "role1", emptyList(), true, diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index c997823a..717dee2a 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -3,7 +3,7 @@ package pt.up.fe.ni.website.backend.controller import com.epages.restdocs.apispec.ResourceDocumentation.parameterWithName import com.fasterxml.jackson.databind.ObjectMapper import jakarta.transaction.Transactional -import java.util.* +import java.util.Calendar import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.DisplayName @@ -14,14 +14,11 @@ import org.springframework.http.MediaType import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.delete import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.get import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.post +import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders.put import org.springframework.test.web.servlet.MockMvc -import org.springframework.test.web.servlet.delete -import org.springframework.test.web.servlet.get -import org.springframework.test.web.servlet.post import org.springframework.test.web.servlet.result.MockMvcResultMatchers.content import org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath import org.springframework.test.web.servlet.result.MockMvcResultMatchers.status -import org.springframework.web.servlet.function.RequestPredicates.contentType import pt.up.fe.ni.website.backend.model.Account import pt.up.fe.ni.website.backend.model.CustomWebsite import pt.up.fe.ni.website.backend.model.Generation @@ -100,7 +97,7 @@ internal class RoleControllerTest @Autowired constructor( @DisplayName("GET /roles") inner class GetAllRoles { - val roles = listOf( + private val roles = listOf( testRole, Role( "El Presidant", @@ -861,4 +858,85 @@ internal class RoleControllerTest @Autowired constructor( ) } } + + @NestedTest + @DisplayName("PUT /roles/{id}") + inner class UpdateRoleTest { + + @BeforeEach + fun addAll() { + generationRepository.save(testGeneration) + testRole.generation = testGeneration + otherTestRole.generation = testGeneration + roleRepository.save(testRole) + roleRepository.save(otherTestRole) + TestUtils.startNewTransaction() + } + + @Test + fun `should correctly update role name and isSection`() { + mockMvc.perform( + put("/roles/{id}", testRole.id) + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "name" to "Membro", + "isSection" to true + ) + ) + ) + ) + .andExpectAll(status().isOk) + .andDocument( + documentationRoles, + "Updates role information by ID", + "Updates some role information (eg: name and isSection). " + + "It will throw an error if the desired name already exists on role generation " + + "or if role doesn't exist.", + hasRequestPayload = true + ) + TestUtils.startNewTransaction() + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.name == "Membro") + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.isSection) + } + + @Test + fun `should not update role name if it already exists in generation`() { + mockMvc.perform( + put("/roles/{id}", testRole.id) + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "name" to otherTestRole.name, + "isSection" to true + ) + ) + ) + ) + .andExpectAll(status().isUnprocessableEntity) + .andDocumentErrorResponse(documentationRoles) + TestUtils.startNewTransaction(rollback = true) + assert(roleRepository.findByIdOrNull(testRole.id!!)!!.name == "TestRole") + assert(!roleRepository.findByIdOrNull(testRole.id!!)!!.isSection) + } + + @Test + fun `should not update role name if role doesn't exist`() { + mockMvc.perform( + put("/roles/{id}", 1234) + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + mapOf( + "name" to otherTestRole.name, + "isSection" to true + ) + ) + ) + ) + .andExpectAll(status().isNotFound) + } + } } From cbac9ff620d963da6fb7ad4843c98719dbe9832b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 16 Aug 2023 11:37:53 +0000 Subject: [PATCH 23/24] update tests because of merge --- .../ni/website/backend/controller/RoleControllerTest.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt index 717dee2a..41ec65d7 100644 --- a/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt +++ b/src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt @@ -75,7 +75,9 @@ internal class RoleControllerTest @Autowired constructor( val project = Project( "UNI", - "Melhor app" + "Melhor app", + image = "image.png", + targetAudience = "Estudantes" ) val testAccount = Account( @@ -88,7 +90,7 @@ internal class RoleControllerTest @Autowired constructor( "https://linkedin.com", "https://github.com", listOf( - CustomWebsite("https://test-website.com", "https://test-website.com/logo.png") + CustomWebsite("https://test-website.com", "https://test-website.com/logo.png", "Website pessoal") ), mutableListOf() ) @@ -736,7 +738,7 @@ internal class RoleControllerTest @Autowired constructor( @BeforeEach fun addAll() { - project = Project("test project", "test") + project = Project("test project", "test", image = "image.png", targetAudience = "estudantes") roleRepository.save(testRole) projectRepository.save(project) val perActivityRole = PerActivityRole(Permissions(listOf(Permission.EDIT_ACTIVITY))) From 100836c5017be7f32f5048f2f66bd4a6aec233f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Duarte?= Date: Wed, 16 Aug 2023 16:19:17 +0100 Subject: [PATCH 24/24] make updating logic simpler --- .../kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt index 239fb1c3..2ba45a9d 100644 --- a/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt +++ b/src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt @@ -103,13 +103,11 @@ class RoleService( fun updateRole(roleId: Long, dto: UpdateRoleDto): Role { val role = getRoleById(roleId) - // just for validation - role.name = dto.name + dto.update(role) if (validator.validateProperty(role.generation, "roles").isNotEmpty()) { throw IllegalArgumentException(ErrorMessages.roleAlreadyExists(role.name, role.generation.schoolYear)) } - dto.update(role) return roleRepository.save(role) } }