Conversation
Implement API for Community, Mentorship and Mentor feedback.
Add feedback migration file.
Update migration file name.
- Accepted mentorship cycle implementation from main - Resolved GlobalExceptionHandler conflict - Feedback system integrated with cycle tracking - Fixed V34 migration to reference mentorship_cycles table - All 15 mentorship cycle files accepted from main
Update/add mentor.json, feedback.json, mentee.json
|
I will remove two admin files that were updated by accident. |
| feedback.getIsApproved()); | ||
|
|
||
| // Return the last inserted ID | ||
| return jdbc.queryForObject("SELECT LASTVAL()", Long.class); |
There was a problem hiding this comment.
I am not sure that is the best way to get the id of the feedback I would play safe, make void and if we need the id later it should be made select to get the id. If there are concurrency, this could be incorrect.
| /** | ||
| * API to create feedback. | ||
| * | ||
| * @param feedbackDto DTO containing feedback data |
There was a problem hiding this comment.
[CRITICAL] @Valid annotation is missing on @RequestBody
The FeedbackDto has both field-level constraints (@NotNull, @Min, @Max, @NotBlank) and the class-level @ValidFeedback custom constraint — but none of them will trigger because @Valid is absent from the parameter.
// Current — validation is silently skipped
public ResponseEntity<Feedback> createFeedback(@RequestBody final FeedbackDto feedbackDto)
// Fix — add @Valid
public ResponseEntity<Feedback> createFeedback(@Valid @RequestBody final FeedbackDto feedbackDto)The same applies to updateFeedback. Without @Valid, callers can submit a MENTOR_REVIEW without a revieweeId or rating and it will be accepted at the controller layer.
| public Feedback mapRowToFeedback(final ResultSet rs) throws SQLException { | ||
| return Feedback.builder() | ||
| .id(rs.getLong(COLUMN_ID)) | ||
| .reviewerId(rs.getLong(COLUMN_REVIEWER_ID)) |
There was a problem hiding this comment.
[CRITICAL] SELECT LASTVAL() is not thread-safe
LASTVAL() returns the last value used by any sequence in the current session. Under concurrent writes, another thread could insert into a different table with a sequence between your INSERT and the LASTVAL() call, causing you to fetch the wrong ID — or the wrong sequence entirely.
Use RETURNING id in the INSERT statement instead, which atomically returns the generated key:
private static final String INSERT_SQL =
"INSERT INTO feedback (reviewer_id, reviewee_id, mentorship_cycle_id, feedback_type_id, "
+ "rating, feedback_text, feedback_year, is_anonymous, is_approved) "
+ "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING id";
public Long addFeedback(final Feedback feedback) {
return jdbc.queryForObject(
INSERT_SQL,
Long.class,
feedback.getReviewerId(),
feedback.getRevieweeId(),
// ...
);
}Alternatively use KeyHolder + GeneratedKeyHolder if you prefer the standard JdbcTemplate pattern.
| * match is found, the default {@code COMMUNITY_GENERAL} type is returned. | ||
| * | ||
| * @param typeId the integer ID representing a specific {@code FeedbackType} | ||
| * @return the {@code FeedbackType} that matches the given ID, or {@code COMMUNITY_GENERAL} if no |
There was a problem hiding this comment.
[CRITICAL] Silent fallback to COMMUNITY_GENERAL masks data corruption
When fromId() encounters an unknown typeId, it silently returns COMMUNITY_GENERAL instead of failing. If a row in the database ever has an invalid feedback_type_id (e.g., due to a migration bug or direct DB edit), all those records will appear as COMMUNITY_GENERAL with no indication that something is wrong.
// Current: silently wrong
return COMMUNITY_GENERAL;
// Fix: throw a clear exception
throw new IllegalArgumentException("Unknown FeedbackType id: " + typeId);This will surface data integrity problems immediately rather than silently misclassifying feedback.
| (3, 'MENTORSHIP_PROGRAM', 'Feedback about the mentorship program') | ||
| ON CONFLICT (id) DO NOTHING; | ||
|
|
||
| -- Feedback table |
There was a problem hiding this comment.
[WARNING] DB rating constraint allows 0 but Java validator requires >= 1
The SQL check is rating >= 0 AND rating <= 5, but FeedbackDto uses @Min(1). A rating of 0 can be stored directly in the database (e.g., via a migration or direct SQL), but would be rejected by the API — inconsistent range.
Also the constraint does not enforce that MENTORSHIP_PROGRAM requires a mentorship_cycle_id, while the Java validator does. Consider adding a second check constraint:
-- Fix rating range
rating INTEGER CHECK (rating >= 1 AND rating <= 5),
-- Add missing cycle constraint for MENTORSHIP_PROGRAM
CONSTRAINT feedback_mentorship_program_constraint CHECK (
feedback_type_id != 3 OR mentorship_cycle_id IS NOT NULL
)| @@ -0,0 +1,8 @@ | |||
| { | |||
| "reviewerId": 1, | |||
There was a problem hiding this comment.
[WARNING] Debug/test file committed to the repository root
feedback-post-request.json looks like a local Swagger/curl test file that was accidentally included. It should not be committed — it adds noise to the repo and would need maintenance as the API evolves.
Please delete this file before merging.
| @Override | ||
| public Feedback update(final Long id, final Feedback entity) { | ||
| feedbackMapper.updateFeedback(entity, id); | ||
| return entity; |
There was a problem hiding this comment.
[WARNING] update() returns the passed-in entity, not what is in the database
After updateFeedback() runs, the returned entity object will not have the updated_at timestamp set (it comes from CURRENT_TIMESTAMP in SQL), and reviewerName/revieweeName are not persisted at all. Callers of updateFeedback will see stale metadata in the response.
Consider re-fetching after update, similar to what create() already does:
@Override
public Feedback update(final Long id, final Feedback entity) {
feedbackMapper.updateFeedback(entity, id);
return findById(id).orElseThrow();
}|
|
||
| /** | ||
| * Update feedback. Only reviewer or admin can update. | ||
| * |
There was a problem hiding this comment.
[WARNING] reviewerName and revieweeName are set in memory but not stored in the DB
After feedbackRepository.create(feedback), the returned object will have reviewerName=null because mapRowToFeedback() hardcodes both name fields to null (the column does not exist in the schema). The names are enriched here before create(), but the INSERT does not persist them.
This means:
createFeedbackreturns the correct names (set on the entity before saving)getFeedbackByIdwill returnnullfor both names on any subsequent read
If names should be returned on read, either join with the members table in the SELECT query, or — if they do not need to be stored — document clearly that names are only present on the create/update response and enrich them in the service on every read.
A JOIN in SELECT_BY_ID would be the most consistent approach:
SELECT f.*, m.full_name AS reviewer_name, m2.full_name AS reviewee_name
FROM feedback f
LEFT JOIN members m ON m.id = f.reviewer_id
LEFT JOIN members m2 ON m2.id = f.reviewee_id
WHERE f.id = ?| import static org.hamcrest.Matchers.is; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.argThat; | ||
| import static org.mockito.ArgumentMatchers.eq; |
There was a problem hiding this comment.
[WARNING] Tests are missing @DisplayName — project convention
Per the project conventions in CLAUDE.md, all tests should use @DisplayName with Given-When-Then format:
@Test
@DisplayName("Given a valid MENTOR_REVIEW DTO, when createFeedback is called, then it returns HTTP 201 with the created feedback")
void testCreateFeedbackReturnsCreated() { ... }This applies to all test methods in FeedbackControllerTest, FeedbackDtoValidationTest, FeedbackServiceTest, PostgresFeedbackRepositoryTest, and FeedbackMapperTest. Also, test method names could follow the should prefix convention (e.g., shouldReturnCreatedWhenFeedbackIsValid).
| * Create new feedback. Validates that reviewer/reviewee exists (if provided). | ||
| * | ||
| * @return created Feedback | ||
| */ |
There was a problem hiding this comment.
[INFO] isAnonymous privacy flag is not enforced in read operations
The PR description states: "isAnonymous=true → reviewerName returns null — Protects reviewer identity". However, getAllFeedback() and getFeedbackById() return whatever reviewerName comes from the DB (which is always null currently — see the related comment about mapRowToFeedback). There is no active nulling-out of reviewerName based on isAnonymous.
When the names issue is fixed (via a JOIN), this service method will need to clear the reviewerName before returning:
if (Boolean.TRUE.equals(feedback.getIsAnonymous())) {
feedback.setReviewerName(null);
}This is worth tracking as a TODO before this feature goes live.
womencodingcommunity
left a comment
womencodingcommunity
left a comment
There was a problem hiding this comment.
Great effort on this feature — the architecture is well-thought-out, the layered structure is clean, and the custom @ValidFeedback constraint is a nice pattern. The test coverage is a solid start too. There are three critical issues that need to be resolved before merging, and a few warnings worth addressing:
Must fix before merge:
@Validmissing on@RequestBody— neither field-level nor class-level validation triggers without it (createFeedback,updateFeedback)SELECT LASTVAL()race condition — useRETURNING idin the INSERT insteadFeedbackType.fromId()silent fallback — throwsIllegalArgumentExceptioninstead of returningCOMMUNITY_GENERALfor unknown IDs
Should address:
feedback-post-request.jsoncommitted to repo root — please remove- DB
rating CHECK >= 0vs Java@Min(1)inconsistency; missing DB constraint forMENTORSHIP_PROGRAMrequiringmentorship_cycle_id update()returns in-memory entity —updated_atand names will be stale in responsereviewerName/revieweeNameset in service but not persisted or joined on read — they will always benullfromGETendpointsisAnonymousprivacy logic not enforced on GET responses- All test methods need
@DisplayNamewith Given-When-Then format (project convention)
The TODO items in the PR description (PATCH methods, more Swagger testing, reaching 70% coverage) should be tracked as follow-up issues before this is considered production-ready.
Update Feedback.java and FeedbackSearchCriteria to use only needed lombok annotations Update FeedbackController.java - use Patch instead of Put for approveFeedback and setFeedbackAnonymousStatus Update/Add tests Feedback coverage is above 70% threshold. Implement PMD suggestions.
"chore: rename feedback migration to V36 to maintain chronological order after main merge"
# Conflicts: # src/main/java/com/wcc/platform/configuration/GlobalExceptionHandler.java
|
3 participants
Description
Implement feedback APIs.
Three Feedback Types:
Architecture
Tables Created
1. feedback_types (Lookup Table)
2. feedback (Main Table)
Constraints
Data Privacy
isAnonymous=true→reviewerNamereturnsnullProtects reviewer identity
Reviewer can toggle anonymity
Default:
isApproved=falseOnly approved feedback visible to public
Admin controls publication
Feedback API Coverage Analysis Report
Generated: April 6, 2026
Source: build/reports/jacoco/test/jacocoTestReport.xml
Executive Summary
✅ PASS: Feedback API meets 70% coverage threshold on all metrics!
The Feedback API implementation has excellent test coverage across all layers:
TODO:
Related Issue
Please link to the issue here
Change Type
Screenshots
Pull request checklist
Please check if your PR fulfills the following requirements: