Add destroy for thoughts, resources, journal entries#79
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds destroy support for project thoughts, resources, and journal entries, including UI affordances and controller logic for both HTML and Turbo Stream flows, plus authorization checks and tests.
Changes:
- Extend nested
projectsroutes to supportdestroyfor thoughts, resources, and journal entries. - Add
destroyactions in the corresponding controllers, with Turbo Stream responses updating flash and list partials, and HTML redirects. - Update partials to include hover-revealed delete buttons, and add controller tests covering HTML/turbo deletes and authorization for cross-project deletion.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/controllers/project_thoughts_controller_test.rb |
Adds tests for destroying thoughts in HTML and Turbo Stream modes and for preventing deletes on other users’ projects. |
test/controllers/project_resources_controller_test.rb |
Adds similar destroy and authorization tests for project resources. |
test/controllers/project_journal_entries_controller_test.rb |
Adds destroy and authorization tests for project journal entries. |
config/routes.rb |
Enables :destroy on project-scoped thoughts, resources, and journal_entries resources. |
app/views/projects/thoughts/_thought.html.erb |
Adds a hover-visible delete button for a thought and adjusts layout to accommodate the control. |
app/views/projects/resources/_resource.html.erb |
Adds a hover-visible delete button for a resource with consistent styling. |
app/views/projects/journal_entries/_journal_entry.html.erb |
Adds a hover-visible delete button for a journal entry with consistent styling. |
app/controllers/project_thoughts_controller.rb |
Introduces destroy with project-scoped lookup, Turbo Stream list refresh, and HTML redirect. |
app/controllers/project_resources_controller.rb |
Adds destroy for resources with list refresh and redirect behavior mirroring create. |
app/controllers/project_journal_entries_controller.rb |
Adds destroy for journal entries with Turbo Stream response and HTML redirect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def destroy | ||
| @thought.destroy! | ||
| flash.now[:notice] = "Thought deleted." | ||
|
|
||
| respond_to do |format| | ||
| format.turbo_stream do | ||
| thoughts = @project.thoughts.last_two.with_attached_image | ||
| render turbo_stream: [ | ||
| turbo_stream.replace("flash", partial: "shared/flash"), | ||
| turbo_stream.replace("thoughts_list", partial: "projects/thoughts/list", | ||
| locals: { thoughts: thoughts }) | ||
| ] | ||
| end | ||
| format.html { redirect_to project_path(@project), notice: "Thought deleted." } | ||
| end |
There was a problem hiding this comment.
The destroy action uses destroy! and redirects without specifying an explicit status, which differs from established patterns in this codebase (for example, TodosController#destroy and ProjectMilestonesController#destroy both call destroy without the bang and redirect with status: :see_other). To avoid unhandled ActiveRecord::RecordNotDestroyed exceptions causing 500s and to align with the existing redirect semantics after non-GET requests, consider switching to @thought.destroy and adding status: :see_other to the HTML redirect.
| def destroy | ||
| @resource.destroy! | ||
| flash.now[:notice] = "Resource deleted." | ||
|
|
||
| respond_to do |format| | ||
| format.turbo_stream do | ||
| resources = @project.resources.last_two | ||
| render turbo_stream: [ | ||
| turbo_stream.replace("flash", partial: "shared/flash"), | ||
| turbo_stream.replace("resources_list", partial: "projects/resources/list", | ||
| locals: { resources: resources }) | ||
| ] | ||
| end | ||
| format.html { redirect_to project_path(@project), notice: "Resource deleted." } | ||
| end |
There was a problem hiding this comment.
The destroy action currently calls destroy! and redirects without an explicit status code, which is inconsistent with other controllers like TodosController#destroy and ProjectMilestonesController#destroy that use non-bang destroy and status: :see_other on the redirect. For consistency and to avoid raising ActiveRecord::RecordNotDestroyed errors without handling them, it would be better to use @resource.destroy here and add status: :see_other to the HTML redirect.
| def destroy | ||
| @journal_entry.destroy! | ||
| flash.now[:notice] = "Journal entry deleted." | ||
|
|
||
| respond_to do |format| | ||
| format.turbo_stream do | ||
| journal_entries = @project.journal_entries.last_two.with_attached_image | ||
| render turbo_stream: [ | ||
| turbo_stream.replace("flash", partial: "shared/flash"), | ||
| turbo_stream.replace("journal_entries_list", partial: "projects/journal_entries/list", | ||
| locals: { journal_entries: journal_entries }) | ||
| ] | ||
| end | ||
| format.html { redirect_to project_path(@project), notice: "Journal entry deleted." } | ||
| end |
There was a problem hiding this comment.
In this destroy action you are using destroy! and omitting an explicit redirect status, whereas similar patterns in the app (e.g., TodosController#destroy, ProjectMilestonesController#destroy) use non-bang destroy and status: :see_other on the HTML redirect. To keep behavior consistent and avoid unhandled ActiveRecord::RecordNotDestroyed exceptions bubbling up as 500s, consider changing this to use @journal_entry.destroy and adding status: :see_other to the redirect.
1 participant
No description provided.