Week 20: A Pull Request That Cannot See Its Prerequisites

Eight pull requests moved this week, five of them new. The grading pipeline got its LMS leg, group rosters got a manual sync, and
subgroups started with a data layer, a listing scope and a policy. The thing that connects all of them is not a feature. It is one
sentence that showed up on #8003, #8043 and #8048, in slightly different words each time: the thing this code calls is not on
this branch. This post is about the two ways I answered it and which one I would not do again.

The sentence

CodeRabbit on the roster sync:

sync_roster reaches Lti::Membership::SCOPE. The reviewed head defines Lti::AccessToken, but it defines neither Lti::Membership
nor Lti::RosterImport or Lti::RosterDrop. The request can therefore raise NameError and return 500 without syncing.

On the subgroup controller:

On the current PR branch, Group has no subgroups association, and the schema has no parent_group_id. A create request therefore
fails at @group.subgroups.

And on the results page from last week, a human reviewer: the model and table it reads were absent, so an authorized request fails at
the lookup.

The reviewers are right every time, and none of them is wrong to be annoyed. Each branch is cut from master and only contains its own
commits, so a reviewer who checks out the branch sees code that calls something that does not exist. In my head these are links in a
chain. On GitHub each one is a separate pull request that has to make sense by itself.

Answer one: put the prerequisites in the branch

On the 12th I handled the two oldest ones by merging the branches they depend on into them. The editor got the testbench model merged
in, and the results page got the settings and the result model. A single commit message captures it: Merge branch
'feat/grading-result' into feat/grading-results-ui. The branches now run from a clean checkout.

That is the correct answer to a stack that is going to merge in order, and the price is the diff. A pull request that adds a results
page now shows 21 changed files, and most of them belong to other pull requests. I know which lines are mine. A reviewer has to
work it out. I would only call this a fix in the sense that the CI is green.

While I was in the editor I fixed a different finding that had been waiting, from a reviewer who read the JavaScript:

beforeSubmit() has already converted the deadline to iso when this blocks submission. fixing the pin mismatch and retrying
produces an invalid deadline; validate before converting it.

The submit handler rewrote the deadline field into an ISO string and then discovered a problem and refused the submit. The form came
back with the field already rewritten, so the second attempt posted a deadline in the wrong format. The rule is old and I broke it
anyway: validate first, mutate second, so a refusal leaves the form the way the user left it.

The same day, AutogradeJob stopped giving fractional credit when partial_credit is false, which is last week's finding finally
closed, and the results page started drawing one row per test case instead of one row per group. Neither needed a new idea. They needed
the review comment to be read slowly.

Answer two: make the code survive the absence

The roster sync and the subgroup controller got a different treatment, and I am less happy with it.

For the roster action I added a guard in front of the call:

return redirect_to(group_path(@group), alert: t(".not_available")) unless roster_services_loaded?

def roster_services_loaded?
  defined?(Lti::Membership) && defined?(Lti::RosterImport) && defined?(Lti::RosterDrop)
end

For subgroups, the equivalent on the 18th:

before_action :check_subgroups_available

def check_subgroups_available
  return if @group.respond_to?(:subgroups)

  redirect_to group_path(@group), alert: t("subgroups.not_available")
end

Both turn a 500 into a redirect with a message, and both make the branch pass a review that was about the branch. Neither is something
I would write if the prerequisite were on master. After the stack merges, each one is a permanent branch that can never be taken, a
test for a state production will never be in, and a translation key nobody will see. I wrote a feature detection for a feature that is
going to exist, because it was cheaper than answering a reviewer's question by changing the shape of the stack.

The honest accounting is that answer one costs the reviewer's attention and answer two costs the codebase's. A reviewer's attention is
spent once and the dead branch is paid for by everyone who reads that controller later. My rule for next time is that code which only
exists to survive its own pull request being early gets deleted in the pull request that makes it unnecessary, and that I write that
deletion down in the description so someone knows to look for it.

The LMS leg

#8029 is the last link in the grading pipeline: once a submission is graded and the assignment came from an LMS, post the score
back to the instructor's gradebook through the AGS line item. It went up on the 14th, and CodeRabbit found two real problems in it
that I had listed last week as unchecked assumptions.

The first was the rescue that returns nil:

If Lti::Score.publish fails, this rescue returns nil without reporting the failure. The caller cannot distinguish a failed
publish from a skipped push, so a transient LMS failure can leave the gradebook without the score.

I had written nil for both "nothing to push" and "the push failed", so no caller could tell a silent skip from a lost grade. It
returns a boolean now and logs the failure:

Lti::Score.publish(link.lineitem_url, token, user_id: sub, score_given: given, score_maximum: 100)
true
rescue Lti::AccessToken::Error, Lti::Score::Error => e
  Rails.logger.error("Autograder::AgsPush failed for project #{project.id}: #{e.message}")
  false

The second was the single users.lti_sub column:

LTI defines sub as unique within an issuer, not across issuers. If one user launches from two LMS issuers, users.lti_sub can
retain only one subject. Autograder::AgsPush can then send that subject to the other issuer's line item.

This one scared me more than the first, because it is a wrong grade going to the wrong place with no error. A subject identifier is
meaningful only together with the platform that issued it. The column became a separate identity per deployment, and the lookup takes
the deployment from the assignment's resource link:

sub = link && project.author&.lti_sub_for(link.lti_deployment)

A third finding, that a stored line item URL should be rejected unless it is HTTPS before the bearer token is sent to it, is the same
cleartext concern CodeRabbit raised on the runner in September. It is still open on the score service.

Rosters, and a migration that would have locked a table

#8043 adds a Sync Roster button on a group page. It fetches the current course roster from the LMS, adds the members who joined,
drops the ones who left and records when it last succeeded. Until now a roster entered CircuitVerse once, at launch, so a student who
dropped the course stayed in the group. I chose a button over syncing on every launch because a visible button with a visible "last
synced" time is easier to reason about when the LMS is flaky, and an automatic sync would hide failures and slow every launch.

The migration needed work. strong_migrations refused the foreign key on groups as one step, so the commit on the 17th split it into
a two step version that does not hold a long lock on the table. It is the shape I now reach for by default, so I will not belabour it.

Subgroups begin

The subgroup work started on the 16th and it begins with a choice I am glad I made early: a subgroup is just a Group with a
parent_group_id, not a new table. Everything already built for groups, such as members, assignments and the mentor checks, works on a
subgroup for free. The model keeps the rules in one place: one level of nesting, the subgroup takes the parent's mentor, names are
unique within a parent, and creating a subgroup does not send the "new group" email, because sorting a class into teams should not
trigger one email per team.

scope :top_level, -> { where(parent_group_id: nil) }
validates :name, uniqueness: { scope: :parent_group_id, case_sensitive: false }, if: :subgroup?

The consequence of that choice showed up the next day in #8046. A subgroup is a row in groups, so every "list my groups" query
returns it next to the real classes, on the dashboard and in the API. I filter in SQL with top_level at each listing, rather than
in the serializer, so pagination counts stay right and later listings get the behaviour from the scope instead of remembering to
filter. A reviewer found the hole in the inheritance rule:

The callback runs only on creation. If Api::V1::GroupsController#update changes a subgroup's primary_mentor_id, the subgroup no
longer has its parent's primary mentor.

Inheritance at creation is not an invariant. A rule that is supposed to always hold has to be enforced on update too, including when a
parent's mentor changes and its subgroups are left pointing at the old one. The commit on the 17th makes it hold past creation.

Where the project stands

The grading pipeline is complete as code, from test editor to gradebook. Subgroups have a data layer, a listing scope and a policy with
a controller. Two of the answers to "the prerequisite is missing" are in the code, and I would use only one of them again.

Next week: the subgroup UI, API and team project go up, and every review is about who is allowed to see what.