Model-Layer Invariants and Tests as Contracts

PR #7681 (model architecture) and PR #7668 (test coverage for CriteriaController). Two patches, one principle: if a rule about your data must always hold, it belongs in the model, not the controller — and the test is what documents that the rule holds.

The deletion bug

Section records had a deletion path that could orphan student rows and leave dangling foreign keys to starter file groups. The original cleanup logic lived in the controller — the request handler was responsible for tearing down associated records before destroying the parent.

This is the wrong place for it. Controllers handle the request lifecycle: parse params, authorize, dispatch, render. The rules that keep your data consistent need to hold whether the deletion comes from a controller, a background job, a console session, a seed script, or a test that calls .destroy directly. Encoding integrity rules in the controller protects exactly one access path. Every other path is unguarded.

ActiveRecord gives you the right vocabulary on the association itself:

class Section < ApplicationRecord
  has_many :section_starter_file_groups, dependent: :destroy
  has_many :students, dependent: :restrict_with_error
end

Two different strategies for two different relationships:

:destroy for starter file groups — these are owned by the section. A starter file group has no meaning outside the section it belongs to. When the section goes, they cascade. Using :destroy (not :delete_all) means any callbacks on SectionStarterFileGroup still fire during teardown, which matters if there are dependent file-system cleanups.

:restrict_with_error for students — students are not owned. They exist independently and might be enrolled in other sections, courses, semesters. If an instructor tries to delete a section with students still in it, the deletion fails with a validation error and the instructor is forced to reassign first. Data loss prevented by construction.

The whole controller cleanup block deleted in the same PR. The model now self-enforces, regardless of who calls destroy.

The full table of options:

The choice between :destroy and :delete_all is the one that bites most often. :delete_all is faster but skips callbacks — and if your child model has a before_destroy that cleans up a filesystem artifact, :delete_all leaves orphaned files. Default to :destroy until performance forces otherwise.

Tests as contracts

The other PR was a test-only patch on CriteriaController — the controller that handles grading rubrics. The controller had decent happy-path coverage but the edge cases were thin: nothing for what happens when results have already been released to students, partial coverage on the authorization paths, gaps in the reordering logic.

The rubric-modification case is the interesting one. Once grading results are released to students, modifying the rubric would silently invalidate existing grades — a student might have been graded out of 10 on a criterion that now says it’s out of 15, and there’s no consistent way to reconcile. The controller checks for released results and returns HTTP 409 Conflict if any modification is attempted. This is an application-level optimistic concurrency check, and it had no test.

describe CriteriaController do
  let(:assignment) { create(:assignment) }
  let(:criterion)  { create(:rubric_criterion, assignment: assignment) }

  describe "#update" do
    context "when results have been released" do
      let!(:released_result) { create(:released_result, assignment: assignment) }

      it "rejects the update with 409 Conflict" do
        patch :update, params: { id: criterion.id, criterion: { max_mark: 10 } }
        expect(response).to have_http_status(:conflict)
      end
    end
  end
end

The test is the contract. It says: “if results have been released, modifications return 409.” A future developer who refactors the controller and accidentally removes the released-results check now has CI failing the moment they push.

Most of the work on this PR wasn’t writing assertions, it was building the factory composition that produced the right preconditions. A released_result factory composes submission, result, and grouping factories with the released_to_students: true flag. Factory composition over fixtures, because factories make the dependencies explicit — every test states the minimal preconditions it needs, and you can read the spec without going to a fixtures.yml to figure out what state the database is in.

Three roles tested on every endpoint: students (denied), TAs (limited access), instructors (full). This is the cheapest possible insurance against authorization regressions, which are the kind of bug that doesn’t surface until someone uses the system in a way you didn’t anticipate.

The connection

Both patches are the same idea applied at different layers. Model invariants encode rules that must always hold, in the place that catches every access path. Tests encode behavioral contracts, in the place that catches every future refactor. Neither one is glamorous on its own. Both compound — every constraint you encode declaratively is one you don’t have to enforce in code review, in PR comments, in your head at 2 AM debugging a production issue.