feat(posts): add safe delete flow for custom post types#247
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR enhances the custom post type deletion flow by introducing an explicit modal confirmation dialog and backend dependency validation. The backend now prevents deletion when active ideas reference the custom post type, with corresponding test coverage for both successful and blocked deletion scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend UI
participant Modal
participant Backend as Backend View
participant Database
User->>Frontend: Click Delete button
Frontend->>Modal: Trigger open action
Modal->>Modal: Display modal dialog
User->>Modal: Check acknowledgement + Click Confirm
Modal->>Backend: POST with confirm_delete=yes
Backend->>Backend: Check confirmation flag
alt Confirmation Valid
Backend->>Database: Query active title_suggestions
alt Dependencies Found
Backend->>Backend: Build error message
Backend->>Frontend: Redirect with error
Frontend->>User: Display "Cannot delete - active ideas"
else No Dependencies
Backend->>Database: Delete custom post type
Backend->>Frontend: Redirect to success page
Frontend->>User: Deletion confirmed
end
else Confirmation Missing
Backend->>Frontend: Redirect with error
Frontend->>User: Display "Delete not confirmed"
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR introduces a safe-delete flow for custom post types, replacing the previous browser Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant Django as Django (ProjectCustomPostTypeDeleteView)
participant DB
User->>Browser: Clicks "Delete" button
Browser->>Browser: showModal() → <dialog> opens
User->>Browser: Checks confirmation checkbox & clicks "Delete post type"
Browser->>Django: POST /project/{pk}/post-type/{post_type_pk}/delete/<br/>confirm_delete=yes
Django->>Django: Check confirm_delete == "yes"
alt confirm_delete missing / not "yes"
Django-->>Browser: redirect → error message "Delete not confirmed"
else confirmed
Django->>DB: COUNT title_suggestions WHERE archived=False
alt active_dependency_count > 0
Django-->>Browser: redirect → error message "Cannot delete '…'"
else no active dependencies
Django->>DB: DELETE ProjectCustomPostType
DB-->>Django: SET_NULL on archived idea FKs
Django-->>Browser: redirect → success message "Deleted '…'"
end
end
Last reviewed commit: "feat(posts): add saf..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <script> | ||
| document.querySelectorAll("[data-delete-modal-open]").forEach((button) => { | ||
| button.addEventListener("click", () => { | ||
| const modalId = button.getAttribute("data-delete-modal-open"); | ||
| const modal = document.getElementById(modalId); | ||
| if (modal) { | ||
| modal.showModal(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| document.querySelectorAll("[data-delete-modal-close]").forEach((button) => { | ||
| button.addEventListener("click", () => { | ||
| const modal = button.closest("dialog"); | ||
| if (modal) { | ||
| modal.close(); | ||
| } | ||
| }); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
<script> instead of Stimulus controller
The project's frontend.mdc rule explicitly states: "Prefer Stimulus JS for adding interactivity to Django templates instead of raw script elements" and "New controllers should be created in frontend/src/controllers directory".
This inline <script> block that wires up the modal open/close behavior should instead be a dedicated Stimulus controller (e.g., delete-modal_controller.js) using data-controller, data-action, and target attributes. The current script tag circumvents the established convention for encapsulating JS behaviour.
Rule Used: .cursor/rules/frontend.mdc (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <dialog id="delete-post-type-modal-{{ post_type.id }}" class="p-0 w-full max-w-md rounded-lg border border-gray-200 shadow-xl backdrop:bg-black/50"> | ||
| <div class="p-6 space-y-4"> | ||
| <div> | ||
| <h4 class="text-lg font-semibold text-gray-900">Delete "{{ post_type.name }}"?</h4> | ||
| <p class="mt-2 text-sm text-gray-600"> | ||
| This action cannot be undone. Deletion is blocked if this type is still used by active ideas. | ||
| </p> | ||
| </div> | ||
|
|
||
| <form method="post" action="{% url 'project_custom_post_type_delete' project.id post_type.id %}" class="space-y-4"> | ||
| {% csrf_token %} | ||
| <input type="hidden" name="confirm_delete" value="yes" /> | ||
| <label class="flex gap-2 items-start text-sm text-gray-700"> | ||
| <input type="checkbox" required class="mt-0.5 rounded border-gray-300" /> | ||
| <span>I understand this will permanently delete this custom post type if it is safe to do so.</span> | ||
| </label> | ||
|
|
||
| <div class="flex gap-3 justify-end items-center"> | ||
| <button type="button" class="inline-flex items-center px-3 py-2 text-sm font-medium text-gray-700 bg-white rounded-md border border-gray-300 hover:bg-gray-50" data-delete-modal-close> | ||
| Cancel | ||
| </button> | ||
| <button type="submit" class="inline-flex items-center px-3 py-2 text-sm font-medium text-white bg-red-600 rounded-md border border-red-600 hover:bg-red-700"> | ||
| Delete post type | ||
| </button> | ||
| </div> | ||
| </form> | ||
| </div> | ||
| </dialog> |
There was a problem hiding this comment.
The <dialog> element opened with showModal() does not close when the user clicks on the semi-transparent backdrop outside the dialog box. The native <dialog> element fires a cancel event when Escape is pressed, but there is no built-in backdrop-click-to-close behaviour. Users who click outside expecting the dialog to dismiss will be confused.
Consider adding a click listener on the <dialog> element itself (the backdrop area outside .p-6 triggers the event on the dialog, not on inner content):
modal.addEventListener("click", (event) => {
if (event.target === modal) {
modal.close();
}
});Or, if the intent is to intentionally prevent accidental dismissal for this destructive action, a brief comment would clarify that this is deliberate.
| messages.error(request, "Delete not confirmed. Custom post type was not deleted.") | ||
| return redirect("project_custom_post_types", pk=project.pk) | ||
|
|
||
| active_dependency_count = custom_post_type.title_suggestions.filter(archived=False).count() |
There was a problem hiding this comment.
The guard only prevents deletion while archived=False ideas reference the type. If a post type has only archived ideas, deletion proceeds and those archived ideas will have their custom_post_type FK silently set to NULL (via on_delete=models.SET_NULL). This is likely the intended behaviour given the PR description, but the user-facing modal copy says "Deletion is blocked if this type is still used by active ideas" — which matches. Just worth confirming this silent nullification of archived idea references is intentional and expected downstream (e.g., in analytics or listing views that may previously filter by post type).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary\n- add explicit confirmation flow for custom post type deletion via in-page modal\n- require backend delete confirmation and block deletion when active (non-archived) ideas still reference the type\n- keep clear user messaging for unconfirmed and blocked delete attempts\n- add regression tests for confirmed delete and blocked-delete scenarios\n- update CHANGELOG with the shipped delete flow\n\n## Testing\n- local pytest execution in this runtime is blocked because PostgreSQL host
dbis unavailable outside docker compose\n- CI will run the full suiteSummary by CodeRabbit