diff --git a/web/src/domain/assignments.test.ts b/web/src/domain/assignments.test.ts index 85b5477f..bf6c5a78 100644 --- a/web/src/domain/assignments.test.ts +++ b/web/src/domain/assignments.test.ts @@ -42,6 +42,14 @@ import { import { TEST_FAILURE_DETAILS_LEVELS } from "@/types/classroom" import type { SubmissionMode } from "@/types/classroom" +// An untouched row, so a test can assert the edited row keeps its position. +const neighborRow = (slug: string): Assignment => ({ + slug, + name: `Homework ${slug}`, + mode: "individual", + autograder: "default", +}) + const fullSource: Assignment = { slug: "hw1", name: "Homework 1", @@ -499,13 +507,16 @@ describe("editAssignment (preserved-entry integration)", () => { // on the template-less path: ref read, commit read, assignments.json contents // read, then tree/commit/ref writes. classroom.json is absent (404) so the // archive guard reads the classroom as active. - function makeClient(entry: Assignment = existingEntry): { + function makeClient( + entry: Assignment = existingEntry, + ...neighbors: Assignment[] + ): { client: GitHubClient committedContent: () => string } { const assignmentsFile = { schema: "classroom50/assignments/v1", - assignments: [entry], + assignments: [entry, ...neighbors], } const b64 = (s: string) => Buffer.from(s, "utf-8").toString("base64") @@ -635,6 +646,37 @@ describe("editAssignment (preserved-entry integration)", () => { expect(written.assignments[0].name).toBe("Homework 1 (edited)") }) + it("edits the entry in place instead of moving it to the end", async () => { + const { client, committedContent } = makeClient( + existingEntry, + neighborRow("hw2"), + ) + + await editAssignment(client, editInput()) + + const written = JSON.parse(committedContent()) as { + assignments: Assignment[] + } + expect(written.assignments.map((a) => a.slug)).toEqual([SLUG, "hw2"]) + }) + + it("rewrites only the first row when a manifest holds a duplicate slug", async () => { + // A hand-edited manifest can hold two rows for one slug. The edit is built + // from the first (`find`), so the second must survive untouched. + const duplicate: Assignment = { ...existingEntry, name: "Homework 1 (dup)" } + const { client, committedContent } = makeClient(existingEntry, duplicate) + + await editAssignment(client, editInput()) + + const written = JSON.parse(committedContent()) as { + assignments: Assignment[] + } + expect(written.assignments.map((a) => a.name)).toEqual([ + "Homework 1 (edited)", + "Homework 1 (dup)", + ]) + }) + it.each([ ["omitted", undefined, undefined], ["zero", 0, undefined], @@ -4623,6 +4665,7 @@ describe("setAssignmentLock", () => { const assignmentsFile = { schema: "classroom50/assignments/v1", assignments: [ + neighborRow("hw0"), { slug: SLUG, name: "Homework 1", @@ -4631,6 +4674,7 @@ describe("setAssignmentLock", () => { ...(opts.locked ? { locked: true } : {}), ...(template ? { template } : {}), }, + neighborRow("hw2"), ], } const classroomJson: Record = { @@ -4773,6 +4817,19 @@ describe("setAssignmentLock", () => { expect(grants()).toContain("classroom50-cs50") }) + it("flips the flag in place instead of moving the row to the end", async () => { + const { client, committed } = makeLockClient({ locked: false }) + await setAssignmentLock(client, { + org: ORG, + classroom: CLASSROOM, + slug: SLUG, + locked: true, + }) + const written = JSON.parse(committed()!) as { assignments: Assignment[] } + // Matches the CLI's lock, which assigns back into the same index. + expect(written.assignments.map((a) => a.slug)).toEqual(["hw0", SLUG, "hw2"]) + }) + it("makes a public template a UX-gate-only lock (no access change)", async () => { const { client, revokes } = makeLockClient({ locked: false, @@ -4841,6 +4898,7 @@ describe("setAssignmentClosed", () => { const assignmentsFile = { schema: "classroom50/assignments/v1", assignments: [ + neighborRow("hw0"), { slug: SLUG, name: "Homework 1", @@ -4849,6 +4907,7 @@ describe("setAssignmentClosed", () => { template: { owner: ORG, repo: "tmpl", branch: "main" }, ...(opts.closed ? { closed: true } : {}), }, + neighborRow("hw2"), ], } const classroomJson: Record = { @@ -4930,6 +4989,18 @@ describe("setAssignmentClosed", () => { expect(committed()).not.toContain(`"closed"`) }) + it("flips the flag in place instead of moving the row to the end", async () => { + const { client, committed } = makeClosedClient({ closed: false }) + await setAssignmentClosed(client, { + org: ORG, + classroom: CLASSROOM, + slug: SLUG, + closed: true, + }) + const written = JSON.parse(committed()!) as { assignments: Assignment[] } + expect(written.assignments.map((a) => a.slug)).toEqual(["hw0", SLUG, "hw2"]) + }) + it("no-ops when already in the requested state (no commit)", async () => { const { client, committed } = makeClosedClient({ closed: true }) const result = await setAssignmentClosed(client, { diff --git a/web/src/domain/assignments/createEdit.ts b/web/src/domain/assignments/createEdit.ts index 1c67778d..711497d0 100644 --- a/web/src/domain/assignments/createEdit.ts +++ b/web/src/domain/assignments/createEdit.ts @@ -171,6 +171,26 @@ const EDIT_MANAGED_ASSIGNMENT_KEYS = new Set( .map(([key]) => key), ) +// Replace an entry in place so the row keeps its position, matching the CLI's +// UpsertAssignment ("Position preserved on replace"). Drop-and-append would +// reorder `gh teacher assignment list` and turn a one-field edit into a +// whole-row diff in the config repo. +// +// First match only, also like UpsertAssignment: callers build `entry` from +// `find`, so replacing every duplicate-slug row would clobber the second one. +// Callers throw on a missing slug before reaching this, so a miss is a no-op. +function replaceAssignmentEntry( + entries: Assignment[], + slug: string, + entry: Assignment, +): Assignment[] { + const index = entries.findIndex((a) => a.slug === slug) + if (index === -1) return entries + const next = [...entries] + next[index] = entry + return next +} + // Copy forward entry-level keys the edit form doesn't manage (e.g. // `migrated_from`, unknown future keys) onto the rebuilt edit, without // overwriting managed keys. Mirrors the CLI's AssignmentEntry.Extra round-trip. @@ -266,10 +286,11 @@ export async function editAssignment( const nextAssignments = { ...currentAssignments, - assignments: [ - ...currentAssignments.assignments.filter((a) => a.slug !== slug), + assignments: replaceAssignmentEntry( + currentAssignments.assignments, + slug, preservedEntry, - ], + ), } const tree = await createGitTree(client, { @@ -1401,10 +1422,11 @@ export async function setAssignmentLock( const nextAssignments: AssignmentsFile = { ...currentAssignments, - assignments: [ - ...currentAssignments.assignments.filter((a) => a.slug !== slug), + assignments: replaceAssignmentEntry( + currentAssignments.assignments, + slug, updatedEntry, - ], + ), } const tree = await createGitTree(client, { @@ -1542,10 +1564,11 @@ export async function setAssignmentClosed( const nextAssignments: AssignmentsFile = { ...currentAssignments, - assignments: [ - ...currentAssignments.assignments.filter((a) => a.slug !== slug), + assignments: replaceAssignmentEntry( + currentAssignments.assignments, + slug, updatedEntry, - ], + ), } const tree = await createGitTree(client, { diff --git a/web/src/domain/assignments/rename.ts b/web/src/domain/assignments/rename.ts index 81847c5e..7863c49a 100644 --- a/web/src/domain/assignments/rename.ts +++ b/web/src/domain/assignments/rename.ts @@ -283,8 +283,9 @@ async function commitRenameConfig( }) } - // Map in place so the entry keeps its position in the array (unlike the - // lock flip's filter+push, a rename shouldn't reorder the manifest). + // Map in place so the entry keeps its position in the array: a rename + // shouldn't reorder the manifest, same as the edit/lock/close writers in + // createEdit.ts and the CLI's UpsertAssignment. // Lock for the fan-out window: an accept mid-rename would mint a fresh // empty repo at the NEW name and 422 the real repo's rename. const nextAssignments: AssignmentsFile = {