-
Notifications
You must be signed in to change notification settings - Fork 57
Feedback system design #578
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
a4a1986
d088391
409fb56
e7c257a
8f4a5ab
de1fee3
c8cb676
aedc0b6
144af13
07d3614
267cd09
85c8cb7
cc261de
71343cc
dd08294
cb42ef7
f2e8da5
725032c
5d1e8f1
d27fdbd
80f682a
3f363b6
3074aea
319d8ff
c980481
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| package com.wcc.platform.controller; | ||
|
|
||
| import com.wcc.platform.domain.platform.feedback.Feedback; | ||
| import com.wcc.platform.domain.platform.feedback.FeedbackDto; | ||
| import com.wcc.platform.domain.platform.feedback.FeedbackSearchCriteria; | ||
| import com.wcc.platform.domain.platform.type.FeedbackType; | ||
| import com.wcc.platform.service.FeedbackService; | ||
| import io.swagger.v3.oas.annotations.Operation; | ||
| import io.swagger.v3.oas.annotations.Parameter; | ||
| import io.swagger.v3.oas.annotations.security.SecurityRequirement; | ||
| import io.swagger.v3.oas.annotations.tags.Tag; | ||
| import jakarta.validation.Valid; | ||
| import jakarta.validation.constraints.Max; | ||
| import jakarta.validation.constraints.Min; | ||
| import java.util.List; | ||
| import lombok.AllArgsConstructor; | ||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.validation.annotation.Validated; | ||
| import org.springframework.web.bind.annotation.DeleteMapping; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PatchMapping; | ||
| 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.RequestParam; | ||
| import org.springframework.web.bind.annotation.ResponseStatus; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
|
|
||
| /** Rest controller for feedback APIs. */ | ||
| @RestController | ||
| @RequestMapping("/api/platform/v1/feedback") | ||
| @SecurityRequirement(name = "apiKey") | ||
| @Tag(name = "Feedback", description = "Feedback management APIs") | ||
| @AllArgsConstructor | ||
| @Validated | ||
| public class FeedbackController { | ||
|
|
||
| private final FeedbackService feedbackService; | ||
|
|
||
| /** | ||
| * API to retrieve all feedback with optional filters. | ||
| * | ||
| * @param reviewerId Filter by reviewer ID | ||
| * @param revieweeId Filter by reviewee I) | ||
| * @param mentorshipCycleId Filter by mentorship cycle ID | ||
| * @param feedbackType Filter by feedback type | ||
| * @param year Filter by year | ||
| * @param isAnonymous Filter by anonymous status | ||
| * @param isApproved Filter by approval status | ||
| * @return List of feedback matching the criteria | ||
| */ | ||
| @GetMapping | ||
| @Operation(summary = "Get all feedback with optional filters") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably the permissions for now can be Admin, mentorship admin and leaders should have permissions |
||
| @ResponseStatus(HttpStatus.OK) | ||
| public ResponseEntity<List<Feedback>> getAllFeedback( | ||
| @Parameter(description = "Reviewer ID") @RequestParam(required = false) final Long reviewerId, | ||
| @Parameter(description = "Reviewee ID") @RequestParam(required = false) final Long revieweeId, | ||
| @Parameter(description = "Mentorship Cycle ID") @RequestParam(required = false) | ||
| final Long mentorshipCycleId, | ||
| @Parameter(description = "Feedback Type") @RequestParam(required = false) | ||
| final FeedbackType feedbackType, | ||
| @Parameter(description = "Year") @RequestParam(required = false) @Min(2000) @Max(2100) | ||
| final Integer year, | ||
| @Parameter(description = "Anonymous status") @RequestParam(required = false) | ||
| final Boolean isAnonymous, | ||
| @Parameter(description = "Approved status") @RequestParam(required = false) | ||
| final Boolean isApproved) { | ||
| final FeedbackSearchCriteria criteria = | ||
| FeedbackSearchCriteria.builder() | ||
| .reviewerId(reviewerId) | ||
| .revieweeId(revieweeId) | ||
| .mentorshipCycleId(mentorshipCycleId) | ||
| .feedbackType(feedbackType) | ||
| .year(year) | ||
| .isAnonymous(isAnonymous) | ||
| .isApproved(isApproved) | ||
| .build(); | ||
|
|
||
| final List<Feedback> feedback = feedbackService.getAllFeedback(criteria); | ||
| return ResponseEntity.ok(feedback); | ||
| } | ||
|
|
||
| /** | ||
| * API to retrieve feedback by ID. | ||
| * | ||
| * @param feedbackId ID of the feedback | ||
| * @return Feedback details | ||
| */ | ||
| @GetMapping("/{feedbackId}") | ||
| @Operation(summary = "Get feedback by ID") | ||
| @ResponseStatus(HttpStatus.OK) | ||
| public ResponseEntity<Feedback> getFeedbackById( | ||
| @Parameter(description = "ID of the feedback") @PathVariable final Long feedbackId) { | ||
| final Feedback feedback = feedbackService.getFeedbackById(feedbackId); | ||
| return ResponseEntity.ok(feedback); | ||
| } | ||
|
|
||
| /** | ||
| * API to create feedback. | ||
| * | ||
| * @param feedbackDto DTO containing feedback data | ||
|
dricazenck marked this conversation as resolved.
|
||
| * @return Created feedback | ||
| */ | ||
| @PostMapping | ||
| @Operation(summary = "Create feedback") | ||
| @ResponseStatus(HttpStatus.CREATED) | ||
| public ResponseEntity<Feedback> createFeedback( | ||
| @Valid @RequestBody final FeedbackDto feedbackDto) { | ||
| final Feedback feedback = feedbackService.createFeedback(feedbackDto); | ||
| return new ResponseEntity<>(feedback, HttpStatus.CREATED); | ||
| } | ||
|
|
||
| /** | ||
| * API to update feedback. | ||
| * | ||
| * @param feedbackId ID of the feedback to update | ||
| * @param feedbackDto DTO with updated feedback data | ||
| * @return Updated feedback | ||
| */ | ||
| @PutMapping("/{feedbackId}") | ||
| @Operation(summary = "Update feedback") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably all the APIs here need to be included the permission using bearer token |
||
| @ResponseStatus(HttpStatus.OK) | ||
| public ResponseEntity<Feedback> updateFeedback( | ||
| @Parameter(description = "ID of the feedback") @PathVariable final Long feedbackId, | ||
| @Valid @RequestBody final FeedbackDto feedbackDto) { | ||
| final Feedback feedback = feedbackService.updateFeedback(feedbackId, feedbackDto); | ||
| return ResponseEntity.ok(feedback); | ||
| } | ||
|
|
||
| /** | ||
| * API to approve feedback (admin only). | ||
| * | ||
| * @param feedbackId ID of the feedback to approve | ||
| * @return No content | ||
| */ | ||
| @PatchMapping("/{feedbackId}/approve") | ||
| @Operation(summary = "Approve feedback (admin only)") | ||
| @ResponseStatus(HttpStatus.OK) | ||
| public ResponseEntity<Void> approveFeedback( | ||
| @Parameter(description = "ID of the feedback") @PathVariable final Long feedbackId) { | ||
| feedbackService.approveFeedback(feedbackId); | ||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| /** | ||
| * API to set feedback anonymous status (to hide/show reviewer name). | ||
| * | ||
| * @param feedbackId ID of the feedback | ||
| * @param isAnonymous true to hide reviewer name, false to show reviewer name | ||
| * @return No content | ||
| */ | ||
| @PatchMapping("/{feedbackId}/anonymous-status") | ||
| @Operation(summary = "Update feedback anonymous status (to hide/show reviewer name)") | ||
| @ResponseStatus(HttpStatus.OK) | ||
| public ResponseEntity<Void> updateFeedbackAnonymousStatus( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this logic should be handled in the get logic, only not another api. But as we discussed this can be worked/changed in another task, so we can merge your changes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will create issue for this new PR. |
||
| @Parameter(description = "ID of the feedback") @PathVariable final Long feedbackId, | ||
| @Parameter(description = "Is anonymous") @RequestParam final Boolean isAnonymous) { | ||
| feedbackService.updateFeedbackAnonymousStatus(feedbackId, isAnonymous); | ||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| /** | ||
| * API to delete feedback. | ||
| * | ||
| * @param feedbackId ID of the feedback to delete | ||
| * @return No content | ||
| */ | ||
| @DeleteMapping("/{feedbackId}") | ||
| @Operation(summary = "Delete feedback") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only admin should be able to delete feedback, and mentorship admins |
||
| @ResponseStatus(HttpStatus.NO_CONTENT) | ||
| public ResponseEntity<Void> deleteFeedback( | ||
| @Parameter(description = "ID of the feedback") @PathVariable final Long feedbackId) { | ||
| feedbackService.deleteFeedback(feedbackId); | ||
| return ResponseEntity.noContent().build(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package com.wcc.platform.domain.exceptions; | ||
|
|
||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| /** Platform Feedback not found exception. */ | ||
| @Slf4j | ||
| public class FeedbackNotFoundException extends RuntimeException { | ||
|
|
||
| public FeedbackNotFoundException(final Long feedbackId) { | ||
| super("Feedback with id: " + feedbackId + " not found."); | ||
| } | ||
|
|
||
| public FeedbackNotFoundException(final String message) { | ||
| super("Feedback not found: " + message); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package com.wcc.platform.domain.platform.feedback; | ||
|
|
||
| import com.wcc.platform.domain.platform.type.FeedbackType; | ||
| import jakarta.validation.constraints.Max; | ||
| import jakarta.validation.constraints.Min; | ||
| import jakarta.validation.constraints.NotBlank; | ||
| import jakarta.validation.constraints.NotNull; | ||
| import java.time.OffsetDateTime; | ||
| import lombok.AllArgsConstructor; | ||
| import lombok.Builder; | ||
| import lombok.EqualsAndHashCode; | ||
| import lombok.Getter; | ||
| import lombok.NoArgsConstructor; | ||
| import lombok.Setter; | ||
| import lombok.ToString; | ||
|
|
||
| /** Feedback for tracking member feedback. */ | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| @ToString | ||
| @EqualsAndHashCode | ||
| @Getter | ||
| @Builder(toBuilder = true) | ||
| public class Feedback { | ||
|
|
||
| @Setter private Long id; | ||
| @NotNull private Long reviewerId; | ||
| private String reviewerName; | ||
| private Long revieweeId; // For MENTOR_REVIEW | ||
| private String revieweeName; | ||
| private Long mentorshipCycleId; // For MENTORSHIP_PROGRAM | ||
| @NotNull private FeedbackType feedbackType; | ||
|
|
||
| @Min(1) | ||
| @Max(5) | ||
| private Integer rating; | ||
|
|
||
| @NotBlank private String feedbackText; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add here also min of 150 characters, at least we can have more valuable feedbacks |
||
|
|
||
| @Min(2000) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably we can add 2024 since we started the WCC |
||
| @Max(2100) | ||
| private Integer year; | ||
|
|
||
| @Setter private Boolean isAnonymous; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should make set of this, this is an information that we need to receive the request, the user say it wants make anonymous. |
||
| @Setter private Boolean isApproved; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not use Boolean here, but use status: pending by default, approved, disabled, deleted. But we can create another ticket to take care of this change. |
||
| private OffsetDateTime createdAt; | ||
| private OffsetDateTime updatedAt; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| package com.wcc.platform.domain.platform.feedback; | ||
|
|
||
| import com.wcc.platform.domain.platform.feedback.validation.ValidFeedback; | ||
| import com.wcc.platform.domain.platform.type.FeedbackType; | ||
| import jakarta.validation.constraints.Max; | ||
| import jakarta.validation.constraints.Min; | ||
| import jakarta.validation.constraints.NotBlank; | ||
| import jakarta.validation.constraints.NotNull; | ||
| import lombok.AllArgsConstructor; | ||
| import lombok.Builder; | ||
| import lombok.Getter; | ||
| import lombok.NoArgsConstructor; | ||
|
|
||
| /** DTO for feedback creation/update - uses IDs for member identification. */ | ||
| @Getter | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| @Builder | ||
| @ValidFeedback | ||
| public class FeedbackDto { | ||
| private Long id; | ||
| @NotNull private Long reviewerId; | ||
| private Long revieweeId; | ||
| private Long mentorshipCycleId; | ||
| @NotNull private FeedbackType feedbackType; | ||
|
|
||
| @Min(1) | ||
| @Max(5) | ||
| private Integer rating; | ||
|
|
||
| @NotBlank private String feedbackText; | ||
|
|
||
| @Min(2000) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same as previous comment in the year |
||
| @Max(2100) | ||
| private Integer year; | ||
|
|
||
| @NotNull private Boolean isAnonymous; | ||
|
|
||
| /** | ||
| * Convert to domain object. | ||
| * | ||
| * @return Feedback domain entity | ||
| */ | ||
| public Feedback merge() { | ||
| return Feedback.builder() | ||
| .id(id) | ||
| .reviewerId(reviewerId) | ||
| .revieweeId(revieweeId) | ||
| .mentorshipCycleId(mentorshipCycleId) | ||
| .feedbackType(feedbackType) | ||
| .rating(rating) | ||
| .feedbackText(feedbackText) | ||
| .year(year) | ||
| .isAnonymous(isAnonymous) | ||
| .isApproved(false) // Default - requires admin approval | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be defined in the init of the attribute. Or constructor. |
||
| .build(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package com.wcc.platform.domain.platform.feedback; | ||
|
|
||
| import com.wcc.platform.domain.platform.type.FeedbackType; | ||
| import lombok.AllArgsConstructor; | ||
| import lombok.Builder; | ||
| import lombok.EqualsAndHashCode; | ||
| import lombok.Getter; | ||
| import lombok.NoArgsConstructor; | ||
| import lombok.ToString; | ||
|
|
||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| @ToString | ||
| @EqualsAndHashCode | ||
| @Getter | ||
| @Builder | ||
| public class FeedbackSearchCriteria { | ||
| private Long reviewerId; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about memberId |
||
| private Long revieweeId; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we discussed maybe rename as mentorId would make clear |
||
| private Long mentorshipCycleId; | ||
| private FeedbackType feedbackType; | ||
| private Integer year; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use Year instead of integer. |
||
| private Boolean isAnonymous; | ||
| private Boolean isApproved; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.