From 136f16f613652312ea66a97e331b36d3c40b4b92 Mon Sep 17 00:00:00 2001 From: Ludy Date: Mon, 16 Jun 2025 22:08:50 +0200 Subject: [PATCH] feat: Improve team management UX with message-based feedback and internal team protection (#3719) # Description of Changes - Refactored team management logic to unify and streamline feedback via `messageType` query parameters. - Added backend checks to prevent renaming, deleting, or reassigning users to/from the protected Internal team. - Updated Thymeleaf templates (`teams.html`, `team-details.html`, `adminSettings.html`) to support user-visible success and error messages based on controller redirects. - Ensured `team.cannotMoveInternalUsers`, `team.internalTeamNotAccessible`, and `invalidRoleMessage` are properly internationalized. - Replaced hardcoded `/adminSettings` redirects with `/teams` for more consistent UX. **Why**: To provide admins with immediate, meaningful feedback during team operations and to enforce data integrity around protected teams like "Internal". --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/HowToAddNewLanguage.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [x] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md#6-testing) for more details. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../security/config/AccountWebController.java | 3 ++ .../controller/api/TeamController.java | 20 +++++------ .../controller/web/TeamWebController.java | 34 +++++++++++++++++-- .../templates/accounts/team-details.html | 5 +++ .../resources/templates/accounts/teams.html | 19 ++++++++++- .../main/resources/messages_en_GB.properties | 1 + .../resources/templates/adminSettings.html | 26 +++++++------- 7 files changed, 82 insertions(+), 26 deletions(-) diff --git a/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java b/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java index 4a429fc48..9ea537e23 100644 --- a/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java +++ b/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java @@ -331,6 +331,9 @@ public class AccountWebController { case "userNotFound" -> "userNotFoundMessage"; case "downgradeCurrentUser" -> "downgradeCurrentUserMessage"; case "disabledCurrentUser" -> "disabledCurrentUserMessage"; + case "cannotMoveInternalUsers" -> "team.cannotMoveInternalUsers"; + case "internalTeamNotAccessible" -> "team.internalTeamNotAccessible"; + case "invalidRole" -> "invalidRoleMessage"; default -> messageType; }; model.addAttribute("changeMessage", changeMessage); diff --git a/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/TeamController.java b/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/TeamController.java index f5dde134f..57b9c7a17 100644 --- a/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/TeamController.java +++ b/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/TeamController.java @@ -31,12 +31,12 @@ public class TeamController { @PostMapping("/create") public RedirectView createTeam(@RequestParam("name") String name) { if (teamRepository.existsByNameIgnoreCase(name)) { - return new RedirectView("/adminSettings?messageType=teamExists"); + return new RedirectView("/teams?messageType=teamExists"); } Team team = new Team(); team.setName(name); teamRepository.save(team); - return new RedirectView("/adminSettings?messageType=teamCreated"); + return new RedirectView("/teams?messageType=teamCreated"); } @PreAuthorize("hasRole('ROLE_ADMIN')") @@ -45,21 +45,21 @@ public class TeamController { @RequestParam("teamId") Long teamId, @RequestParam("newName") String newName) { Optional existing = teamRepository.findById(teamId); if (existing.isEmpty()) { - return new RedirectView("/adminSettings?messageType=teamNotFound"); + return new RedirectView("/teams?messageType=teamNotFound"); } if (teamRepository.existsByNameIgnoreCase(newName)) { - return new RedirectView("/adminSettings?messageType=teamNameExists"); + return new RedirectView("/teams?messageType=teamNameExists"); } Team team = existing.get(); // Prevent renaming the Internal team if (team.getName().equals(TeamService.INTERNAL_TEAM_NAME)) { - return new RedirectView("/adminSettings?messageType=internalTeamNotAccessible"); + return new RedirectView("/teams?messageType=internalTeamNotAccessible"); } team.setName(newName); teamRepository.save(team); - return new RedirectView("/adminSettings?messageType=teamRenamed"); + return new RedirectView("/teams?messageType=teamRenamed"); } @PreAuthorize("hasRole('ROLE_ADMIN')") @@ -68,23 +68,23 @@ public class TeamController { public RedirectView deleteTeam(@RequestParam("teamId") Long teamId) { Optional teamOpt = teamRepository.findById(teamId); if (teamOpt.isEmpty()) { - return new RedirectView("/adminSettings?messageType=teamNotFound"); + return new RedirectView("/teams?messageType=teamNotFound"); } Team team = teamOpt.get(); // Prevent deleting the Internal team if (team.getName().equals(TeamService.INTERNAL_TEAM_NAME)) { - return new RedirectView("/adminSettings?messageType=internalTeamNotAccessible"); + return new RedirectView("/teams?messageType=internalTeamNotAccessible"); } long memberCount = userRepository.countByTeam(team); if (memberCount > 0) { - return new RedirectView("/adminSettings?messageType=teamHasUsers"); + return new RedirectView("/teams?messageType=teamHasUsers"); } teamRepository.delete(team); - return new RedirectView("/adminSettings?messageType=teamDeleted"); + return new RedirectView("/teams?messageType=teamDeleted"); } @PreAuthorize("hasRole('ROLE_ADMIN')") diff --git a/proprietary/src/main/java/stirling/software/proprietary/security/controller/web/TeamWebController.java b/proprietary/src/main/java/stirling/software/proprietary/security/controller/web/TeamWebController.java index ef2e0c2bd..9e5b8bb84 100644 --- a/proprietary/src/main/java/stirling/software/proprietary/security/controller/web/TeamWebController.java +++ b/proprietary/src/main/java/stirling/software/proprietary/security/controller/web/TeamWebController.java @@ -1,5 +1,6 @@ package stirling.software.proprietary.security.controller.web; +import jakarta.servlet.http.HttpServletRequest; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -32,7 +33,7 @@ public class TeamWebController { @GetMapping @PreAuthorize("hasRole('ROLE_ADMIN')") - public String listTeams(Model model) { + public String listTeams(HttpServletRequest request, Model model) { // Get teams with user counts using a DTO projection List allTeamsWithCounts = teamRepository.findAllTeamsWithUserCount(); @@ -53,6 +54,27 @@ public class TeamWebController { teamLastRequest.put(teamId, lastActivity); } + String messageType = request.getParameter("messageType"); + if (messageType != null) { + if ("teamCreated".equals(messageType)) { + model.addAttribute("addMessage", "teamCreated"); + } else if ("teamExists".equals(messageType)) { + model.addAttribute("errorMessage", "teamExists"); + } else if ("teamNotFound".equals(messageType)) { + model.addAttribute("errorMessage", "teamNotFound"); + } else if ("teamNameExists".equals(messageType)) { + model.addAttribute("errorMessage", "teamNameExists"); + } else if ("internalTeamNotAccessible".equals(messageType)) { + model.addAttribute("errorMessage", "team.internalTeamNotAccessible"); + } else if ("teamRenamed".equals(messageType)) { + model.addAttribute("changeMessage", "teamRenamed"); + } else if ("teamHasUsers".equals(messageType)) { + model.addAttribute("errorMessage", "teamHasUsers"); + } else if ("teamDeleted".equals(messageType)) { + model.addAttribute("deleteMessage", "teamDeleted"); + } + } + // Add data to the model model.addAttribute("teamsWithCounts", teamsWithCounts); model.addAttribute("teamLastRequest", teamLastRequest); @@ -62,7 +84,8 @@ public class TeamWebController { @GetMapping("/{id}") @PreAuthorize("hasRole('ROLE_ADMIN')") - public String viewTeamDetails(@PathVariable("id") Long id, Model model) { + public String viewTeamDetails( + HttpServletRequest request, @PathVariable("id") Long id, Model model) { // Get the team Team team = teamRepository @@ -105,6 +128,13 @@ public class TeamWebController { userLastRequest.put(username, lastRequest); } + String errorMessage = request.getParameter("error"); + if (errorMessage != null) { + if ("cannotMoveInternalUsers".equals(errorMessage)) { + model.addAttribute("errorMessage", "team.cannotMoveInternalUsers"); + } + } + model.addAttribute("team", team); model.addAttribute("teamUsers", teamUsers); model.addAttribute("availableUsers", availableUsers); diff --git a/proprietary/src/main/resources/templates/accounts/team-details.html b/proprietary/src/main/resources/templates/accounts/team-details.html index aff0c4150..3fb779bae 100644 --- a/proprietary/src/main/resources/templates/accounts/team-details.html +++ b/proprietary/src/main/resources/templates/accounts/team-details.html @@ -32,6 +32,11 @@ + +
+ Default message if not found +
+
arrow_back diff --git a/proprietary/src/main/resources/templates/accounts/teams.html b/proprietary/src/main/resources/templates/accounts/teams.html index 509c3f727..a1e485d62 100644 --- a/proprietary/src/main/resources/templates/accounts/teams.html +++ b/proprietary/src/main/resources/templates/accounts/teams.html @@ -29,12 +29,29 @@
- + arrow_back Back to Settings
+ +
+ Default message if not found +
+ +
+ Default message if not found +
+ +
+ Default message if not found +
+ +
+ Default message if not found +
+
person_add Add New User - - + + group Manage Teams @@ -108,7 +108,7 @@ Change User's Role - + analytics Usage Statistics @@ -120,27 +120,27 @@ # - Username - Team - Roles + Username + Team + Roles Authenticated Last Request - Actions + Actions - - - + + + shield - Role + Role - - + +