Skip to content

(retired) feat(assessment): Attempt base-record split (additive) - #8510

Draft
LWS49 wants to merge 26 commits into
masterfrom
lws49/feat-marketplace-pr7a-attempt-base-record
Draft

(retired) feat(assessment): Attempt base-record split (additive)#8510
LWS49 wants to merge 26 commits into
masterfrom
lws49/feat-marketplace-pr7a-attempt-base-record

Conversation

@LWS49

@LWS49 LWS49 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Note to reviewer

This PR explores the decoupling of Submission and Course, and is currently retired. Revisit if needs change and the need to decouple arose again.

Splits Course::Assessment::Submission into an Attempt base record (keeps the unrenamed course_assessment_submissions table via self.table_name) plus a Submission extension (course_assessment_submission_details), repairs every regression the split touched, and pre-emptively guards base-table reads so no preview row (added in PR7b) can leak into staff-facing counts/stats/reports.

Additive-only - no rename, no data risk. The extension table is new; answers/submission_questions keep submission_id (the base id). Backfill is a gentle 1:1 side-table insert. The unique index (assessment_id, creator_id) stays.

Commits

  • Extension table migration + additive split (Attempt base + Submission extension)
  • Preview-leak guards on base-table reads (statistics counts/grades/times/answers, grade_summary, discussion from_user, get-help reports) — behavior-preserving today (no previews exist yet)
  • Split-regression repairs (data-access surface, authorization rules, grading/ability/live-feedback)

Reviewer guide — submission.rb net changes

The submission.rb diff is large but almost entirely relocation, not deletion: Submission shrank to the course-coupled EXP extension, and the attempt/answers/grading machinery moved to attempt.rb. Nothing below was deleted outright — every removed line either moved to attempt.rb or stayed with a query rewrite.

Moved out to attempt.rb (the base record):

  • Workflow state machine (workflow do … end) + the Workflow / WorkflowEventConcern / AnswersConcern includes
  • Lifecycle callbacks: after_save :auto_grade_submission, after_save :retrieve_codaveri_feedback, after_create :create_force_submission_job
  • Associations: belongs_to :assessment; has_many :submission_questions, :answers (+ the MRQ / text / programming / scribing / forum-post actable answer associations), :question_bundle_assignments, :logs; accepts_nested_attributes_for :answers
  • Validations: validate_unique_submission, validate_autograded_no_partial_answer; validates :submitted_at, :workflow_state, :creator, :updater, :assessment
  • Methods: auto_grade!, auto_feedback!, questions, assigned_questions, current_answers, current_programming_answers, answer_history, user_get_help_message_counts, evaluated_or_graded_answers, unsubmitting?, submission_view_blocked?, create_force_submission_job; privates auto_grade_submission, retrieve_codaveri_feedback

Stayed on Submission (the EXP / course-coupled slice):

  • acts_as_experience_points_record, belongs_to :publisher, validates :last_graded_time
  • validate_consistent_user, validate_awarded_attributes
  • current_points_awarded, self.grade_summary, self.on_dependent_status_change, the alias_method workflow setters
  • calculated :graded_at / :log_count / :grade / :grader_ids — kept, but the subquery now correlates on attempt_id instead of the base id
  • scopes — by_user / by_users rewritten as attempt subqueries; confirmed / pending_for_grading now join the base

New (extension wiring the split introduced):

  • self.table_name = 'course_assessment_submission_details', belongs_to :attempt
  • delegate … to: :attempt — re-exposes the ~30 attempt-level readers/writers so existing submission.<x> call sites keep working unchanged
  • self.record_userstamp = false + creator/updater reflection cleanup (stamping belongs to the base)
  • Workflow-event wrappers finalise! / publish! / unsubmit! / resubmit_programming! + the after_*_hook seams, with the EXP/email tail (assign_zero_experience_points, send_email_after_publishing, publish_delayed_posts, …) relocated here from WorkflowEventConcern
  • assessment_id, ensure_last_graded_time, repoint_attempt_errors_to_base

BEFORE (pre-split)

erDiagram
    COURSE_ASSESSMENT_SUBMISSIONS {
        int id PK
        int assessment_id FK
        string workflow_state
        int creator_id FK
        int updater_id FK
        int publisher_id FK
        datetime published_at
        string session_id
        datetime submitted_at
        datetime last_graded_time
    }
    COURSE_ASSESSMENT_ANSWERS {
        int id PK
        int submission_id FK
    }
    COURSE_ASSESSMENT_SUBMISSION_QUESTIONS {
        int id PK
        int submission_id FK
        int question_id FK
    }
    COURSE_EXPERIENCE_POINTS_RECORDS {
        int id PK
        int course_user_id FK
        int points_awarded
        int draft_points_awarded
        int awarder_id FK
        datetime awarded_at
        int actable_id FK "polymorphic"
        string actable_type "polymorphic"
    }

    COURSE_ASSESSMENT_SUBMISSIONS ||--o{ COURSE_ASSESSMENT_ANSWERS : "has"
    COURSE_ASSESSMENT_SUBMISSIONS ||--o{ COURSE_ASSESSMENT_SUBMISSION_QUESTIONS : "has"
    COURSE_ASSESSMENT_SUBMISSIONS ||--o| COURSE_EXPERIENCE_POINTS_RECORDS : "acts_as_experience_points_record (polymorphic actable)"
Loading

Course::Assessment::Submission is a single model mapped straight to course_assessment_submissions. It directly acts_as_experience_points_record, so the EXP identity columns (course_user_id, points_awarded, draft_points_awarded, awarder_id, awarded_at) live on course_experience_points_records, linked back via a polymorphic actable. There is no course_assessment_submission_details table — it does not exist yet.

AFTER — PR7a (base/extension split)

erDiagram
    COURSE_ASSESSMENT_SUBMISSIONS {
        int id PK
        int assessment_id FK
        string workflow_state
        int creator_id FK
        int updater_id FK
        int publisher_id FK "duplicated, deferred cleanup"
        datetime published_at
        string session_id "duplicated, deferred cleanup"
        datetime submitted_at
        datetime last_graded_time "duplicated, deferred cleanup"
    }
    COURSE_ASSESSMENT_SUBMISSION_DETAILS {
        int id PK
        int attempt_id FK "NEW, UNIQUE"
        int publisher_id FK
        string session_id
        datetime last_graded_time
    }
    COURSE_ASSESSMENT_ANSWERS {
        int id PK
        int submission_id FK "unchanged column name; now points to Attempt base id"
    }
    COURSE_ASSESSMENT_SUBMISSION_QUESTIONS {
        int id PK
        int submission_id FK "unchanged column name; now points to Attempt base id"
        int question_id FK
    }
    COURSE_EXPERIENCE_POINTS_RECORDS {
        int id PK
        int course_user_id FK
        int points_awarded
        int draft_points_awarded
        int awarder_id FK
        datetime awarded_at
        int actable_id FK "polymorphic"
        string actable_type "polymorphic"
    }

    COURSE_ASSESSMENT_SUBMISSIONS ||--o{ COURSE_ASSESSMENT_ANSWERS : "has"
    COURSE_ASSESSMENT_SUBMISSIONS ||--o{ COURSE_ASSESSMENT_SUBMISSION_QUESTIONS : "has"
    COURSE_ASSESSMENT_SUBMISSIONS ||--o| COURSE_ASSESSMENT_SUBMISSION_DETAILS : "0..1 (absent ⇒ preview) — NEW relationship"
    COURSE_ASSESSMENT_SUBMISSION_DETAILS ||--o| COURSE_EXPERIENCE_POINTS_RECORDS : "acts_as_experience_points_record moved here (polymorphic actable)"
Loading

What's new in this diagram: the COURSE_ASSESSMENT_SUBMISSION_DETAILS box (a brand-new table) and the 0..1 (absent ⇒ preview) relationship line connecting it to the base table — that single optional edge is the entire feature. Everything else (answers, submission_questions, their FK columns) is structurally untouched; only what the FK conceptually points to has shifted from "the one and only submission row" to "the Attempt base row, which may or may not have a submission extension."

course_assessment_submissions is now Course::Assessment::Attempt — the base record: workflow_state, answers, grading, and the unique (assessment_id, creator_id) index. course_assessment_submission_details is the new Course::Assessment::Submission extension, has_one-linked via attempt_id (unique, so at most one extension row per base row). The extension is now what acts_as_experience_points_record.

The base table still physically carries publisher_id, session_id, and last_graded_time alongside the new extension's copies of the same names — this is intentional duplication left over from the additive, no-rename migration strategy. A later deferred-cleanup PR drops the base table's copies once all callers read from the extension. This is noted here in prose rather than cluttering the diagram above.

ID identity — how old data stays compatible

The base table was never renamed or re-keyed, so every historical submission row keeps its id. The backfill inserts one extension row per base row with attempt_id = base.id. The result:

Object id it exposes Equals the legacy/historical submission id?
Attempt (base, course_assessment_submissions) attempt.id ✅ same row, same id, never re-keyed
Submission (extension, course_assessment_submission_details) submission.id ❌ brand-new serial, nothing legacy references it
Submission (extension) submission.attempt_id ✅ = the base/historical id

So submission.attempt_id == attempt.id == the pre-split submission's original id. Every child FK (answers.submission_id, submission_questions.submission_id, logs.submission_id, …) still references that base id, unchanged — which is why old data stays valid.

Consequential rule for the whole split: whenever code holds a Submission (extension) object and must match a child's submission_id, use .attempt_id, not .id (e.g. the live_feedback/thread_concern and koditsu submissions_concern fixes in PR7a). .id is the extension's own serial and matches nothing on the children.

LWS49 added 11 commits July 17, 2026 16:03
- add Listing and Adoption models under Course::Assessment::Marketplace
  namespace, with course_assessment_marketplace_ prefix on both tables
- Listing tracks published state and publisher, with a uniqueness
  constraint per assessment and an adoption_count helper
- Adoption links a listing to a destination course and duplicated
  assessment, one adoption per duplicated assessment
- wire has_one :marketplace_listing onto Course::Assessment
- add AssessmentMarketplaceAbilityComponent: admins can publish listings,
  course managers/owners can access, duplicate, and preview published
  listings
- add publish/remove listing endpoints (admin-gated create/destroy)
- expose canPublishToMarketplace + listing state on assessment show DTO
- add Publish/Remove to Marketplace button on the assessment header
- warn in the delete Prompt when a listed assessment is removed
- add MarketplaceAPI client, translations, and controller/FE specs
- add cross-instance listings index (published only, live counts)
- add browse page with title search, adoptions/newest sort, row select
- add sidebar admin entry + /courses/:id/marketplace route
- add "Import Assessments" button on assessments index (from_tab)
- add FE api/operations/types, controller + component specs
- add DuplicationJob: copies listings into a course tab, writes adoption
- add bulk duplicate endpoint enqueuing the job for selected listings
- add DuplicateConfirmation modal with row + bulk triggers, job polling
- add MarketplaceAPI.duplicate and duplicateListings poll operation
- serialize and assert live distinct-course adoption count in index
Add the read-only backend for the marketplace browse flow:

- listings#show serializes a curated, read-only view of a published
  assessment (config + per-question summaries) for the listing preview.
- questions#show serializes a single question's detail, dispatching to
  per-type detail partials (multiple/text/voice/forum/programming/
  rubric/scribing) so each renderer gets exactly the data it needs.
- The listings index gains destination tabs plus preview/duplicate URLs
  so the browse table can link into the flow and target a tab.

Type labels are serialized human-readable (question_type_readable) to
match the real assessment show page, while the demodulized discriminator
is kept for frontend renderer dispatch. The base controller pulls in
AssessmentsHelper so the preview views can reuse display_graded_test_types,
and the sidebar component now uses the :marketplace (storefront) icon.
…port

Extract the assessment/tab/question tree from AssessmentsListing into a
reusable DuplicationAssessmentTree component so both the duplication page
and the marketplace duplicate dialog render an identical tree. The old
DuplicateItemsConfirmation listing is rewired onto it.

Also add the shared table primitives the marketplace index needs:

- renderEmpty flows through TableTemplate -> Body -> MuiTable so a table
  can render a custom empty state when it has no rows.
- hideSelectAll drops the select-all header checkbox while keeping the
  per-row checkboxes.
- Register the storefront icon in COURSE_COMPONENT_ICONS.
Build the read-only browse experience on top of the preview endpoints:

- Marketplace index: single-toolbar table with pagination, empty states,
  hidden select-all, and links into the listing preview / duplicate flow.
- Listing preview page: read-only assessment config, per-question cards
  (type chip, staff-only notes, expandable options) and a Duplicate
  Assessment action.
- Question detail preview: header chip plus a renderer dispatcher with a
  renderer per question type (multiple/text/voice/forum/programming/
  rubric/scribing).
- Duplicate dialog now shows the destination course and the shared
  assessment tree.

Includes the api client, operations, types, translations and locale
strings backing the above.
Thread the origin assessment tab (from_tab) through the whole browse flow (index -> listing -> question preview and back) via withFromTab helpers,
so a duplication always imports into the tab the user started from no matter how they navigate. Add the route data handles that build the
marketplace / listing / question breadcrumbs, preserving from_tab on the crumb links.
Replace the static destination summary with an in-dialog tab picker and
report duplication results honestly:

- Add DestinationTabPicker, a radio tree grouping the current course's
  tabs by category, so the duplicator chooses the destination tab inside
  the dialog instead of it being fixed by the launching `from_tab`.
  The selection seeds from `from_tab` (falling back to the first tab)
  and re-seeds on each reopen, but a parent re-render never resets a
  choice mid-decision.
- Serve `destinationTabs` from the listing show endpoint too, so the
  picker is available when duplicating from the listing detail page,
  not just the marketplace index.
- Restyle the dialog: vertically stacked tabs with larger category/tab
  text, a dense TypeBadge variant, an explicit "Duplicating" heading,
  the ⊘ "arrives unpublished" hint, and explicit cancel/primary colors.
- Report a *completed* duplication (the toast fires from pollJob's
  completion callback, not on submit) and link to where the copy landed
  via the job's redirectUrl; reword the failure copy to plain language.
  Widen the shared toast Toaster type to ReactNode so the toast can
  carry that link (type-only change, no runtime effect).
`marketplace/listings` with no listing id matched no route and 404'd.
Add a redirect so it lands on the same page as `marketplace/`.
Add a "Preview" chip beside the title on the read-only listing detail
page, so it is never mistaken for the real assessment it mirrors.
Specs commit (use_transactional_fixtures is false), so a bare per-second
timestamp collides whenever two rspec processes start within the same second,
tripping unique constraints. Append a random suffix per process.
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr6d-audit branch 3 times, most recently from 5230cdc to 86dac50 Compare July 24, 2026 04:44
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr7a-attempt-base-record branch from 93216c7 to 68546f0 Compare July 24, 2026 04:46
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr6d-audit branch from 86dac50 to 69326c2 Compare July 24, 2026 05:02
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr7a-attempt-base-record branch from 68546f0 to b1b31c1 Compare July 24, 2026 05:03
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr6d-audit branch from 69326c2 to 74232c9 Compare July 24, 2026 05:29
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr7a-attempt-base-record branch 4 times, most recently from fbed966 to f35dd0c Compare July 24, 2026 07:37
LWS49 added 3 commits July 24, 2026 17:20
Gate assessment-marketplace browsing per person rather than per current-course
role. A typed allow-list (user / instance / email-domain / everyone rules)
grants access to baseline-capable users (course manager/owner or instance
instructor/admin anywhere), with individual access blocks as overrides.

- AllowlistRule and AccessBlock models, migrations, and per-type uniqueness
- RuleMatchQuery / RulePreviewQuery / AccessListQuery for matching and audit
- ability component gates :access_marketplace on the allow-list minus blocks
- User baseline predicates and delete-user FK handling for both tables
- System::Admin CRUD, access-list, and block/unblock endpoints
System::Admin page to manage the marketplace allow-list: add/remove typed
rules (specific user, whole instance, or email domain) with a live preview of
who each rule would let in, and an open-to-everyone / restrict toggle.
Add the access audit section to the allow-list page: an audit list of everyone
with effective access (filterable by status and granting rule), block/unblock
controls per user, and the rule-match counts that flag allow-list rules which
currently grant access to nobody.
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr6d-audit branch from 74232c9 to ec36419 Compare July 24, 2026 09:29
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr7a-attempt-base-record branch from f35dd0c to eb015b3 Compare July 24, 2026 09:29
LWS49 added 11 commits July 24, 2026 17:55
Phase 2 preview-leak gate: num_attempted/num_submitted/num_late/latest_submission
read the base course_assessment_submissions directly, so a preview (an Attempt with
no course_assessment_submission_details row) would inflate them. Add an INNER JOIN to
the extension table to restrict to real submissions. No-op on current data (no previews
exist yet); regression test seeds a bare preview attempt and asserts the counts hold.
…istics

Phase 2 preview-leak gate (continued). grade/duration/answer-correctness raw SQL and
the assessment submission_statistics/ancestor/live_feedback list builders read the base
course_assessment_submissions (or Attempt directly), so a preview (Attempt with no
extension row) would leak into staff-facing statistics. Add INNER JOINs to
course_assessment_submission_details (raw SQL) and .joins(:submission) (AR) to restrict
to real submissions. No-op on current data; regression test seeds a bare preview attempt
and asserts submission_statistics reports the student as unstarted (mutation-verified).
…ussion from_user

Phase 2 preview-leak gate (model layer):
- grade_summary raw SQL reads the base directly; add INNER JOIN to the extension so a
  graded preview (Attempt with no extension row) is not summed into gradebook grades.
- SubmissionQuestion.from_user and ProgrammingFileAnnotation.from_user join the Attempt
  base; join through to the extension (submission: :submission) to exclude previews.

Also fixes a Task 2 regression that shipped in ff465f5 and the Phase 1 gate missed
(topic_spec was not in the gate): ProgrammingFileAnnotation.from_user referenced
Course::Assessment::Submission.arel_table[:creator_id], but post-split that arel_table is
the column-less extension table -> PG::UndefinedTable. creator_id lives on Attempt now.
Regression covered by spec/models/course/assessment/submission_spec.rb (grade_summary)
and the now-green spec/models/course/discussion/topic_spec.rb (from_user).
…1 gate

Three sites broke when Submission was split into Attempt + extension but were not in the
12-file acceptance gate, so they shipped red in ff465f5:
- ProgrammingFileAnnotation.from_user referenced Submission.arel_table[:creator_id] (now
  the column-less extension table) -> PG::UndefinedTable (fixed in prior commit).
- answer/comment_notifier/annotated email called submission.course_user on the Attempt
  base; route through attempt.submission (the extension) like its replied sibling.
- ai_generated_post_service_spec called answer.submission.course_user (Attempt); the
  service already uses .submission.submission.course_user — update the spec to match.
assessment_ability.rb was not touched by the split (ff465f5) but its CanCan hash
conditions navigate associations that moved: Course::Assessment::Submission (extension)
no longer has assessment/creator_id/workflow_state (now on the Attempt base). Rules that
did 'Submission, assessment:/creator_id:' raised CanCan::WrongAssociationName, and the
attempting-hash (experience_points_record) applied via Answer.submission (now Attempt)
raised NoMethodError. Route Submission conditions through attempt:, and express the
attempting-owned check against the Attempt (workflow_state + creator_id, the owner per
validate_consistent_user). assessment_ability_spec: 75 examples, 0 failures (was 11 red).
Another gate-missed Task 2 regression (spec not in the 12-file acceptance gate).
More gate-missed Task 2 regressions (download/statistics/gradebook paths). The split left
the extension Submission unable to serve two relation-level access patterns its callers use:
- calculated attributes: register grade/grader_ids/log_count/graded_at on Submission
  (correlated on attempt_id) so @assessment.submissions.calculated(...) works — was raising
  'undefined method call for nil' in submissions_controller#load_submissions and the
  statistics/csv/zip download services.
- eager loading: Submission has no :answers/:assessment AR association (those moved to
  Attempt), so includes(:answers)/(:assessment) raised AssociationNotFound; route them
  through includes(attempt: ...).
Verified green: zip/csv/statistics/ssid download services + jobs, submissions_controller.
Final batch of gate-missed Task 2 regressions:
- base_auto_grading_job: update_exp?/update_exp received answer.submission (now the
  Attempt), whose awarder/awarded_at/points_awarded live on the extension -> the guard was
  always false and re-grade never recomputed EXP (points stale). Route through the extension.
- programming_ability: :create/:destroy_programming_files checked can?(:update, attempt);
  :update rules are on the extension Submission -> check answer.submission.submission.
- live_feedback ThreadConcern: SubmissionQuestion.where(submission_id: @submission) coerced
  the extension to its own id; match @submission.attempt_id (spec builds the SQ on the attempt).
- skills_mastery_preload: plucked Submission#id but belonging_to_submissions matches the
  attempt id -> pluck(:attempt_id).
- auto_grading_service/job specs: stub auto_grade_submission on the attempt, update_column
  :submitted_at on the attempt, reload the separate extension instance before points assertions.
…n questions)

Final Phase 2 preview-leak gate items:
- system-admin and instance-admin get-help usage reports join course_assessment_submissions
  directly; add INNER JOIN course_assessment_submission_details so preview attempts' live
  feedback never inflates get-help analytics.
- submission_questions#all_answers fetched via @assessment.attempts.find(...).submission and
  dereferenced it unguarded; a preview attempt yields a nil submission (previously only the
  incidental authorize!(:read, nil) blocked it). Raise RecordNotFound explicitly; regression
  test covers it.
…wer/SubmissionQuestion#attempt alias

The extension accessor chain read as answer.submission.submission because Answer#submission
returns the Attempt base (FK column is misleadingly submission_id). Add a reader-only #attempt
alias on Answer and SubmissionQuestion and rewrite the 8 chain sites to answer.attempt.submission.
Attempt#submission (the extension has_one) is unchanged. Behavior-preserving.

Also extract the identical submission= writer that coerces a Submission (extension) to its
Attempt (base) — shared verbatim by Answer, SubmissionQuestion, and QuestionBundleAssignment —
into the new Course::Assessment::CoercesSubmissionToAttempt concern. Behavior-preserving.

Also correct the belongs_to :submission comment in SubmissionQuestion (it wrongly said the
column was "renamed from submission_id").
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr7a-attempt-base-record branch from eb015b3 to e8b9992 Compare July 24, 2026 09:56
@LWS49 LWS49 changed the title PR7a — feat(assessment): Attempt base-record split (Phase 1, additive) feat(assessment): Attempt base-record split (additive) Jul 31, 2026
@LWS49 LWS49 changed the title feat(assessment): Attempt base-record split (additive) (retired) feat(assessment): Attempt base-record split (additive) Jul 31, 2026
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr6d-audit branch 5 times, most recently from ffb97b6 to d75b255 Compare August 1, 2026 06:27
Base automatically changed from lws49/feat-marketplace-pr6d-audit to lws49/feat-marketplace-pr1-foundation August 1, 2026 09:35
@LWS49
LWS49 force-pushed the lws49/feat-marketplace-pr1-foundation branch 2 times, most recently from bbd43f0 to 6eac0c4 Compare August 2, 2026 16:44
Base automatically changed from lws49/feat-marketplace-pr1-foundation to master August 2, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant