Skip to content

Dev To Main #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iamitprakash merged 3 commits into from
Jun 10, 2025
Merged

Dev To Main #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
526e35a
Merge pull request #207 from Shyam-Vishwakarma/integration-tests
Shyam-Vishwakarma Jun 2, 2025
096518f
fix: fix bugs in the endpoint for updating endorsements (#208)
Shyam-Vishwakarma Jun 8, 2025
8f2f843
Merge branch 'main' into develop
iamitprakash Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions skill-tree/src/main/java/com/RDS/skilltree/apis/EndorsementsApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ public class EndorsementsApi {

@PatchMapping("/{id}")
public ResponseEntity<EndorsementViewModel> update(
@PathVariable Integer id, @Valid @RequestBody UpdateEndorsementViewModel body) {
return new ResponseEntity<>(endorsementService.update(id, body), HttpStatus.OK);
@PathVariable Integer id,
@Valid @RequestBody UpdateEndorsementViewModel body,
@RequestParam(name = "dev", required = false, defaultValue = "false") boolean isDev) {

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

@Shyam-Vishwakarma Shyam-Vishwakarma Jun 10, 2025

Choose a reason for hiding this comment

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

  • currently we are using this method only for feature flags or dev mode

Copy link

@korbit-ai korbit-ai bot Jun 10, 2025

Choose a reason for hiding this comment

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

While I understand it's used for feature flags, exposing dev flags via API parameters creates security risks. Consider using a proper feature flag management system or environment-based configuration instead. This provides better security and control over feature rollouts.

return new ResponseEntity<>(endorsementService.update(id, body, isDev), HttpStatus.OK);
}
}
13 changes: 9 additions & 4 deletions skill-tree/src/main/java/com/RDS/skilltree/exceptions/GlobalExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.RDS.skilltree.exceptions;

import com.RDS.skilltree.utils.Constants.ExceptionMessages;
import com.RDS.skilltree.utils.GenericResponse;
import jakarta.validation.ConstraintViolationException;
import java.util.List;
Expand Down Expand Up @@ -27,10 +28,7 @@ public ResponseEntity<GenericResponse<Object>> handleNoEntityException(NoEntityE
@ExceptionHandler({AuthenticationException.class, InsufficientAuthenticationException.class})
public ResponseEntity<GenericResponse<Object>> handleInvalidBearerTokenException(Exception ex) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED)
.body(
new GenericResponse<>(
null,
"The access token provided is expired, revoked, malformed, or invalid for other reasons."));
.body(new GenericResponse<>(ExceptionMessages.INVALID_ACCESS_TOKEN));
}

@ExceptionHandler({AccessDeniedException.class})
Expand Down Expand Up @@ -142,4 +140,11 @@ public ResponseEntity<?> handleEndorsementAlreadyExistsException(
return new ResponseEntity<>(
new GenericResponse<>(ex.getMessage()), HttpStatus.METHOD_NOT_ALLOWED);
}

@ExceptionHandler(IllegalStateException.class)
public ResponseEntity<?> handleIllegalStateException(IllegalStateException ex) {
log.error("IllegalStateException - Error : {}", ex.getMessage());
return new ResponseEntity<>(
new GenericResponse<>(ex.getMessage()), HttpStatus.METHOD_NOT_ALLOWED);
}
}
3 changes: 2 additions & 1 deletion skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ public interface EndorsementService {

EndorsementViewModel create(CreateEndorsementViewModel endorsement);

EndorsementViewModel update(Integer endorsementId, UpdateEndorsementViewModel endorsement);
EndorsementViewModel update(
Integer endorsementId, UpdateEndorsementViewModel endorsement, boolean isDev);

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

@Shyam-Vishwakarma Shyam-Vishwakarma Jun 10, 2025

Choose a reason for hiding this comment

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

In this repository, isDev is used as a feature flag, which is descriptive enough to indicate dev mode

Copy link

@korbit-ai korbit-ai bot Jun 10, 2025

Choose a reason for hiding this comment

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

Fair point. If 'isDev' is the standard feature flag naming in your codebase, keeping it consistent makes sense. I'll resolve this comment.

}
58 changes: 34 additions & 24 deletions skill-tree/src/main/java/com/RDS/skilltree/services/EndorsementServiceImplementation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.RDS.skilltree.dtos.RdsGetUserDetailsResDto;
import com.RDS.skilltree.exceptions.EndorsementAlreadyExistsException;
import com.RDS.skilltree.exceptions.EndorsementNotFoundException;
import com.RDS.skilltree.exceptions.ForbiddenException;
import com.RDS.skilltree.exceptions.SelfEndorsementNotAllowedException;
import com.RDS.skilltree.exceptions.SkillNotFoundException;
import com.RDS.skilltree.models.Endorsement;
Expand Down Expand Up @@ -130,30 +131,39 @@ public EndorsementViewModel create(CreateEndorsementViewModel endorsementViewMod
}

@Override
public EndorsementViewModel update(Integer endorsementId, UpdateEndorsementViewModel body) {
Optional<Endorsement> exitingEndorsement = endorsementRepository.findById(endorsementId);

if (exitingEndorsement.isEmpty()) {
log.info(String.format("Endorsement with id: %s not found", endorsementId));
throw new EndorsementNotFoundException(ExceptionMessages.ENDORSEMENT_NOT_FOUND);
public EndorsementViewModel update(
Integer endorsementId, UpdateEndorsementViewModel body, boolean isDev) {
if (isDev) {
Optional<Endorsement> existingEndorsement = endorsementRepository.findById(endorsementId);

if (existingEndorsement.isEmpty()) {
log.info("Endorsement with id: {} not found", endorsementId);
throw new EndorsementNotFoundException(ExceptionMessages.ENDORSEMENT_NOT_FOUND);
}

Endorsement endorsement = existingEndorsement.get();

JwtUser jwtDetails =
(JwtUser) SecurityContextHolder.getContext().getAuthentication().getPrincipal();
String userId = jwtDetails.getRdsUserId();

if (endorsement.getEndorserId().equals(userId)) {
RdsGetUserDetailsResDto endorseDetails =
rdsService.getUserDetails(endorsement.getEndorseId());
RdsGetUserDetailsResDto endorserDetails = rdsService.getUserDetails(userId);

endorsement.setMessage(body.getMessage());
Endorsement savedEndorsementDetails = endorsementRepository.save(endorsement);

return EndorsementViewModel.toViewModel(
savedEndorsementDetails,
UserViewModel.toViewModel(endorseDetails.getUser()),
UserViewModel.toViewModel(endorserDetails.getUser()));
} else {
log.warn("User: {} is not authorized to update endorsement: {}", userId, endorsementId);
throw new ForbiddenException(ExceptionMessages.UNAUTHORIZED_ENDORSEMENT_UPDATE);
}
}

Endorsement endorsement = exitingEndorsement.get();
String updatedMessage = body.getMessage();

if (updatedMessage != null) {
endorsement.setMessage(updatedMessage);
}

Endorsement savedEndorsementDetails = endorsementRepository.save(endorsement);
RdsGetUserDetailsResDto endorseDetails =
rdsService.getUserDetails(savedEndorsementDetails.getEndorseId());
RdsGetUserDetailsResDto endorserDetails =
rdsService.getUserDetails(savedEndorsementDetails.getEndorserId());

return EndorsementViewModel.toViewModel(
savedEndorsementDetails,
UserViewModel.toViewModel(endorseDetails.getUser()),
UserViewModel.toViewModel(endorserDetails.getUser()));
throw new IllegalStateException(ExceptionMessages.UPDATE_DISABLED_IN_NON_DEV_MODE);
Comment on lines +136 to +167
Copy link

@korbit-ai korbit-ai bot Jun 10, 2025

Choose a reason for hiding this comment

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

Deep Nesting in Update Method category Readability

Tell me more
What is the issue?

The update method has deeply nested logic within an if-statement, making the code flow harder to follow. The dev mode check should be inverted to handle the non-dev case first.

Why this matters

Deep nesting reduces code readability and makes it harder to understand the main flow of the method. Early returns improve code clarity by reducing cognitive load.

Suggested change ∙ Feature Preview
public EndorsementViewModel update(Integer endorsementId, UpdateEndorsementViewModel body, boolean isDev) {
    if (!isDev) {
        throw new IllegalStateException(ExceptionMessages.UPDATE_DISABLED_IN_NON_DEV_MODE);
    }
    
    Optional<Endorsement> existingEndorsement = endorsementRepository.findById(endorsementId);
    // ... rest of the logic ...
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

}
}
3 changes: 2 additions & 1 deletion skill-tree/src/main/java/com/RDS/skilltree/services/external/RdsServiceImplementation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.RDS.skilltree.dtos.RdsGetUserDetailsResDto;
import com.RDS.skilltree.exceptions.UserNotFoundException;
import com.RDS.skilltree.utils.Constants.ExceptionMessages;
import lombok.RequiredArgsConstructor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -26,7 +27,7 @@ public RdsGetUserDetailsResDto getUserDetails(String id) {
return restTemplate.getForObject(url, RdsGetUserDetailsResDto.class);
} catch (RestClientException error) {
log.error("Error calling url {}, error: {}", url, error.getMessage());
throw new UserNotFoundException("Error getting user details");
throw new UserNotFoundException(ExceptionMessages.USER_NOT_FOUND);
}
}
}
9 changes: 9 additions & 0 deletions skill-tree/src/main/java/com/RDS/skilltree/utils/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,14 @@ public static final class ExceptionMessages {
public static final String SKILL_NOT_FOUND = "Skill does not exist";
public static final String ENDORSEMENT_ALREADY_EXISTS = "Endorsement already exists";
public static final String ENDORSEMENT_NOT_FOUND = "Endorsement not found";
public static final String ENDORSEMENT_MESSAGE_EMPTY = "Endorsement message cannot be empty";
public static final String USER_NOT_FOUND = "Error getting user details";
Copy link

@korbit-ai korbit-ai bot Jun 10, 2025

Choose a reason for hiding this comment

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

Inconsistent Error Message for User Not Found Scenario category Error Handling

Tell me more
What is the issue?

The error message 'Error getting user details' does not accurately reflect the USER_NOT_FOUND constant name and scenario.

Why this matters

Using a generic error message instead of a specific one can make debugging harder and may confuse users about the actual issue - that the user was not found in the system.

Suggested change ∙ Feature Preview

Change the message to be consistent with the constant name and specific to the scenario:

public static final String USER_NOT_FOUND = "User not found";
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

public static final String UNAUTHORIZED_ENDORSEMENT_UPDATE =
"Not authorized to update this endorsement";
public static final String INVALID_ACCESS_TOKEN =
"The access token provided is expired, revoked, malformed, or invalid for other reasons.";
public static final String ACCESS_DENIED = "Access Denied";
public static final String UPDATE_DISABLED_IN_NON_DEV_MODE =
"Update is not allowed outside of development mode";
}
}
5 changes: 3 additions & 2 deletions skill-tree/src/main/java/com/RDS/skilltree/viewmodels/UpdateEndorsementViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package com.RDS.skilltree.viewmodels;

import jakarta.validation.constraints.NotNull;
import com.RDS.skilltree.utils.Constants.ExceptionMessages;
import jakarta.validation.constraints.NotBlank;
import lombok.Getter;
import lombok.Setter;

@Getter
@Setter
public class UpdateEndorsementViewModel {
@NotNull(message = "Message cannot be empty")
@NotBlank(message = ExceptionMessages.ENDORSEMENT_MESSAGE_EMPTY)
private String message;
}
2 changes: 1 addition & 1 deletion skill-tree/src/main/resources/application-test.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cookieName=rds-session-v2-development
cookieName=rds-session-v2
test.db.mysql-image=mysql:8.1.0
spring.flyway.enabled=true
spring.flyway.locations=classpath:db/migrations
Expand Down
Loading