PR#49 Link Visit with Vet (#37), reimplemented unguided by Opus

Review

7 implementation assumptions · 6 open review issues · 3 auto-fixed

Round I · while coding

Implementation assumptions

Where the ticket was ambiguous, the reading that was taken — and under Read the other way, the reading that was not. Nothing here is a defect, and nothing here is in the diff: it is the only pile no pass, script or reviewer could reconstruct afterwards.

1. assumption0.45 The seed gives some visits a vet and deliberately withholds one from others

   Below a coin flip, and the entry I most expect to be argued with: #37 does not mention the seed, and its sentence about pre-existing rows keeping no vet reads, taken strictly, as an instruction to leave the dataset alone. I weighted demo value over that strict reading, and which vet got which visit is invention with no source but my own taste.

   Read the other way: leave every seeded visit unattended, since the ticket only says old rows keep no vet

   a demo dataset where nothing has a vet cannot show the feature at all, and one where everything does never exercises the "none" rendering. The older batch (the four visits on Alice's cats) stays vet-less as the pre-change history; Milton's three cover both.

   R__seed.sql:118new code

   +118-- vet_id is deliberately NULL on some rows: an appointment is routinely booked before anyone

2. assumption0.55 vet_id is ON DELETE SET NULL, so deleting a vet is never blocked

   The index on vet_id is there for the same reason, not for searching (which is explicitly out of scope): without it the FK check behind a vet delete scans visits. It matches how pet_id and owner_id are already indexed.
   
   The confidence is barely above a coin flip because #37 never mentions deleting a vet at all — I inferred the rule from a sentence written about booking — and the ticket's own insistence that historical visits keep their state cuts the other way just as well: a clinic that wants to know who saw the animal in 2024 would rather the delete failed than have the answer quietly erased.

   Read the other way: the default RESTRICT — a vet who has attended anything can no longer be deleted, and DELETE /api/vets/{id} starts failing on real data

   #37 says a visit with no vet is normal and "never an error", so a vet's departure landing their old visits in that case is the reading that keeps the ticket's own rule true. RESTRICT would invent a new failure mode the ticket never asks for.

   V4__visit_vet.sql:5new file

   5ALTER TABLE visits ADD COLUMN vet_id INT REFERENCES vets (id) ON DELETE SET NULL;

3. assumption0.6 MCP's create_visit and the chatbot were left alone

   The number is low because #37 contradicts itself on exactly this point: its opening line says the vet should show "everywhere throughout the app", and its third requirement then narrows that to "today that means the owner's page and the all-visits screen". I took the narrowing as the operative one, but list_visits does show an owner their visits with details, so a reader who weights the opening line will say I left a screen out.

   Read the other way: read "everywhere throughout the app" to include the MCP surface and let the assistant book with a vet

   the ticket's own four requirements name the booking form, the edit form, the owner's page and the all-visits screen. The MCP tools keep booking without a vet, which is a valid visit under this change, so nothing there breaks.

   PetClinicMcp.java:48unchanged

    48name = "create_visit",

4. assumption0.7 An unknown vetId is a 404, not a quietly unattended visit

   #37 says nothing about bad input, so none of this came from the ticket — but every other unknown id in this codebase already answers 404 through .orElseThrow(), and only petOfId pulls the other way. That precedent is what holds the number up; the consistency argument is what keeps it off 0.9.

   Read the other way: the house petOfId pattern — build a Vet carrying only the id, run no query, and let the foreign key reject a bad one as a 500

   petId already takes the stub route, so consistency argued for it; correctness won. A typo'd vet id is a client error and reads like one, at the cost of one lean query per booking.

   VisitRestController.java:93-95new code

   +93private Vet attendingVet(Integer vetId) {
   +94    return vetId == null ? null : vetRepository.findByIdWithoutSpecialties(vetId).orElseThrow();
   +95}

5. assumption0.8 The vet rides on VisitDto as three flat fields, not a nested VetDto

   Read the other way: vet: VetDto, the shape the vet screens already speak

   VisitDto already flattens the owner into ownerId/ownerFirstName/ ownerLastName, and a nested VetDto carries specialties, which would put a vet's whole specialty list on every row of the all-visits table.

   VisitDto.java:46new code

   +46private @Nullable Integer vetId;

6. assumption0.85 Two generated files were written by hand because their generators cannot run here

   npm ci and pip install are both blocked in this environment, so npm run generate:api could not run. api-types.ts was written to match openapi-typescript's output exactly. DB.puml was too — and that one is verified: the pre-commit hook bootstrapped its own venv, regenerated the file, and staged a result byte-identical to what I had written. The frontend's Karma suite and ng build could not be run at all for the same reason; the backend suite, Spotless and every guardrail test are green.
   
   This entry is a confession about the environment, not a reading of #37, so its number scores something narrower than the others: how sure I am that hand-writing beat leaving the files stale. Quite sure — stale types break the build for whoever pulls next — and DB.puml came back byte-identical from the real generator, which is as close to proof as this gets. It is not higher because api-types.ts got no such check.

   Read the other way: leave them stale and let CI's regenerate-and-auto-commit fix them

   stale TS types mean visit.vetId does not compile, so the working tree would not build for anyone who pulled it.

   api-types.ts:346new code

   +346/** @description First name of the vet (server-populated). */

7. assumption0.9 A visit with no vet reads "none", and the dropdown offers "-- none --"

   The highest number here, because this is the one decision the ticket argues out loud and even names the wrong answers. It is not 1.0 only because the ticket dictates the meaning and not the string: an em dash would satisfy every word of the requirement too.

   Read the other way: an em dash, or leaving the cell empty and letting the reader infer

   #37's fourth requirement is that the visit reads as *having none* — not "Unknown", not blank-because-broken. A blank cell is indistinguishable from a failed load, which is the thing the requirement rules out.

   visit.ts:14-17new code

   +14export function vetLabel(visit: Visit): string {
   +15  const name = [visit.vetFirstName, visit.vetLastName].filter(part => !!part).join(' ');
   +16  return name || 'none';
   +17}

Round II · code review

Open review issues

Findings the agent read and said no to, with its reason. These are closed decisions, not a queue: your job here is to agree or disagree, and the last three were raised by the agent against itself rather than by a pass.

1. worth a look/code-review (the same finding, its design half) Make PUT /api/visits/{id} keep the vet when the body omits vetId

   The endpoint already replaces date and description unconditionally — a body without date nulls the date. Special-casing vetId would make one field in one DTO behave differently from its neighbours, and telling absent from explicit-null needs a JsonNullable wrapper that nothing else in this codebase uses. The ticket's second requirement is that clearing the field sticks; unconditional assignment is what makes it stick.

   "assign only when the field is present" is precisely the bug #37 tells me not to repeat.

   VisitRestController.java:88new code

   +88currentVisit.setVet(attendingVet(visitDto.getVetId()));

2. worth a lookme, while writing the test for the vet-picker fix A denied @PreAuthorize answers 500 instead of 403

   handleGeneralException catches Exception, which includes AuthorizationDeniedException, so every authorization failure in rest/ is reported as an internal error — and the ProblemDetail echoes the exception's own message back to the caller. Worth its own ticket. ownerAdmin_cannotAddAVet therefore asserts the write is refused rather than pinning the status code, so fixing this will not fail it.

   it is global error handling on every endpoint, and #37 is about a column on visits.

   ExceptionControllerAdvice.java:75-81unchanged

    75public ResponseEntity<ProblemDetail> handleGeneralException(Exception e, HttpServletRequest request) {
    76    log.error("An unexpected error occurred: {}", e.getMessage(), e);
    77    ProblemDetail pd = buildProblemDetail(e.getMessage(),
    78            e.getMessage(),
    79            HttpStatus.INTERNAL_SERVER_ERROR, request);
    80    return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(pd);
    81}

3. nit/code-review (duplication) Collapse the duplicated attendingVet helper

   I first wrote it as one default method on VetRepository, which is where it belongs. It fails at runtime: Spring Data's NullnessMethodInvocationValidator rejects a null argument to any repository method, so "no vet chosen" became a 500 — and neither org.springframework.lang.@Nullable nor org.jspecify.@Nullable on the parameter talks it out of that. A @Component wrapping one ternary would buy less than it costs.

   three lines shared by two controllers that already duplicate the whole booking flow; the honest fix is collapsing those, which is a bigger change than #37 asks for.

   OwnerRestController.java:201new code

   +201log.debug("Attending vet: {}", vet.getLastName());

4. nitme, reading the repo before starting The generated sequence diagram for booking with a vet is orphaned

   This file predates my commit (it arrived with 0d534233 and survived the revert of the earlier attempt at #37). It deep-links to add-visit.spec.ts:52 and OwnerRestController.bookVisit:193, neither of which exists, and describes a VetRepository.getByIdOrNull I deliberately did not build. It is stale either way; my change does not make it staler, and I did not add the e2e test it claims to picture.

   regenerating it needs the whole stack traced — browser, backend, Tempo — and the frontend's dependencies are not installable here.

   add-visit.spec.ts.add-a-visit-attended-by-a-vet.genseq.puml:59unchanged

    59activate Backend

5. nitme, anticipating SonarCloud java:S107 (this repo lowers the cap to 5) OwnerRestController's constructor is now nine parameters

   Resolving the vet needs a repository, and mapper → repository is forbidden by docs/packages.puml. Sonar will re-flag S107 because the constructor's lines changed, so this will show up as a new-code finding on a pre-existing violation.

   it was already eight, the ninth collaborator is genuinely needed, and every alternative is worse.

   OwnerRestController.java:67unchanged

    67

6. nitme The user manual still describes the visit screens without a vet

   manual.md and its screenshots are regenerated by crawling the running UI, which needs the frontend's dependencies installed; hand-editing the prose would leave it disagreeing with its own screenshots.

   manual.md:100unchanged

    100To add a visit, open the owner's record (see [Owners](#owners)), find the pet that came in for the visit, and click *Add Visit* in that pet's section. The *New Visit* form opens with the pet's identifying details at the top and the pet's previous visits listed below. Pick the *Date* from the calendar, write a *Description* of what was done, and click *Add Visit* to save. The visit appears under that pet on the owner record and in the chronological *Visits* list. The *Add Visit* button at the bottom of the list page only works in the context of a specific pet — always start from the owner's record.

Auto-fixed

Read off review-points.md, committed with the fixes. Each one names the reviewer that raised it and shows the diff against a8cd9973, the implementation commit — so what the review changed is separable from what the feature changed.

1. fixed/code-review (cross-role read on a widened screen) The vet picker could not load on the screens that need it

   GET /api/vets sat under the class's hasRole(VET_ADMIN), but the booking and edit forms are owner-admin screens. An owner-admin-only user opening *New Visit* got an error and a permanently empty combo — the one thing #37 asks for most. PetTypeRestController already solves the identical cross-role read, so this follows it, except narrower: only listVets() moves to hasAnyRole(OWNER_ADMIN, VET_ADMIN); adding, editing and deleting vets stay VET_ADMIN.
   
   Worth recording how the reviewer's scenario differed from reality. It predicted a 403; the run produces a **500**, because ExceptionControllerAdvice's catch-all handler swallows AuthorizationDeniedException. Same broken picker, uglier failure. It is also why the two new assertions live in BasicAuthenticationConfigTest and not next to the controller: petclinic.security.enable is false everywhere else, so every @WithMockUser(roles = …) in the other suites decorates without enforcing, and a role test written beside VetTest passes whatever the rule says. The first version of this fix's test did exactly that and proved nothing.

   VetRestController.java+3 −0 vs a8cd9973

   		public class VetRestController {
   45	45	this.specialtyRepository = specialtyRepository;
   46	46	}
   47	47
   	48	+ // Widened past the class's VET_ADMIN, like pettypes: the visit forms are owner-admin screens
   	49	+ // and have to fill a vet picker. Only this read moves; adding and editing vets stay VET_ADMIN.
   	50	+ @PreAuthorize("hasAnyRole(@roles.OWNER_ADMIN, @roles.VET_ADMIN)")
   48	51	@GetMapping
   49	52	@ApiResponse(responseCode = "200", description = "OK",
   50	53	content = @Content(mediaType = "application/json",

   VetRestController.java:50new code

   +50@PreAuthorize("hasAnyRole(@roles.OWNER_ADMIN, @roles.VET_ADMIN)")

2. fixed/code-review (efficiency) Booking a visit read the vet's whole specialty list to set one foreign key

   VetRepository.findById LEFT JOIN FETCHes v.specialties, which is right for the vet screens and pure waste for attendingVet, which reads nothing off the vet but its identity. Both booking paths now use a lean findByIdWithoutSpecialties.

   VetRepository.java+4 −0 vs a8cd9973

   		public interface VetRepository extends Repository<Vet, Integer> {
   14	14	@Query("SELECT v FROM Vet v LEFT JOIN FETCH v.specialties WHERE v.id = :id")
   15	15	Optional<Vet> findById(int id);
   16	16
   	17	+ /** For callers that only need the vet as a reference — booking a visit reads no specialty. */
   	18	+ @Query("SELECT v FROM Vet v WHERE v.id = :id")
   	19	+ Optional<Vet> findByIdWithoutSpecialties(int id);
   	20	+
   17	21	void save(Vet vet);
   18	22
   19	23	void delete(Vet vet);

   VetRepository.java:19new code

   +19Optional<Vet> findByIdWithoutSpecialties(int id);

3. fixed/code-review (the PUT-clears-the-vet scenario) The Cucumber update step dropped the vet out of its PUT body

   iUpdateVisitDescription rebuilt the body from the GET but stopped at description, so after this change renaming a visit unassigned its vet. The step now carries vetId through like every other field, and visits.feature gained the scenario that would have caught it. I kept the endpoint's semantics (see Ignored) and fixed the client, which is where the defect actually was.

   VisitSteps.java+26 −2 vs a8cd9973

   		public class VisitSteps {
   70	70	http.rememberId("visit:current", visitId);
   71	71	}
   72	72
   	73	+ @Given("a visit for {string} on {string} described as {string} attended by vet {string}")
   	74	+ public void aVisitAttendedByVet(String petName, String date, String description, String vetLastName) {
   	75	+ int petId = http.idOf("pet:" + petName);
   	76	+ Integer vetId = jdbc.queryForObject(
   	77	+ "SELECT id FROM vets WHERE last_name = ?", Integer.class, vetLastName);
   	78	+ Integer visitId = jdbc.queryForObject(
   	79	+ "INSERT INTO visits (pet_id, visit_date, description, vet_id)"
   	80	+ + " VALUES (?, ?, ?, ?) RETURNING id",
   	81	+ Integer.class, petId, LocalDate.parse(date), description, vetId);
   	82	+ http.rememberId("visit:current", visitId);
   	83	+ }
   	84	+
   73	85	@When("I update that visit's description to {string}")
   74	86	public void iUpdateVisitDescription(String newDescription) {
   75	87	int visitId = http.idOf("visit:current");
   76	88	var existing = RestAssured.given().baseUri(http.baseUri()).get("/api/visits/" + visitId);
   77	89	assertThat(existing.statusCode()).isEqualTo(200);
   78	90
   	91	+ // PUT replaces the whole visit, so every field it does not resend is cleared — vetId
   	92	+ // included. Read it back off the GET rather than omitting it, or renaming a visit
   	93	+ // would quietly unassign the vet who attended it.
   79	94	String body = """
   80		- {"id":%d,"petId":%d,"date":"%s","description":"%s"}
   	95	+ {"id":%d,"petId":%d,"date":"%s","description":"%s","vetId":%s}
   81	96	""".formatted(
   82	97	visitId,
   83	98	existing.jsonPath().getInt("petId"),
   84	99	existing.jsonPath().getString("date"),
   85		- newDescription);
   	100	+ newDescription,
   	101	+ existing.jsonPath().get("vetId"));
   86	102
   87	103	http.setLastResponse(RestAssured.given()
   88	104	.baseUri(http.baseUri())
   		public class VisitSteps {
   98	114	assertThat(response.statusCode()).isEqualTo(200);
   99	115	assertThat(response.jsonPath().getString("description")).isEqualTo(expected);
   100	116	}
   	117	+
   	118	+ @Then("the visit is still attended by vet {string}")
   	119	+ public void theVisitIsStillAttendedBy(String vetLastName) {
   	120	+ int visitId = http.idOf("visit:current");
   	121	+ var response = RestAssured.given().baseUri(http.baseUri()).get("/api/visits/" + visitId);
   	122	+ assertThat(response.statusCode()).isEqualTo(200);
   	123	+ assertThat(response.jsonPath().getString("vetLastName")).isEqualTo(vetLastName);
   	124	+ }
   101	125	}

   VisitSteps.java:95new code

   +95{"id":%d,"petId":%d,"date":"%s","description":"%s","vetId":%s}

Demo

API

Data

The concepts, as the team drew them

Tests

Link Visit with Vet #37

victorrentea opened on Jun 13, 2026 Semantic Test Coverage

The Visit should be linked to the vet that attended that consultation. Visit should display its vet everywhere throughout the app.

---

What it has to do

1. Booking a visit lets you choose the vet, and lets you not choose one. The vet is optional: half the time the appointment is booked before anyone knows who is taking it, and forcing a choice there produces bad data rather than information.
2. Editing a visit can change the vet, and can remove it. Clearing the field must persist as empty. We had this with pet types: the old value kept coming back.
3. Wherever a visit is shown with its details, the vet is shown too. Today that means the owner's page and the all-visits screen.
4. A visit with no vet reads as having none. Not "Unknown", not blank-because-broken, and never an error. Visits created before this change have no vet and will not get one.

Out of scope

Searching or filtering visits by vet.

Legend:fully coveredpartiallyexecutedmissingN/A

UIclicks the screenAPIREST/MCPunitone isolated component

Sequence

 59@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
 60class AddVisitApiTest {
 ⋯11 lines not shown
 72    @GenerateSequence
 73    @Test
 74    @Order(1)
 75    void addsAVisitToAnExistingPet() throws Exception {
 76        given("an owner with at least one pet exists");
 77        JsonNode owner = anOwnerWithAPet();
 78        int ownerId = owner.path("id").asInt();
 79        int petId = owner.path("pets").get(0).path("id").asInt();
 80
 81        when("the owner detail page is opened");
 82        call(mockMvc, get("/api/owners/{ownerId}", ownerId)).andExpect(status().isOk());
 83
 84        and("a visit is added for the first pet");
 85        String description = "Annual check-up " + System.currentTimeMillis();
 86        call(mockMvc, post("/api/owners/{ownerId}/pets/{petId}/visits", ownerId, petId)
 87                .contentType(MediaType.APPLICATION_JSON)
 88                .content(mapper.writeValueAsString(Map.of("date", VISIT_DATE, "description", description))))
 89                .andExpect(status().isCreated());
 90
 91        then("the visit is listed under the pet");
 92        JsonNode reloaded = json(call(mockMvc, get("/api/owners/{ownerId}", ownerId))
 93                .andExpect(status().isOk()));
 94        assertThat(reloaded.path("pets").get(0).path("visits").toString())
 95                .contains(description)
 96                .contains(VISIT_DATE);

+51test('Add a visit attended by a vet',
+52  {tag: [GENERATE_SEQUENCE_TAG]},
+53  async ({page}) => {
+54    const {ownerId} = await an_owner_with_at_least_one_pet_exists();
+55
+56    await open_owner_detail_page(page, ownerId);
+57    await click_add_visit_for_first_pet(page, 'Add Visit');
+58    const description = await fill_visit_date_and_unique_description(page, VISIT_DATE);
+59    const vetName = await select_first_vet_in_visit_form(page);
+60    await submit_visit_form(page);
+61
+62    await expect_back_on_owner_detail_page(page, ownerId);
+63    await expect_pet_visit_list_shows_vet(page, VISIT_DATE, description, vetName);
 64  });

 34test('Add a visit to an existing pet from the owner detail page',
 35  {tag: [GENERATE_SEQUENCE_TAG]},
 36  async ({page}) => {
 37    const {ownerId} = await an_owner_with_at_least_one_pet_exists();
 38
 39    await open_owner_detail_page(page, ownerId);
 40    await click_add_visit_for_first_pet(page, 'Add Visit');
 41    const description = await fill_visit_date_and_unique_description(page, VISIT_DATE);
 42    await submit_visit_form(page);
 43
 44    await expect_back_on_owner_detail_page(page, ownerId);
 45    await expect_pet_visit_list_contains(page, VISIT_DATE, description);
+46    await expect_pet_visit_list_shows_no_vet(page, VISIT_DATE, description);
+47  });