Make Lesson Deletions Hard Deletions by fspeirs · Pull Request #674 · RaspberryPiFoundation/editor-api

fspeirs · 2026-02-09T10:45:09Z

This PR changes the behaviour of LessonsController#destroy from soft-delete (aka "archived") to hard delete.

During the development of the front end features for this issue, in RaspberryPiFoundation/editor-standalone#720, @zetter-rpf asked whether we should be doing soft- or hard-delete. I had initially built the front-end UI on the basis that the existing back-end implementation existed for a reason.

@kylec-rpf after discussion with @zetter-rpf said that we should implement this as a hard-delete feature.

Status

Related to RaspberryPiFoundation/experience-cs#1122 and RaspberryPiFoundation/editor-standalone#720

Points for consideration:

This PR changes Lesson's relation with Project from :nullify to :destroy:

Lesson.rb: has_one :project, dependent: :nullify becomes has_one :project, dependent: :destroy

This means that the Project associated with the Lesson will be destroyed when the lesson is destroyed. This seems to me the right thing to do, but I am open to other interpretations of the logic in this area.

What's changed?

Rewrote LessonsController#destroy to destroy the lesson.
Retained the Lessons.unfiltered scope as the default scope, just in case there are soft-deleted projects in the database.
Retained the archived_at column on Lessons for similar reasons.
Updated and renamed tests to match the new approach.

Steps to perform after deploying to production

None should be required, unless we want to delete all the soft-deleted projects from the database at some point in the future.

Merge and deploy RaspberryPiFoundation/editor-standalone#720

This commit changes the behaviour of LessonsController#destroy from an archive (soft-delete) operation to a hard destroy operation. Due to the possibility of having existing lessons in the database that were soft-deleted, we retain the `Lesson.unarchived` scope and only return Lessons where `archived_at` is null. We also remove the Archive and Unarchive operations, since these are no longer needed.

zetter-rpf · 2026-02-09T11:29:47Z

app/models/lesson.rb

+  has_one :project, dependent: :destroy
  accepts_nested_attributes_for :project

  before_validation :assign_school_from_school_class


If there's nothing using the archived_at column now, it would be good to add it to the ignored column list (docs)

As well as signifying to us that the column isn't used anymore, this will mean that active record will stop including the column in generated SQL, making it easier for us to delete later if we wanted to.

Copilot

Pull request overview

This PR updates the Lessons REST API and model so that deleting a Lesson performs a hard delete (and cascades to its associated Project), removing the prior “archive/unarchive” soft-delete behavior.

Changes:

Switch Lesson from non-destroyable/archivable to destroyable, and cascade-delete its associated Project.
Simplify LessonsController#destroy to delete the record (removing undo/unarchive behavior) and adjust lesson listing to always exclude previously-archived lessons.
Remove archive/unarchive concept operations and related specs; update request/model specs to assert hard-deletion behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`app/controllers/api/lessons_controller.rb`	Replace archive/unarchive destroy logic with hard delete; adjust lesson filtering to always use `unarchived`.
`app/models/lesson.rb`	Change `project` association to `dependent: :destroy`; remove archive-related callbacks/methods/scopes.
`app/views/api/lessons/_lesson.json.jbuilder`	Stop including `archived_at` in lesson JSON output.
`app/dashboards/lesson_dashboard.rb`	Remove `archived_at` from Administrate dashboard fields.
`spec/models/lesson_spec.rb`	Update model specs to assert lessons are destroyable and cascade-delete the project; remove archive-related specs.
`spec/features/lesson/listing_lessons_spec.rb`	Update listing behavior expectations around previously archived lessons; remove include-archived behavior tests.
`spec/features/lesson/destroying_a_lesson_spec.rb`	Update request specs to assert hard deletion of lesson and associated project.
`lib/concepts/lesson/operations/archive.rb`	Removed (no longer archiving).
`lib/concepts/lesson/operations/unarchive.rb`	Removed (no longer unarchiving).
`spec/concepts/lesson/archive_spec.rb`	Removed (archive operation no longer exists).
`spec/concepts/lesson/unarchive_spec.rb`	Removed (unarchive operation no longer exists).

Copilot · 2026-02-09T11:29:50Z

app/controllers/api/lessons_controller.rb

+    rescue StandardError => e
+      Sentry.capture_exception(e)
+      render json: { error: "Error destroying lesson: #{e}" }, status: :unprocessable_entity


ApiController already has rescue_from StandardError that reports to Sentry and renders a 500 with a consistent { error: "ExceptionClass: message" } shape. This per-action rescue StandardError changes failures during destroy into a 422, double-reports to Sentry, and can mask real server errors (e.g. NoMethodError, FK violations) as client errors. Prefer removing this rescue entirely, or rescuing only specific expected exceptions (e.g. ActiveRecord::RecordNotDestroyed) and letting everything else bubble up to the global handler.

Suggested change

rescue StandardError => e

Sentry.capture_exception(e)

render json: { error: "Error destroying lesson: #{e}" }, status: :unprocessable_entity

Mostly agree with Copilot

Was there a specific kind of error you were worried about (such as ActiveRecord::RecordNotDestroyed)- if you are, I would make this more specific.

And then would this error be expected or not? I.e. is there validation on the records that would prevent them from being destroyed?

If it's expected, then there's no need to re-raise. If it's unexpected then maybe we don't need this case at all as if it happens the error will bubble up and we'll be notified about it.

Copilot · 2026-02-09T11:29:50Z

app/controllers/api/lessons_controller.rb

    def filtered_lessons_scope
-      archive_scope = params[:include_archived] == 'true' ? Lesson : Lesson.unarchived
-      scope = params[:school_class_id] ? archive_scope.where(school_class_id: params[:school_class_id]) : archive_scope
+      scope = params[:school_class_id] ? Lesson.unarchived.where(school_class_id: params[:school_class_id]) : Lesson.unarchived
      scope = scope.joins(:project).where(projects: { identifier: params[:project_identifier] }) if params[:project_identifier].present?


PR description mentions retaining an “unfiltered” scope/default to handle soft-deleted lessons, but the controller now hard-codes Lesson.unarchived and removes the previous include_archived=true behavior entirely. Either update the PR description to match the behavior, or reintroduce an explicit unfiltered/include-archived path if clients still rely on it.

zetter-rpf

Thanks for taking the time to tidy up archiving, it's great to be able to remove something that isn't being used.

I've left a couple of comments/suggestions

fspeirs added 2 commits February 9, 2026 10:29

Rename the 'Archiving a Lesson' spec.

957e61e

cla-bot bot added the cla-signed label Feb 9, 2026

Update specs for hard-delete of lessons.

d4170e8

fspeirs force-pushed the fs-make-project-deletions-hard-deletions branch from 42330f6 to d4170e8 Compare February 9, 2026 10:59

fspeirs marked this pull request as ready for review February 9, 2026 11:22

Copilot AI review requested due to automatic review settings February 9, 2026 11:22

Copilot started reviewing on behalf of fspeirs February 9, 2026 11:22 View session

zetter-rpf reviewed Feb 9, 2026

View reviewed changes

Copilot AI reviewed Feb 9, 2026

View reviewed changes

zetter-rpf requested changes Feb 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Lesson Deletions Hard Deletions#674

Make Lesson Deletions Hard Deletions#674
fspeirs wants to merge 3 commits intomainfrom
fs-make-project-deletions-hard-deletions

fspeirs commented Feb 9, 2026 •

edited

Loading

Uh oh!

zetter-rpf Feb 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

zetter-rpf Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

zetter-rpf left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	rescue StandardError => e
	Sentry.capture_exception(e)
	render json: { error: "Error destroying lesson: #{e}" }, status: :unprocessable_entity

Conversation

fspeirs commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

Points for consideration:

What's changed?

Steps to perform after deploying to production

Uh oh!

zetter-rpf Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

zetter-rpf Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

zetter-rpf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fspeirs commented Feb 9, 2026 •

edited

Loading

zetter-rpf Feb 9, 2026 •

edited

Loading