feat: allow owners to delete pets#266
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request implements pet deletion functionality by introducing a delete button in the frontend's PetDetail component, with a business rule enforcement on the backend that prevents deletion if medical records exist for the pet. The flow integrates frontend confirmation dialogs, API calls via petService, and backend validation through medical record existence checks. Changes
Sequence DiagramsequenceDiagram
actor User
participant PetDetail as PetDetail Component
participant Dialog as Confirmation Dialog
participant petService as petService API
participant Backend as PetService Backend
participant MedRepo as MedicalRecordRepository
participant DB as Database
User->>PetDetail: Click delete button
PetDetail->>Dialog: Show confirmation dialog
Dialog->>Dialog: User confirms deletion
Dialog->>petService: deletePet(pet.id)
petService->>Backend: HTTP DELETE /pets/{id}
Backend->>MedRepo: existsByPetId(petId)
MedRepo->>DB: Query for medical records
DB-->>MedRepo: Records found or not found
alt Records Found
MedRepo-->>Backend: true
Backend-->>petService: BusinessRuleException
petService-->>Dialog: Error response
else No Records Found
MedRepo-->>Backend: false
Backend->>DB: Delete pet
DB-->>Backend: Deletion success
Backend-->>petService: HTTP 200 OK
petService->>PetDetail: onDelete callback
PetDetail->>User: Navigate back to pet list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/PetDetail.jsx (1)
11-26:⚠️ Potential issue | 🟡 MinorClient-side delete gating relies on a fetch that may silently fail.
If
medicalRecordService.getRecordsByPet(pet.id)rejects, the.catchat line 20 setspetRecordsto[], which makeshasHistoryfalseand enables the "Ta bort djurprofil" button even when history actually exists. The backend will still reject the delete viaBusinessRuleException, so no data is lost, but the owner gets an error alert instead of the pre-emptive Info notice.Consider tracking a separate load error and treating it as "history status unknown" — either keep the button disabled with a different message, or at least surface a lightweight error state so the user understands why the UX differs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/PetDetail.jsx` around lines 11 - 26, The fetch for medical records should not treat failures as "no history" — add a separate load error state (e.g., recordsLoadError via setRecordsLoadError) and update the useEffect (that calls medicalRecordService.getRecordsByPet) to: reset recordsLoadError=false before fetching, set petRecords only on success, set recordsLoadError=true in the .catch (do not set petRecords to [] on error), and keep the cancelled guard logic; then change the delete-gating logic that uses hasHistory (and any button/notice rendering) to consider the load error (e.g., treat history as unknown/disable delete when recordsLoadError is true or surface an explicit lightweight error/info message) so the "Ta bort djurprofil" button is not enabled on fetch failure.
🧹 Nitpick comments (2)
frontend/src/pages/PetDetail.jsx (1)
28-47: Delete flow looks correct; a couple of chill notes.
window.confirm/alertsatisfies the issue's "confirmation dialog" requirement but is inconsistent with the rest of the app's custom UI. Consider a styled modal for parity when time permits.- Order of operations is right:
await petService.deletePet→await onDelete()(refresh parent lists) →onBack()(navigate). That avoids a flash of a stale pet in the owner list.setIsDeleting(false)in thefinallywill usually run afteronBack()has unmounted this component. React 19 no longer warns about this so it's harmless, just dead work. If you'd rather avoid it, track mount via the same pattern used in theuseEffect(acancelledflag) or skip the reset on the success path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/PetDetail.jsx` around lines 28 - 47, The delete handler handleDeletePet is fine but can avoid setting state after unmount: wrap the async flow with a mounted/cancelled flag (same pattern used in your component's useEffect) and only call setIsDeleting(false) if still mounted, or alternatively skip calling setIsDeleting(false) on the success path right before invoking onBack(); keep the current window.confirm/alert for now (replace with the app modal later for UI parity) and leave references to petService.deletePet, onDelete, onBack, and setIsDeleting intact so the ordering (delete → onDelete → onBack) remains unchanged.src/main/java/org/example/vet1177/services/PetService.java (1)
144-156: Align the error message across the pre-check and the fallback catch.The pre-check at line 147 and the
DataIntegrityViolationExceptionhandler at line 155 raise the same semantic error with different wording. Since the frontend surfaceserr.response?.data?.messagedirectly to the user (seefrontend/src/pages/PetDetail.jsxline 42), the user could see two different messages for the same root cause. Consider reusing a single constant.♻️ Proposed tweak
+ final String recordsLinkedMsg = + "Djuret kan inte raderas eftersom journaler/ärenden finns kopplade"; + boolean hasRecords = medicalRecordRepository.existsByPetId(petId); if (hasRecords) { - throw new BusinessRuleException("Djuret kan inte raderas eftersom det finns registrerade journaler/ärenden."); + throw new BusinessRuleException(recordsLinkedMsg); } try { petRepository.delete(pet); petRepository.flush(); log.info("Deleted pet id={}", petId); } catch (DataIntegrityViolationException e) { - throw new BusinessRuleException("Djuret kan inte raderas eftersom journaler finns kopplade"); + log.warn("Cannot delete pet id={} due to related records", petId, e); + throw new BusinessRuleException(recordsLinkedMsg); }Also note the catch block currently swallows
ewithout logging — alog.warn(..., e)would help diagnose any non-medical-record FK constraints that slip past the pre-check (this matches the pattern inUserService.deleteUsershown in the context snippet).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/PetService.java` around lines 144 - 156, The two error messages for the same deletion failure should be unified and the exception logged: replace the duplicate string literal with a single constant (e.g. PET_DELETE_HAS_RECORDS_MESSAGE) used in the pre-check (medicalRecordRepository.existsByPetId(petId) -> BusinessRuleException) and in the DataIntegrityViolationException handler thrown after petRepository.delete/flush; also log the caught exception (use log.warn with the exception) before throwing the BusinessRuleException so unexpected FK issues are visible (refer to BusinessRuleException, medicalRecordRepository.existsByPetId, petRepository.delete, petRepository.flush, DataIntegrityViolationException, and log.warn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/pages/PetDetail.jsx`:
- Around line 103-122: The delete button is disabled when hasHistory is true but
lacks programmatic explanation; update the button rendered in the block using
handleDeletePet / hasHistory / isDeleting so that when hasHistory is true it
includes a descriptive title and an aria-describedby pointing to the explanatory
message element, and give that explanatory <div> a stable id (e.g.,
"delete-disabled-reason") so screen readers announce why the button is disabled;
ensure the aria-describedby is only present when hasHistory is true and removed
otherwise.
---
Outside diff comments:
In `@frontend/src/pages/PetDetail.jsx`:
- Around line 11-26: The fetch for medical records should not treat failures as
"no history" — add a separate load error state (e.g., recordsLoadError via
setRecordsLoadError) and update the useEffect (that calls
medicalRecordService.getRecordsByPet) to: reset recordsLoadError=false before
fetching, set petRecords only on success, set recordsLoadError=true in the
.catch (do not set petRecords to [] on error), and keep the cancelled guard
logic; then change the delete-gating logic that uses hasHistory (and any
button/notice rendering) to consider the load error (e.g., treat history as
unknown/disable delete when recordsLoadError is true or surface an explicit
lightweight error/info message) so the "Ta bort djurprofil" button is not
enabled on fetch failure.
---
Nitpick comments:
In `@frontend/src/pages/PetDetail.jsx`:
- Around line 28-47: The delete handler handleDeletePet is fine but can avoid
setting state after unmount: wrap the async flow with a mounted/cancelled flag
(same pattern used in your component's useEffect) and only call
setIsDeleting(false) if still mounted, or alternatively skip calling
setIsDeleting(false) on the success path right before invoking onBack(); keep
the current window.confirm/alert for now (replace with the app modal later for
UI parity) and leave references to petService.deletePet, onDelete, onBack, and
setIsDeleting intact so the ordering (delete → onDelete → onBack) remains
unchanged.
In `@src/main/java/org/example/vet1177/services/PetService.java`:
- Around line 144-156: The two error messages for the same deletion failure
should be unified and the exception logged: replace the duplicate string literal
with a single constant (e.g. PET_DELETE_HAS_RECORDS_MESSAGE) used in the
pre-check (medicalRecordRepository.existsByPetId(petId) ->
BusinessRuleException) and in the DataIntegrityViolationException handler thrown
after petRepository.delete/flush; also log the caught exception (use log.warn
with the exception) before throwing the BusinessRuleException so unexpected FK
issues are visible (refer to BusinessRuleException,
medicalRecordRepository.existsByPetId, petRepository.delete,
petRepository.flush, DataIntegrityViolationException, and log.warn).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622859da-81e3-4278-b104-dd2196c8b04d
📒 Files selected for processing (5)
frontend/src/App.jsxfrontend/src/pages/PetDetail.jsxfrontend/src/services/api.jsxsrc/main/java/org/example/vet1177/repository/MedicalRecordRepository.javasrc/main/java/org/example/vet1177/services/PetService.java
Comment on lines
+103
to
+122
| <button | ||
| onClick={handleDeletePet} | ||
| disabled={hasHistory || isDeleting} | ||
| className={`w-full flex items-center justify-center gap-2 py-3 rounded-xl font-black text-[10px] uppercase tracking-widest transition-all italic | ||
| ${hasHistory | ||
| ? 'bg-slate-50 text-slate-300 cursor-not-allowed' | ||
| : 'text-red-500 hover:bg-red-50'}`} | ||
| > | ||
| <Trash2 size={14} /> | ||
| {isDeleting ? 'Tar bort...' : 'Ta bort djurprofil'} | ||
| </button> | ||
|
|
||
| {hasHistory && ( | ||
| <div className="flex items-start gap-3 p-4 bg-slate-50 rounded-2xl border border-slate-100 mt-2"> | ||
| <Info size={16} className="text-slate-400 shrink-0 mt-0.5" /> | ||
| <p className="text-[9px] font-black text-slate-500 uppercase leading-relaxed italic tracking-wide"> | ||
| Profilen kan ej raderas då den är kopplad till medicinsk historik i systemet. | ||
| </p> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Minor a11y: the disabled delete button has no programmatic explanation.
When hasHistory is true the button is disabled but assistive tech only reads "Ta bort djurprofil, disabled"; the explanatory notice at lines 115–122 is visually adjacent but not associated. Consider adding a title and aria-describedby so screen-reader users learn why the action is unavailable.
♻️ Suggested tweak
<button
onClick={handleDeletePet}
disabled={hasHistory || isDeleting}
+ title={hasHistory ? 'Profilen kan ej raderas då den är kopplad till medicinsk historik.' : undefined}
+ aria-describedby={hasHistory ? 'delete-disabled-reason' : undefined}
className={`w-full flex items-center justify-center gap-2 py-3 rounded-xl font-black text-[10px] uppercase tracking-widest transition-all italic
${hasHistory
? 'bg-slate-50 text-slate-300 cursor-not-allowed'
: 'text-red-500 hover:bg-red-50'}`}
>
<Trash2 size={14} />
{isDeleting ? 'Tar bort...' : 'Ta bort djurprofil'}
</button>
{hasHistory && (
- <div className="flex items-start gap-3 p-4 bg-slate-50 rounded-2xl border border-slate-100 mt-2">
+ <div id="delete-disabled-reason" className="flex items-start gap-3 p-4 bg-slate-50 rounded-2xl border border-slate-100 mt-2">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/PetDetail.jsx` around lines 103 - 122, The delete button
is disabled when hasHistory is true but lacks programmatic explanation; update
the button rendered in the block using handleDeletePet / hasHistory / isDeleting
so that when hasHistory is true it includes a descriptive title and an
aria-describedby pointing to the explanatory message element, and give that
explanatory <div> a stable id (e.g., "delete-disabled-reason") so screen readers
announce why the button is disabled; ensure the aria-describedby is only present
when hasHistory is true and removed otherwise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #245
Summary by CodeRabbit
New Features
Style