registered setter organization registration done #58
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Warning Rate limit exceeded@Kaumadi2k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
src/main/java/com/testify/Testify_Backend/service/ExamSetterService.java (3)
8-14: Add JavaDoc documentation for the interface and methods.The interface methods would benefit from clear documentation explaining their purpose, parameters, return values, and possible exceptions.
Here's a suggested documentation structure:
+/** + * Service interface for managing exam setters and their organization associations. + */ public interface ExamSetterService { + /** + * Retrieves all organizations associated with a specific exam setter. + * + * @param setterId The unique identifier of the exam setter + * @return Set of organization responses + * @throws EntityNotFoundException if the setter doesn't exist + */ Set<OrganizationResponse> getOrganizations(long setterId); + /** + * Validates an exam setter's registration status using their invitation token. + * + * @param token The invitation token + * @return The setter's ID if registered, or a status code + * @throws InvalidTokenException if the token is invalid or expired + */ long checkSetterRegistration(String token); + /** + * Associates an exam setter with their organization using the invitation token. + * + * @param token The invitation token containing organization details + * @return Response indicating success or failure of the operation + * @throws InvalidTokenException if the token is invalid or expired + */ GenericAddOrUpdateResponse addSetterToOrganization(String token);
11-11: Consider enhancing the return type for registration status.Using a
longreturn type forcheckSetterRegistrationcould be ambiguous. Consider using an enum or a dedicated response object to clearly indicate different status scenarios.Example enhancement:
public enum RegistrationStatus { REGISTERED(setterId), PENDING, EXPIRED, INVALID; private final Long setterId; // ... constructors and getters }Then modify the method:
-long checkSetterRegistration(String token); +RegistrationStatus checkSetterRegistration(String token);
3-6: Consider adding validation-related imports.Since these operations involve token validation and entity lookups, consider importing relevant exception classes for better interface documentation.
Add these imports:
+import javax.persistence.EntityNotFoundException; +import com.testify.Testify_Backend.exceptions.InvalidTokenException;src/main/java/com/testify/Testify_Backend/service/ExamSetterServiceImpl.java (1)
28-29: Use constructor injection consistently instead of field injectionConsider making ModelMapper final and letting @requiredargsconstructor handle its injection, consistent with other dependencies.
- @Autowired - private ModelMapper modelMapper; + private final ModelMapper modelMapper;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/main/java/com/testify/Testify_Backend/controller/ExamSetterController.java(1 hunks)src/main/java/com/testify/Testify_Backend/service/ExamSetterService.java(1 hunks)src/main/java/com/testify/Testify_Backend/service/ExamSetterServiceImpl.java(1 hunks)src/main/java/com/testify/Testify_Backend/service/OrganizationServiceImpl.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/testify/Testify_Backend/service/OrganizationServiceImpl.java
🔇 Additional comments (4)
src/main/java/com/testify/Testify_Backend/service/ExamSetterService.java (1)
13-13: Consider adding organization context to addSetterToOrganization.
The method relies solely on the token for organization information. Consider adding explicit organization validation parameters.
Let's verify how the organization ID is extracted from the token:
Consider modifying the method signature to make the organization context explicit:
-GenericAddOrUpdateResponse addSetterToOrganization(String token);
+GenericAddOrUpdateResponse addSetterToOrganization(String token, Long organizationId);src/main/java/com/testify/Testify_Backend/controller/ExamSetterController.java (3)
21-25:
Ensure proper authorization and validation for accessing setter organizations
The getOrganizations endpoint allows retrieval of organizations based on a setterId provided in the URL path. To prevent unauthorized access or disclosure of sensitive information, ensure that proper authentication and authorization mechanisms are in place. Validate that the requesting user has the rights to access the specified setterId.
If authorization is handled elsewhere or secured via security configurations, please disregard this comment.
28-31: 🛠️ Refactor suggestion
Ensure proper exception handling to return meaningful responses
Similar to the getOrganizations method, exceptions thrown by examSetterService.checkSetterRegistration(token) may result in unhandled exceptions and HTTP 500 errors. Implement exception handling to provide appropriate responses, such as 401 Unauthorized for invalid tokens or 400 Bad Request.
34-38: 🛠️ Refactor suggestion
Implement exception handling to improve API reliability
Exceptions from examSetterService.addSetterToOrganization(token) may result in unhandled exceptions. Catch and handle exceptions to return meaningful HTTP responses to the client, such as 400 Bad Request or 409 Conflict if the setter already exists.
| public Set<OrganizationResponse> getOrganizations(long setterId) { | ||
| Optional<ExamSetter> examSetter = examSetterRepository.findById(setterId); | ||
| Set<OrganizationResponse> organizationResponses = new HashSet<>(); | ||
| if (examSetter.isPresent()) { | ||
| Set<Organization> organizations = examSetter.get().getOrganizations(); | ||
| for (Organization organization : organizations) { | ||
| organizationResponses.add(modelMapper.map(organization, OrganizationResponse.class)); | ||
| } | ||
| } | ||
| return organizationResponses; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling and consider using streams for better readability
- The method should handle the case when the setter ID is invalid
- The transformation can be simplified using streams
public Set<OrganizationResponse> getOrganizations(long setterId) {
Optional<ExamSetter> examSetter = examSetterRepository.findById(setterId);
- Set<OrganizationResponse> organizationResponses = new HashSet<>();
- if (examSetter.isPresent()) {
- Set<Organization> organizations = examSetter.get().getOrganizations();
- for (Organization organization : organizations) {
- organizationResponses.add(modelMapper.map(organization, OrganizationResponse.class));
- }
- }
- return organizationResponses;
+ return examSetter.map(setter -> setter.getOrganizations().stream()
+ .map(org -> modelMapper.map(org, OrganizationResponse.class))
+ .collect(Collectors.toSet()))
+ .orElseThrow(() -> new ResourceNotFoundException("Exam setter not found with id: " + setterId));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Set<OrganizationResponse> getOrganizations(long setterId) { | |
| Optional<ExamSetter> examSetter = examSetterRepository.findById(setterId); | |
| Set<OrganizationResponse> organizationResponses = new HashSet<>(); | |
| if (examSetter.isPresent()) { | |
| Set<Organization> organizations = examSetter.get().getOrganizations(); | |
| for (Organization organization : organizations) { | |
| organizationResponses.add(modelMapper.map(organization, OrganizationResponse.class)); | |
| } | |
| } | |
| return organizationResponses; | |
| } | |
| public Set<OrganizationResponse> getOrganizations(long setterId) { | |
| Optional<ExamSetter> examSetter = examSetterRepository.findById(setterId); | |
| return examSetter.map(setter -> setter.getOrganizations().stream() | |
| .map(org -> modelMapper.map(org, OrganizationResponse.class)) | |
| .collect(Collectors.toSet())) | |
| .orElseThrow(() -> new ResourceNotFoundException("Exam setter not found with id: " + setterId)); | |
| } |
| public long checkSetterRegistration(String token) { | ||
| long setterId = 0; | ||
| Optional<ExamSetterInvitation> invitation = examSetterInvitationRepository.findByToken(token); | ||
| String email = invitation.get().getEmail(); | ||
| Optional<ExamSetter> examSetter = examSetterRepository.findByEmail(email); | ||
| if (examSetter.isPresent()) { | ||
| setterId = examSetter.get().getId(); | ||
| } | ||
| return setterId; | ||
| } |
There was a problem hiding this comment.
Fix unsafe Optional handling and add proper error handling
The method has several critical issues:
- Unsafe Optional.get() calls that could throw NullPointerException
- No validation of the token
- Missing error handling for invalid/expired tokens
public long checkSetterRegistration(String token) {
- long setterId = 0;
- Optional<ExamSetterInvitation> invitation = examSetterInvitationRepository.findByToken(token);
- String email = invitation.get().getEmail();
- Optional<ExamSetter> examSetter = examSetterRepository.findByEmail(email);
- if (examSetter.isPresent()) {
- setterId = examSetter.get().getId();
- }
- return setterId;
+ if (token == null || token.trim().isEmpty()) {
+ throw new IllegalArgumentException("Token cannot be null or empty");
+ }
+ return examSetterInvitationRepository.findByToken(token)
+ .orElseThrow(() -> new ResourceNotFoundException("Invalid or expired token"))
+ .map(ExamSetterInvitation::getEmail)
+ .flatMap(examSetterRepository::findByEmail)
+ .map(ExamSetter::getId)
+ .orElse(0L);Committable suggestion skipped: line range outside the PR's diff.
| package com.testify.Testify_Backend.service; | ||
|
|
||
| import com.testify.Testify_Backend.model.ExamSetter; | ||
| import com.testify.Testify_Backend.model.ExamSetterInvitation; | ||
| import com.testify.Testify_Backend.model.Organization; | ||
| import com.testify.Testify_Backend.repository.ExamSetterInvitationRepository; | ||
| import com.testify.Testify_Backend.repository.ExamSetterRepository; | ||
| import com.testify.Testify_Backend.repository.OrganizationRepository; | ||
| import com.testify.Testify_Backend.responses.GenericAddOrUpdateResponse; | ||
| import com.testify.Testify_Backend.responses.exam_management.OrganizationResponse; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.modelmapper.ModelMapper; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.stereotype.Service; | ||
|
|
||
| import java.util.HashSet; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class ExamSetterServiceImpl implements ExamSetterService { | ||
| private final ExamSetterRepository examSetterRepository; | ||
| private final ExamSetterInvitationRepository examSetterInvitationRepository; | ||
| private final OrganizationRepository organizationRepository; | ||
|
|
||
| @Autowired | ||
| private ModelMapper modelMapper; | ||
|
|
||
| @Override | ||
| public Set<OrganizationResponse> getOrganizations(long setterId) { | ||
| Optional<ExamSetter> examSetter = examSetterRepository.findById(setterId); | ||
| Set<OrganizationResponse> organizationResponses = new HashSet<>(); | ||
| if (examSetter.isPresent()) { | ||
| Set<Organization> organizations = examSetter.get().getOrganizations(); | ||
| for (Organization organization : organizations) { | ||
| organizationResponses.add(modelMapper.map(organization, OrganizationResponse.class)); | ||
| } | ||
| } | ||
| return organizationResponses; | ||
| } | ||
|
|
||
| @Override | ||
| public long checkSetterRegistration(String token) { | ||
| long setterId = 0; | ||
| Optional<ExamSetterInvitation> invitation = examSetterInvitationRepository.findByToken(token); | ||
| String email = invitation.get().getEmail(); | ||
| Optional<ExamSetter> examSetter = examSetterRepository.findByEmail(email); | ||
| if (examSetter.isPresent()) { | ||
| setterId = examSetter.get().getId(); | ||
| } | ||
| return setterId; | ||
| } | ||
|
|
||
| @Override | ||
| public GenericAddOrUpdateResponse addSetterToOrganization(String token) { | ||
| GenericAddOrUpdateResponse response = new GenericAddOrUpdateResponse(); | ||
| Optional<ExamSetterInvitation> examSetterInvitation = examSetterInvitationRepository.findByToken(token); | ||
| Organization organization = examSetterInvitation.get().getOrganization(); | ||
| examSetterInvitation.get().setAccepted(true); | ||
| ExamSetter examSetter = examSetterRepository.findByEmail(examSetterInvitation.get().getEmail()).get(); | ||
| examSetter.getOrganizations().add(organization); | ||
| organization.getExamSetters().add(examSetter); | ||
|
|
||
| examSetterRepository.save(examSetter); | ||
| organizationRepository.save(organization); | ||
|
|
||
| response.setSuccess(true); | ||
| response.setMessage("Successfully added an organization"); | ||
| return response; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add proper exception handling for Optional.get() operations
The code has multiple unsafe Optional.get() calls without proper null checks, which could lead to NoSuchElementException. While the imports mentioned in the review aren't strictly necessary (no stream collectors or @transactional operations), there are other critical issues:
checkSetterRegistration: Usesinvitation.get()without checking if invitation is presentaddSetterToOrganization: Multiple unsafeget()calls onexamSetterInvitationandexamSetterRepository.findByEmail()getOrganizations: Has proper null check but other methods need similar treatment
Suggested fixes:
- Add proper null checks before
Optional.get()calls - Consider using
orElseThrow()with appropriate exception types - Handle cases where entities are not found
🔗 Analysis chain
Verify required dependencies and exception handling
Please ensure that the following are properly defined in the project:
- Custom exceptions (ResourceNotFoundException)
- Required imports for Collectors and @transactional
Let me verify if the code actually uses operations that would require these imports and exception handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for custom exceptions and required imports
rg -l "class ResourceNotFoundException"
rg -l "import.*javax.transaction.Transactional"
rg -l "import.*stream.Collectors"
Length of output: 444
Script:
#!/bin/bash
# Check if the code uses operations that would require Collectors or @Transactional
ast-grep --pattern 'stream().collect($$$)'
ast-grep --pattern 'Optional<$_> $_ = $_Repository.findBy$_($$$)'
# Check for potential null pointer scenarios
ast-grep --pattern '$_.get()'
Length of output: 2100
| public GenericAddOrUpdateResponse addSetterToOrganization(String token) { | ||
| GenericAddOrUpdateResponse response = new GenericAddOrUpdateResponse(); | ||
| Optional<ExamSetterInvitation> examSetterInvitation = examSetterInvitationRepository.findByToken(token); | ||
| Organization organization = examSetterInvitation.get().getOrganization(); | ||
| examSetterInvitation.get().setAccepted(true); | ||
| ExamSetter examSetter = examSetterRepository.findByEmail(examSetterInvitation.get().getEmail()).get(); | ||
| examSetter.getOrganizations().add(organization); | ||
| organization.getExamSetters().add(examSetter); | ||
|
|
||
| examSetterRepository.save(examSetter); | ||
| organizationRepository.save(organization); | ||
|
|
||
| response.setSuccess(true); | ||
| response.setMessage("Successfully added an organization"); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Add transaction management and proper error handling
Several critical issues need to be addressed:
- Missing @transactional annotation for atomic operations
- Unsafe Optional.get() calls
- No validation or error handling
- Generic success message
+ @Transactional
public GenericAddOrUpdateResponse addSetterToOrganization(String token) {
- GenericAddOrUpdateResponse response = new GenericAddOrUpdateResponse();
- Optional<ExamSetterInvitation> examSetterInvitation = examSetterInvitationRepository.findByToken(token);
- Organization organization = examSetterInvitation.get().getOrganization();
- examSetterInvitation.get().setAccepted(true);
- ExamSetter examSetter = examSetterRepository.findByEmail(examSetterInvitation.get().getEmail()).get();
- examSetter.getOrganizations().add(organization);
- organization.getExamSetters().add(examSetter);
-
- examSetterRepository.save(examSetter);
- organizationRepository.save(organization);
-
- response.setSuccess(true);
- response.setMessage("Successfully added an organization");
- return response;
+ if (token == null || token.trim().isEmpty()) {
+ throw new IllegalArgumentException("Token cannot be null or empty");
+ }
+
+ ExamSetterInvitation invitation = examSetterInvitationRepository.findByToken(token)
+ .orElseThrow(() -> new ResourceNotFoundException("Invalid or expired token"));
+
+ if (invitation.isAccepted()) {
+ throw new IllegalStateException("Invitation already accepted");
+ }
+
+ Organization organization = invitation.getOrganization();
+ ExamSetter examSetter = examSetterRepository.findByEmail(invitation.getEmail())
+ .orElseThrow(() -> new ResourceNotFoundException("Exam setter not found"));
+
+ if (examSetter.getOrganizations().contains(organization)) {
+ throw new IllegalStateException("Exam setter already added to this organization");
+ }
+
+ invitation.setAccepted(true);
+ examSetter.getOrganizations().add(organization);
+ organization.getExamSetters().add(examSetter);
+
+ examSetterRepository.save(examSetter);
+ organizationRepository.save(organization);
+
+ return new GenericAddOrUpdateResponse(true,
+ String.format("Successfully added exam setter to organization %s", organization.getName()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public GenericAddOrUpdateResponse addSetterToOrganization(String token) { | |
| GenericAddOrUpdateResponse response = new GenericAddOrUpdateResponse(); | |
| Optional<ExamSetterInvitation> examSetterInvitation = examSetterInvitationRepository.findByToken(token); | |
| Organization organization = examSetterInvitation.get().getOrganization(); | |
| examSetterInvitation.get().setAccepted(true); | |
| ExamSetter examSetter = examSetterRepository.findByEmail(examSetterInvitation.get().getEmail()).get(); | |
| examSetter.getOrganizations().add(organization); | |
| organization.getExamSetters().add(examSetter); | |
| examSetterRepository.save(examSetter); | |
| organizationRepository.save(organization); | |
| response.setSuccess(true); | |
| response.setMessage("Successfully added an organization"); | |
| return response; | |
| } | |
| @Transactional | |
| public GenericAddOrUpdateResponse addSetterToOrganization(String token) { | |
| if (token == null || token.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Token cannot be null or empty"); | |
| } | |
| ExamSetterInvitation invitation = examSetterInvitationRepository.findByToken(token) | |
| .orElseThrow(() -> new ResourceNotFoundException("Invalid or expired token")); | |
| if (invitation.isAccepted()) { | |
| throw new IllegalStateException("Invitation already accepted"); | |
| } | |
| Organization organization = invitation.getOrganization(); | |
| ExamSetter examSetter = examSetterRepository.findByEmail(invitation.getEmail()) | |
| .orElseThrow(() -> new ResourceNotFoundException("Exam setter not found")); | |
| if (examSetter.getOrganizations().contains(organization)) { | |
| throw new IllegalStateException("Exam setter already added to this organization"); | |
| } | |
| invitation.setAccepted(true); | |
| examSetter.getOrganizations().add(organization); | |
| organization.getExamSetters().add(examSetter); | |
| examSetterRepository.save(examSetter); | |
| organizationRepository.save(organization); | |
| return new GenericAddOrUpdateResponse(true, | |
| String.format("Successfully added exam setter to organization %s", organization.getName())); | |
| } |
| @GetMapping("/{token}/checkSetterRegistration") | ||
| public ResponseEntity<Long> checkSetterRegistration(@PathVariable("token") String token) { | ||
| long setterId = examSetterService.checkSetterRegistration(token); | ||
| return ResponseEntity.ok(setterId); | ||
| } |
There was a problem hiding this comment.
Avoid passing sensitive tokens in URL path parameters
Passing sensitive tokens as path variables exposes them to security risks, as URLs can be logged, cached, or intercepted. It is recommended to pass tokens in the request headers (e.g., the Authorization header) or in the request body for POST requests.
Apply this diff to modify the endpoint to use a request header:
- @GetMapping("/{token}/checkSetterRegistration")
- public ResponseEntity<Long> checkSetterRegistration(@PathVariable("token") String token) {
+ @GetMapping("/checkSetterRegistration")
+ public ResponseEntity<Long> checkSetterRegistration(@RequestHeader("Authorization") String token) {
long setterId = examSetterService.checkSetterRegistration(token);
return ResponseEntity.ok(setterId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GetMapping("/{token}/checkSetterRegistration") | |
| public ResponseEntity<Long> checkSetterRegistration(@PathVariable("token") String token) { | |
| long setterId = examSetterService.checkSetterRegistration(token); | |
| return ResponseEntity.ok(setterId); | |
| } | |
| @GetMapping("/checkSetterRegistration") | |
| public ResponseEntity<Long> checkSetterRegistration(@RequestHeader("Authorization") String token) { | |
| long setterId = examSetterService.checkSetterRegistration(token); | |
| return ResponseEntity.ok(setterId); | |
| } |
|
|
||
| @PostMapping("/{token}/addSetterToOrganization") | ||
| public ResponseEntity<GenericAddOrUpdateResponse> addSetterToOrganization(@PathVariable("token") String token){ | ||
| log.info("setter reached: {}",token); |
There was a problem hiding this comment.
Do not log sensitive tokens to prevent security breaches
Logging tokens can expose sensitive information in logs, which might be accessed by unauthorized parties. Remove or mask the token in log statements.
Apply this diff to remove the logging statement:
- log.info("setter reached: {}", token);If logging is necessary for debugging, consider logging a masked version of the token or other non-sensitive identifiers.
Committable suggestion skipped: line range outside the PR's diff.
| @PostMapping("/{token}/addSetterToOrganization") | ||
| public ResponseEntity<GenericAddOrUpdateResponse> addSetterToOrganization(@PathVariable("token") String token){ | ||
| log.info("setter reached: {}",token); | ||
| GenericAddOrUpdateResponse response = examSetterService.addSetterToOrganization(token); | ||
| return ResponseEntity.ok(response); | ||
| } |
There was a problem hiding this comment.
Avoid passing sensitive tokens in URL path parameters and logging them
Sensitive tokens should not be included in the URL path or logged, as this can lead to security vulnerabilities. Instead, pass the token in the request headers or request body, and avoid logging the token directly.
Apply this diff to modify the endpoint and remove logging of the token:
- @PostMapping("/{token}/addSetterToOrganization")
- public ResponseEntity<GenericAddOrUpdateResponse> addSetterToOrganization(@PathVariable("token") String token){
- log.info("setter reached: {}", token);
+ @PostMapping("/addSetterToOrganization")
+ public ResponseEntity<GenericAddOrUpdateResponse> addSetterToOrganization(@RequestHeader("Authorization") String token){
GenericAddOrUpdateResponse response = examSetterService.addSetterToOrganization(token);
return ResponseEntity.ok(response);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping("/{token}/addSetterToOrganization") | |
| public ResponseEntity<GenericAddOrUpdateResponse> addSetterToOrganization(@PathVariable("token") String token){ | |
| log.info("setter reached: {}",token); | |
| GenericAddOrUpdateResponse response = examSetterService.addSetterToOrganization(token); | |
| return ResponseEntity.ok(response); | |
| } | |
| @PostMapping("/addSetterToOrganization") | |
| public ResponseEntity<GenericAddOrUpdateResponse> addSetterToOrganization(@RequestHeader("Authorization") String token){ | |
| GenericAddOrUpdateResponse response = examSetterService.addSetterToOrganization(token); | |
| return ResponseEntity.ok(response); | |
| } |
| public ResponseEntity<Set<OrganizationResponse>> getOrganizations(@PathVariable("setterId") long setterId) { | ||
| Set<OrganizationResponse> organizations = examSetterService.getOrganizations(setterId); | ||
| return ResponseEntity.ok(organizations); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement exception handling to return appropriate HTTP responses
If an exception occurs during examSetterService.getOrganizations(setterId), it may result in an unhandled exception and a generic HTTP 500 error. Implement exception handling to return meaningful HTTP responses, such as 404 Not Found if the setter is not found or 400 Bad Request for invalid input.
Apply this diff to add exception handling:
public ResponseEntity<Set<OrganizationResponse>> getOrganizations(@PathVariable("setterId") long setterId) {
+ try {
Set<OrganizationResponse> organizations = examSetterService.getOrganizations(setterId);
return ResponseEntity.ok(organizations);
+ } catch (SetterNotFoundException e) {
+ return ResponseEntity.status(HttpStatus.NOT_FOUND).body(null);
+ } catch (Exception e) {
+ return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(null);
+ }
}Alternatively, consider using @ExceptionHandler or @ControllerAdvice to manage exceptions globally and keep controllers clean.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
requestVerificationmethod from the organization service, streamlining the verification process.Refactor