From f31ba9e339e97da1e343302a072d26ab6261012b Mon Sep 17 00:00:00 2001 From: Cory Pride Date: Fri, 6 Nov 2026 20:28:09 -0600 Subject: [PATCH] feat: move Learning Record out of core (EN-208) Learning Record runs as its own service (unlocked-learning-record), so core drops the module: modules/learningrecord, its registry entry, its depguard rules, and its route tests and 19 golden files, which moved to the new repo unchanged. The feature-flag integration test that used its route goes too; the module router tests still cover the feature gate. No other module asked it anything, so nothing else in core changes. Dev: make dev builds it from GitHub (LEARNING_RECORD_REF, or your checkout in LEARNING_RECORD_DIR) as learning-record (port 8085; Programs has 8084) and learning-record-migrate. The dev proxy sends /api/learning-record there, and make migrate and migrate-fresh run its migrate and migrate-reset. --- .env.example | 4 + Makefile | 19 +- README.md | 6 + backend/.golangci.yml | 12 +- backend/src/modules/learningrecord/README.md | 89 -------- backend/src/modules/learningrecord/api/api.go | 5 - .../src/modules/learningrecord/handlers.go | 192 ---------------- backend/src/modules/learningrecord/http.go | 197 ----------------- .../src/modules/learningrecord/http_test.go | 42 ---- ...00001_move_into_learning_record_schema.sql | 66 ------ .../learningrecord/migrations/README.md | 10 - backend/src/modules/learningrecord/models.go | 131 ----------- backend/src/modules/learningrecord/module.go | 50 ----- backend/src/modules/learningrecord/store.go | 189 ---------------- backend/src/modules/registry/registry.go | 9 +- backend/src/modules/registry/registry_test.go | 10 +- .../facility_feature_flags_test.go | 34 --- .../learning_record_routes_test.go | 206 ------------------ .../learning_record/account_history.golden | 26 --- .../learning_record/draft_delete.golden | 4 - .../draft_delete_missing_client_id.golden | 4 - .../draft_get_after_delete.golden | 4 - .../draft_get_after_seed.golden | 37 ---- .../learning_record/draft_get_none.golden | 4 - .../learning_record/draft_upsert.golden | 4 - .../entries_after_creates.golden | 73 ------- .../entries_after_delete.golden | 39 ---- .../learning_record/entries_empty.golden | 4 - .../learning_record/entry_create.golden | 37 ---- .../entry_create_bad_facility_id.golden | 4 - .../entry_create_both_facility_fields.golden | 4 - .../entry_create_facility_other.golden | 37 ---- ...ntry_create_facility_other_too_long.golden | 4 - .../learning_record/entry_delete.golden | 4 - .../learning_record/entry_update.golden | 37 ---- .../entry_update_bad_id.golden | 4 - .../learning_record/facilities.golden | 17 -- config/dev.nginx.conf | 10 + docker-compose.yml | 45 ++++ 39 files changed, 87 insertions(+), 1586 deletions(-) delete mode 100644 backend/src/modules/learningrecord/README.md delete mode 100644 backend/src/modules/learningrecord/api/api.go delete mode 100644 backend/src/modules/learningrecord/handlers.go delete mode 100644 backend/src/modules/learningrecord/http.go delete mode 100644 backend/src/modules/learningrecord/http_test.go delete mode 100644 backend/src/modules/learningrecord/migrations/00001_move_into_learning_record_schema.sql delete mode 100644 backend/src/modules/learningrecord/migrations/README.md delete mode 100644 backend/src/modules/learningrecord/models.go delete mode 100644 backend/src/modules/learningrecord/module.go delete mode 100644 backend/src/modules/learningrecord/store.go delete mode 100644 backend/tests/integration/learning_record_routes_test.go delete mode 100644 backend/tests/integration/testdata/learning_record/account_history.golden delete mode 100644 backend/tests/integration/testdata/learning_record/draft_delete.golden delete mode 100644 backend/tests/integration/testdata/learning_record/draft_delete_missing_client_id.golden delete mode 100644 backend/tests/integration/testdata/learning_record/draft_get_after_delete.golden delete mode 100644 backend/tests/integration/testdata/learning_record/draft_get_after_seed.golden delete mode 100644 backend/tests/integration/testdata/learning_record/draft_get_none.golden delete mode 100644 backend/tests/integration/testdata/learning_record/draft_upsert.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entries_after_creates.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entries_after_delete.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entries_empty.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_create.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_create_bad_facility_id.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_create_both_facility_fields.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_create_facility_other.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_create_facility_other_too_long.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_delete.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_update.golden delete mode 100644 backend/tests/integration/testdata/learning_record/entry_update_bad_id.golden delete mode 100644 backend/tests/integration/testdata/learning_record/facilities.golden diff --git a/.env.example b/.env.example index 6809423ae..d6fe2432c 100644 --- a/.env.example +++ b/.env.example @@ -67,6 +67,10 @@ VITE_STATE= # with PROGRAMS_DIR and PROGRAMS_REF. # PROGRAMS_DIR=../unlocked-programs # PROGRAMS_REF=main +# Learning Record (github.com/UnlockedLabs/unlocked-learning-record) is built the +# same way, with LEARNING_RECORD_DIR and LEARNING_RECORD_REF. +# LEARNING_RECORD_DIR=../unlocked-learning-record +# LEARNING_RECORD_REF=main # Path to the AI tutor checkout (github.com/UnlockedLabs/unlocked-hiset-ai), # relative to this repo or absolute. Defaults to ../ai/unlocked-hiset-ai. diff --git a/Makefile b/Makefile index 05005b4b5..30b06adf6 100644 --- a/Makefile +++ b/Makefile @@ -29,6 +29,10 @@ PROVIDER_PLATFORMS_RUN=docker compose run --rm --no-deps providerplatforms-migra # GitHub, or your checkout in PROGRAMS_DIR. PROGRAMS_REPO=https://github.com/UnlockedLabs/unlocked-programs.git PROGRAMS_RUN=docker compose run --rm --no-deps programs-migrate +# Learning Record runs as its own service (EN-208), built the same way: +# LEARNING_RECORD_REF from GitHub, or your checkout in LEARNING_RECORD_DIR. +LEARNING_RECORD_REPO=https://github.com/UnlockedLabs/unlocked-learning-record.git +LEARNING_RECORD_RUN=docker compose run --rm --no-deps learning-record-migrate # Canvas admin created on first `make canvas`. Override in the environment if # you want different credentials. These are also what you log into Canvas with # at localhost:3001 to mint the API token. @@ -84,6 +88,7 @@ define run_dev_compose build_source OPEN_CONTENT "Open Content" $(OPEN_CONTENT_REPO) || exit 1; \ build_source PROVIDER_PLATFORMS "Provider Platforms" $(PROVIDER_PLATFORMS_REPO) || exit 1; \ build_source PROGRAMS "Programs" $(PROGRAMS_REPO) || exit 1; \ + build_source LEARNING_RECORD "Learning Record" $(LEARNING_RECORD_REPO) || exit 1; \ AI_DIR=$${TUTOR_DIR:-$$(sed -n 's/^TUTOR_DIR=//p' .env | tail -1)}; \ AI_DIR=$${AI_DIR:-$(DEFAULT_AI_DIR)}; \ TUTOR_READY=0; TUTOR_WHY=; \ @@ -248,25 +253,29 @@ install-dep: ascii_art fi docker compose down && docker volume rm -f unlockedv2_node_modules && cd frontend && yarn add $(NAME) && cd .. $(call run_dev_compose,) -# Open Content's, Provider Platforms' and Programs' migrations roll back first, -# since they move tables that core's reset drops, and run again after it. Their -# services restart because their connections switch to the role the reset -# recreates, and Open Content's worker seeds the fresh database when it starts. +# Open Content's, Provider Platforms', Programs' and Learning Record's +# migrations roll back first, since they move tables that core's reset drops, and +# run again after it. Their services restart because their connections switch to +# the role the reset recreates, and Open Content's worker seeds the fresh +# database when it starts. migrate-fresh: ascii_art $(OPEN_CONTENT_RUN) migrate-reset $(PROVIDER_PLATFORMS_RUN) migrate-reset $(PROGRAMS_RUN) migrate-reset + $(LEARNING_RECORD_RUN) migrate-reset go run -tags dev $(MIGRATE_MAIN) --fresh $(OPEN_CONTENT_RUN) migrate $(PROVIDER_PLATFORMS_RUN) migrate $(PROGRAMS_RUN) migrate - docker compose restart open-content open-content-worker providerplatforms providerplatforms-worker programs programs-worker + $(LEARNING_RECORD_RUN) migrate + docker compose restart open-content open-content-worker providerplatforms providerplatforms-worker programs programs-worker learning-record migrate: ascii_art go run $(MIGRATE_MAIN) $(OPEN_CONTENT_RUN) migrate $(PROVIDER_PLATFORMS_RUN) migrate $(PROGRAMS_RUN) migrate + $(LEARNING_RECORD_RUN) migrate kolibri: ascii_art $(call run_dev_compose,-f $(DOCKER_COMPOSE) -f $(KOLIBRI_COMPOSE)) diff --git a/README.md b/README.md index 423cdb547..5165c676d 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,12 @@ pages are still in core's frontend; the rev_proxy sends their `/api` paths to it questions with `PROGRAMS_TOKEN`. If you run the server outside compose, `.env.example` points `PROGRAMS_URL` at its port, 8084. +**Learning Record** ([unlocked-learning-record](https://github.com/UnlockedLabs/unlocked-learning-record)) +runs as its own service the same way: `make dev` builds its `main` branch from GitHub, or your checkout when +`LEARNING_RECORD_DIR` is set (such as `../unlocked-learning-record`), or another branch with +`LEARNING_RECORD_REF`. The rev_proxy sends `/api/learning-record` to it, and it asks core who the +user is and whether `learning_record` is on. It listens on port 8085. + **NOTE:** you _must_ be sure to use `127.0.0.1` in place of `localhost` in your browser, as the cookies required for authentication are not shared between the two, and this can cause bad states in the browser that will prevent successful login/auth flow. diff --git a/backend/.golangci.yml b/backend/.golangci.yml index 0c5c67e13..bd3f999ec 100644 --- a/backend/.golangci.yml +++ b/backend/.golangci.yml @@ -2,7 +2,7 @@ version: "2" # golangci-lint's standard linters (what CI ran before this file existed) plus # depguard for the feature-module boundaries (EN-190). Open Content, Provider -# Platforms and Programs aren't here: they are their own services. +# Platforms, Programs and Learning Record aren't here: they are their own services. linters: default: standard enable: @@ -29,7 +29,6 @@ linters: allow: - github.com/UnlockedLabs/unlocked-core/backend/src/modules/module - github.com/UnlockedLabs/unlocked-core/backend/src/modules/registry - - github.com/UnlockedLabs/unlocked-core/backend/src/modules/learningrecord/api # The seam imports nothing from core. depguard only sees each file's # own imports, so a core type handed to modules through the seam # (a *database.DB, handlers.Claims) would slip past the module rules. @@ -41,12 +40,3 @@ linters: desc: the module seam must not expose core types to feature modules (EN-188) allow: - github.com/UnlockedLabs/unlocked-core/backend/src/modules/module - learningrecord: - list-mode: lax - files: ["**/src/modules/learningrecord/**"] - deny: - - pkg: github.com/UnlockedLabs/unlocked-core/backend - desc: a feature module reaches core only through src/modules/module and unlocked-contracts, and other modules only through their api package (EN-186, EN-188) - allow: - - github.com/UnlockedLabs/unlocked-core/backend/src/modules/module - - github.com/UnlockedLabs/unlocked-core/backend/src/modules/learningrecord diff --git a/backend/src/modules/learningrecord/README.md b/backend/src/modules/learningrecord/README.md deleted file mode 100644 index 818daaafc..000000000 --- a/backend/src/modules/learningrecord/README.md +++ /dev/null @@ -1,89 +0,0 @@ -# Learning Record module - -The resident achievement journal: entries a resident records about a program -or activity, and the in-progress draft of one. It moved here from core in -EN-195. - -## Boundaries - -- **It imports nothing from core.** It talks to core only through - `modules/module` (the seam) and `unlocked-contracts`. depguard in - `backend/.golangci.yml` enforces this. -- **It owns no data but its own table** (`learning_record_entries`, in the - `learning_record` schema; see Database). An - entry's `program_name` is free text, not a foreign key into Programs, so it - has nothing to ask Programs today. Provider Platforms (EN-206) and Programs - (EN-207) are their own services, so they aren't modules here. -- **It reads and writes some of core's tables:** it reads `facilities` for the - achievement-location picker (the `facility` type in `models.go`), and adds a - `user_account_history` row when a resident deletes an entry, in the same - transaction. It touches no other core table; the caller's identity comes from - the request claims, not from `users`. Its role can do only those two things - with core's tables (see Database). -- **Core still owns** the `learning_record` feature flag - (`models.LearningRecordAccess`). The `learning_record_deleted` audit action - is this module's own private constant (`LearningRecordDeleted` in - `models.go`), not an import of core's -- depguard forbids that import, so - it's duplicated like `UserAccountHistory` itself. - -## Database - -- **Its table lives in the `learning_record` schema** (EN-286). Its first - migration moved `learning_record_entries` there from `public`; its rollback - moves it back. Core's migrations 00069 and 00073 still create the table in - `public` on a new database, and this module's migrations run after core's. -- **Its connection runs as the `learning_record` role**, which can read and - write only this schema, read core's `facilities`, and insert into core's - `user_account_history` (plus `USAGE` on its id sequence). Postgres refuses - anything else. -- **The role can't log in.** `Schema()` in `module.go` names the schema, and - core opens a pool for this module as the app's usual database user - (`database.OpenModuleDBs`). Each connection switches with - `SET ROLE learning_record` and `search_path = learning_record, public`, so - queries stay unqualified and no site needs a new password. -- **Later migrations must create tables as `learning_record.`.** Module - migrations run on core's connection, so an unqualified table lands in - `public`, where the role can't use it. Tables created here get the role's - grants automatically. -- **On SQLite** (the tests) there are no schemas or roles, so Learning Record - uses core's connection. - -## Files - -| File | What it holds | -|---|---| -| `module.go` | Registers the routes, lists the test table | -| `handlers.go` | The routes, the 8 handlers, and location validation | -| `store.go` | The database queries the routes use | -| `api/` | The answers to core's questions (none yet, EN-186) | -| `models.go` | `LearningRecordEntry` and the local `facility`/audit-history types | -| `http.go` | Responses, errors and request helpers, matching core's JSON | -| `migrations/` | This module's own migrations. The first moves its table into the `learning_record` schema and creates its role | - -## Tests - -- `backend/tests/integration/learning_record_routes_test.go` pins every - route's status, JSON, access level and feature flag against the saved - responses in `testdata/learning_record/`. After an intended change, - regenerate them from `backend/` with: - - ```bash - go test ./tests/integration -run 'TestLearningRecord' -update - ``` - - The changed JSON then shows in the PR for review. -- `PUT /api/learning-record/draft` (the upsert) is pinned as a 500: its - `ON CONFLICT` target doesn't match migration 00069's partial unique index - on `(user_id, client_id) WHERE deleted_at IS NULL`, and Postgres won't - infer a partial index for a target that doesn't repeat its predicate. - Confirmed against a live Postgres container, not just this SQLite - harness — a pre-existing production bug, not introduced by the move, and - not fixed here (out of scope for a move). The fix is - `clause.OnConflict{..., TargetWhere: clause.Where{Exprs: []clause.Expression{ - clause.Expr{SQL: "deleted_at IS NULL"}}}}` in `store.go`'s `UpsertDraft` -- - `TargetWhere` repeats the index's predicate on the conflict target itself; - `Where` instead conditions the `DO UPDATE`, which doesn't match the index - either (confirmed against a live Postgres container: it fails a different - way, `42702` ambiguous column, not `42P10`). Also swap `gorm.Expr("NOW()")` - for a portable `CURRENT_TIMESTAMP` (needed for the SQLite test, not for - Postgres) — a follow-up ticket. diff --git a/backend/src/modules/learningrecord/api/api.go b/backend/src/modules/learningrecord/api/api.go deleted file mode 100644 index 7d7054f42..000000000 --- a/backend/src/modules/learningrecord/api/api.go +++ /dev/null @@ -1,5 +0,0 @@ -// Package api is the only part of learningrecord that other feature modules may -// import: the questions Learning Record answers for them. -// -// No other module asks Learning Record anything yet (EN-186). -package api diff --git a/backend/src/modules/learningrecord/handlers.go b/backend/src/modules/learningrecord/handlers.go deleted file mode 100644 index 850b30c41..000000000 --- a/backend/src/modules/learningrecord/handlers.go +++ /dev/null @@ -1,192 +0,0 @@ -package learningrecord - -import ( - "encoding/json" - "fmt" - "net/http" - "strconv" - "strings" - "unicode/utf8" - - "github.com/UnlockedLabs/unlocked-contracts/features" - "github.com/UnlockedLabs/unlocked-core/backend/src/modules/module" -) - -func (srv *server) routes() []module.Route { - lr := []features.Access{features.LearningRecord} - return []module.Route{ - {Pattern: "GET /api/learning-record/facilities", Handler: handle(srv.handleIndexFacilities), Access: module.AnyUser, Features: lr}, - {Pattern: "GET /api/learning-record/entries", Handler: handle(srv.handleIndexEntries), Access: module.AnyUser, Features: lr}, - {Pattern: "POST /api/learning-record/entries", Handler: handle(srv.handleCreateEntry), Access: module.AnyUser, Features: lr}, - {Pattern: "PUT /api/learning-record/entries/{id}", Handler: handle(srv.handleUpdateEntry), Access: module.AnyUser, Features: lr}, - {Pattern: "DELETE /api/learning-record/entries/{id}", Handler: handle(srv.handleDeleteEntry), Access: module.AnyUser, Features: lr}, - {Pattern: "GET /api/learning-record/draft", Handler: handle(srv.handleGetDraft), Access: module.AnyUser, Features: lr}, - {Pattern: "PUT /api/learning-record/draft", Handler: handle(srv.handleUpsertDraft), Access: module.AnyUser, Features: lr}, - {Pattern: "DELETE /api/learning-record/draft", Handler: handle(srv.handleDeleteDraft), Access: module.AnyUser, Features: lr}, - } -} - -type learningRecordFacility struct { - ID uint `json:"id"` - Name string `json:"name"` -} - -// maxFacilityOtherRunes caps the free-text location. It prints as a single -// line on the transcript, and the column itself is unbounded text. -const maxFacilityOtherRunes = 120 - -/* -validateLocation normalizes and checks the achievement location on an -incoming entry or draft. The transcript treats facility_id and facility_other -as mutually exclusive alternatives, so that invariant is enforced here instead -of trusting the client, an unknown facility_id gets a 400 rather than a -foreign key error, and facility_name is replaced with the name on record so a -client cannot label a facility as something it is not. -*/ -func (srv *server) validateLocation(entry *LearningRecordEntry) error { - // Whitespace-only text is the same as no text: the resident may have opened - // the "Other" field and typed nothing meaningful into it yet. - entry.FacilityOther = strings.TrimSpace(entry.FacilityOther) - entry.FacilityName = "" - if utf8.RuneCountInString(entry.FacilityOther) > maxFacilityOtherRunes { - return newBadRequestServiceError(nil, - fmt.Sprintf("location must be %d characters or fewer", maxFacilityOtherRunes)) - } - if entry.FacilityID == nil { - return nil - } - if entry.FacilityOther != "" { - return newBadRequestServiceError(nil, "location must be either a facility or free text, not both") - } - name, ok, err := srv.store.GetLocationFacility(*entry.FacilityID) - if err != nil { - return newDatabaseServiceError(err) - } - if !ok { - return newBadRequestServiceError(nil, "facility_id does not match a known facility") - } - entry.FacilityName = name - return nil -} - -// The main facilities routes are admin-only, so residents read the list -// through here, gated on the learning_record feature. -func (srv *server) handleIndexFacilities(w http.ResponseWriter, r *http.Request, log sLog) error { - facilities, err := srv.store.GetAllFacilitiesOrdered() - if err != nil { - return newDatabaseServiceError(err) - } - results := make([]learningRecordFacility, 0, len(facilities)) - for _, f := range facilities { - results = append(results, learningRecordFacility{ID: f.ID, Name: f.Name}) - } - return writeJsonResponse(w, http.StatusOK, results) -} - -func (srv *server) handleIndexEntries(w http.ResponseWriter, r *http.Request, log sLog) error { - userID := claimsFrom(r).UserID - entries, err := srv.store.GetEntries(userID) - if err != nil { - log.add("user_id", userID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusOK, entries) -} - -func (srv *server) handleCreateEntry(w http.ResponseWriter, r *http.Request, log sLog) error { - var entry LearningRecordEntry - if err := json.NewDecoder(r.Body).Decode(&entry); err != nil { - return newJSONReqBodyServiceError(err) - } - entry.UserID = claimsFrom(r).UserID - if err := srv.validateLocation(&entry); err != nil { - log.add("user_id", entry.UserID) - return err - } - if err := srv.WithUserContext(r).CreateEntry(&entry); err != nil { - log.add("user_id", entry.UserID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusCreated, entry) -} - -func (srv *server) handleUpdateEntry(w http.ResponseWriter, r *http.Request, log sLog) error { - id, err := strconv.Atoi(r.PathValue("id")) - if err != nil { - return newInvalidIdServiceError(err, "entry ID") - } - var entry LearningRecordEntry - if err := json.NewDecoder(r.Body).Decode(&entry); err != nil { - return newJSONReqBodyServiceError(err) - } - entry.ID = uint(id) - entry.UserID = claimsFrom(r).UserID - if err := srv.validateLocation(&entry); err != nil { - log.add("entry_id", id) - log.add("user_id", entry.UserID) - return err - } - if err := srv.WithUserContext(r).UpdateEntry(&entry); err != nil { - log.add("entry_id", id) - log.add("user_id", entry.UserID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusOK, entry) -} - -func (srv *server) handleDeleteEntry(w http.ResponseWriter, r *http.Request, log sLog) error { - id, err := strconv.Atoi(r.PathValue("id")) - if err != nil { - return newInvalidIdServiceError(err, "entry ID") - } - userID := claimsFrom(r).UserID - if err := srv.WithUserContext(r).DeleteEntry(uint(id), userID); err != nil { - log.add("entry_id", id) - log.add("user_id", userID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusOK, "Entry deleted successfully") -} - -func (srv *server) handleGetDraft(w http.ResponseWriter, r *http.Request, log sLog) error { - userID := claimsFrom(r).UserID - draft, err := srv.store.GetDraft(userID) - if err != nil { - log.add("user_id", userID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusOK, draft) -} - -func (srv *server) handleUpsertDraft(w http.ResponseWriter, r *http.Request, log sLog) error { - var draft LearningRecordEntry - if err := json.NewDecoder(r.Body).Decode(&draft); err != nil { - return newJSONReqBodyServiceError(err) - } - draft.UserID = claimsFrom(r).UserID - if err := srv.validateLocation(&draft); err != nil { - log.add("user_id", draft.UserID) - log.add("client_id", draft.ClientID) - return err - } - if err := srv.WithUserContext(r).UpsertDraft(&draft); err != nil { - log.add("user_id", draft.UserID) - log.add("client_id", draft.ClientID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusOK, draft) -} - -func (srv *server) handleDeleteDraft(w http.ResponseWriter, r *http.Request, log sLog) error { - userID := claimsFrom(r).UserID - clientID := r.URL.Query().Get("client_id") - if clientID == "" { - return newBadRequestServiceError(nil, "client_id query parameter is required") - } - if err := srv.WithUserContext(r).DeleteDraft(userID, clientID); err != nil { - log.add("user_id", userID) - log.add("client_id", clientID) - return newDatabaseServiceError(err) - } - return writeJsonResponse(w, http.StatusOK, "Draft deleted successfully") -} diff --git a/backend/src/modules/learningrecord/http.go b/backend/src/modules/learningrecord/http.go deleted file mode 100644 index a50f05edd..000000000 --- a/backend/src/modules/learningrecord/http.go +++ /dev/null @@ -1,197 +0,0 @@ -package learningrecord - -import ( - "context" - "encoding/json" - "errors" - "fmt" - "net/http" - - "github.com/UnlockedLabs/unlocked-core/backend/src/modules/module" - - "github.com/UnlockedLabs/unlocked-contracts/identity" - "github.com/sirupsen/logrus" - "gorm.io/gorm" -) - -// The response and error helpers Learning Record's handlers use. They are -// copies of core's (EN-188: handler helpers stay in each module), so every -// response is the same JSON as before the move. - -type resource[T any] struct { - Message string `json:"message"` - Data T `json:"data"` -} - -func writeJsonResponse[T any](w http.ResponseWriter, status int, data T) error { - if any(data) == nil { - w.WriteHeader(status) - return nil - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(status) - if str, ok := any(data).(string); ok { - resp := resource[struct{}]{Message: str, Data: struct{}{}} - if err := json.NewEncoder(w).Encode(resp); err != nil { - return newResponseServiceError(err) - } - return nil - } - resp := resource[T]{Message: "Data fetched successfully", Data: data} - if err := json.NewEncoder(w).Encode(resp); err != nil { - return newResponseServiceError(err) - } - return nil -} - -func errorResponse(w http.ResponseWriter, status int, message string) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(status) - if err := json.NewEncoder(w).Encode(resource[any]{Message: message}); err != nil { - logrus.Error("error writing error response: ", err) - http.Error(w, message, status) - } -} - -// handlerFunc is a route handler that returns its error for handle to write. -type handlerFunc func(w http.ResponseWriter, r *http.Request, log sLog) error - -// handle turns h into an http.Handler that writes h's error as core's -// handleError does. -func handle(h handlerFunc) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Start from core's fields, so an admin write's error line names the - // admin, as core's own handlers' lines do. - f := logrus.Fields(module.LogFields(r.Context())) - f["module"] = "learningrecord" - f["method"] = r.Method - f["path"] = r.URL.Path - log := sLog{ctx: r.Context(), f: f} - if err := h(w, r, log); err != nil { - var svcErr serviceError - if errors.As(err, &svcErr) { - errorResponse(w, svcErr.Status, svcErr.Message) - log.error("Error occurred is ", svcErr.Err) - } else { - errorResponse(w, http.StatusInternalServerError, err.Error()) - log.error("Error occurred is ", err.Error()) - } - } - }) -} - -// sLog adds fields to a request's log lines, like core's sLog. -type sLog struct { - ctx context.Context - f logrus.Fields -} - -// add adds a field to this module's log lines and to core's, so it also -// reaches the admin audit line, as it did before the move. -func (slog sLog) add(key string, value any) { - slog.f[key] = value - module.LogField(slog.ctx, key, value) -} - -func (slog sLog) error(args ...any) { - logrus.WithFields(slog.f).Error(args...) -} - -// serviceError is an error with the status and message a handler answers -// with. -type serviceError struct { - Status int - Message string - Err error -} - -func (svcErr serviceError) Error() string { - return fmt.Sprintf("service error: %s", svcErr.Err.Error()) -} - -func newServiceError(err error, status int, msg string) serviceError { - svcErr := serviceError{Status: status, Message: msg, Err: err} - if dbErr, ok := err.(dbError); ok { - svcErr.Err = errors.Join(errors.New(dbErr.Message), dbErr.InternalErr) - } - return svcErr -} - -func newBadRequestServiceError(err error, msg string) serviceError { - return newServiceError(err, http.StatusBadRequest, msg) -} - -func newResponseServiceError(err error) serviceError { - return newServiceError(err, http.StatusInternalServerError, "error writing response") -} - -func newJSONReqBodyServiceError(err error) serviceError { - return newServiceError(err, http.StatusBadRequest, "error reading json request") -} - -func newInvalidIdServiceError(err error, idName string) serviceError { - return newServiceError(err, http.StatusBadRequest, fmt.Sprintf("invalid %s", idName)) -} - -// newDatabaseServiceError answers with a dbError's status and message, and -// with a 500 and the error's text for any other error. -func newDatabaseServiceError(err error) serviceError { - if dbErr, ok := err.(dbError); ok { - return serviceError{Status: dbErr.Status, Message: dbErr.Message, Err: dbErr.InternalErr} - } - return serviceError{Status: http.StatusInternalServerError, Message: err.Error(), Err: err} -} - -// dbError is a failed query, with the status and message to answer with. -type dbError struct { - Status int - Message string - InternalErr error -} - -func (e dbError) Error() string { - return fmt.Sprintf("db error: %s", e.InternalErr.Error()) -} - -func newDBError(err error, msg string) dbError { - dbErr := dbError{Message: msg, InternalErr: err} - switch { - case errors.Is(err, gorm.ErrRecordNotFound), - errors.Is(err, gorm.ErrInvalidData), - errors.Is(err, gorm.ErrEmptySlice), - errors.Is(err, gorm.ErrInvalidField), - errors.Is(err, gorm.ErrInvalidValueOfLength): - dbErr.Status = http.StatusBadRequest - case errors.Is(err, gorm.ErrPrimaryKeyRequired), - errors.Is(err, gorm.ErrForeignKeyViolated), - errors.Is(err, gorm.ErrDuplicatedKey), - errors.Is(err, gorm.ErrCheckConstraintViolated): - dbErr.Status = http.StatusUnprocessableEntity - default: - dbErr.Status = http.StatusInternalServerError - } - return dbErr -} - -func newGetRecordsDBError(err error, table string) dbError { - return newDBError(err, fmt.Sprintf("error getting %s", table)) -} - -func newDeleteDBError(err error, table string) dbError { - return newDBError(err, fmt.Sprintf("error deleting %s", table)) -} - -func newCreateDBError(err error, table string) dbError { - return newDBError(err, fmt.Sprintf("error creating %s", table)) -} - -func newUpdateDBError(err error, table string) dbError { - return newDBError(err, fmt.Sprintf("error updating %s", table)) -} - -// claimsFrom is the caller of a request. Core only hands a module requests -// it has authenticated, so the claims are always there. -func claimsFrom(r *http.Request) *identity.Claims { - c, _ := identity.FromContext(r.Context()) - return c -} diff --git a/backend/src/modules/learningrecord/http_test.go b/backend/src/modules/learningrecord/http_test.go deleted file mode 100644 index d1341b857..000000000 --- a/backend/src/modules/learningrecord/http_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package learningrecord - -import ( - "context" - "errors" - "net/http" - "net/http/httptest" - "testing" - - "github.com/UnlockedLabs/unlocked-core/backend/src/modules/module" - - "github.com/sirupsen/logrus" - logtest "github.com/sirupsen/logrus/hooks/test" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// A failed request's error line carries core's fields, such as the admin on -// an admin write, as core's own handlers' error lines do. -func TestHandleErrorLogFields(t *testing.T) { - oldHooks := logrus.StandardLogger().ReplaceHooks(make(logrus.LevelHooks)) - defer logrus.StandardLogger().ReplaceHooks(oldHooks) - hook := logtest.NewGlobal() - - core := map[string]any{"admin_id": uint(3), "username": "facadmin", "handler": "NameNotFound"} - ctx := module.WithLog(context.Background(), core, func(string, any) {}) - r := httptest.NewRequestWithContext(ctx, http.MethodPut, "/api/learning-record/entries/1", nil) - w := httptest.NewRecorder() - handle(func(http.ResponseWriter, *http.Request, sLog) error { - return errors.New("boom") - }).ServeHTTP(w, r) - - require.Equal(t, http.StatusInternalServerError, w.Code) - entry := hook.LastEntry() - require.NotNil(t, entry) - assert.Equal(t, logrus.ErrorLevel, entry.Level) - assert.Equal(t, uint(3), entry.Data["admin_id"]) - assert.Equal(t, "facadmin", entry.Data["username"]) - assert.Equal(t, "learningrecord", entry.Data["module"]) - assert.Equal(t, http.MethodPut, entry.Data["method"]) - assert.Equal(t, "/api/learning-record/entries/1", entry.Data["path"]) -} diff --git a/backend/src/modules/learningrecord/migrations/00001_move_into_learning_record_schema.sql b/backend/src/modules/learningrecord/migrations/00001_move_into_learning_record_schema.sql deleted file mode 100644 index aa157ca97..000000000 --- a/backend/src/modules/learningrecord/migrations/00001_move_into_learning_record_schema.sql +++ /dev/null @@ -1,66 +0,0 @@ --- +goose Up --- Learning Record's table moves into its own schema, and its queries run as --- the learning_record role, which can write only that schema, read core's --- facilities, and add audit rows to core's user_account_history (EN-286). --- Goose runs this in one transaction, so a failure leaves the database as it --- was. --- --- SET SCHEMA keeps the table's rows, indexes, constraints, owned sequence and --- column defaults, and foreign keys keep working across schemas. --- --- The move holds a brief exclusive lock on the table. If a long query holds --- one first, this gives up instead of queueing every later query behind it; --- the transaction rolls back and the migration can run again. -SET LOCAL lock_timeout = '5s'; - -CREATE SCHEMA learning_record; - -ALTER TABLE public.learning_record_entries SET SCHEMA learning_record; - --- The role can't log in: the app connects as its own user and switches to --- learning_record with SET ROLE, so no new password is needed. Roles belong to --- the whole Postgres server, so it may already exist. This assumes one --- Unlocked database per server: a second one with a different app user fails --- at the GRANT below, and its Down can't drop a role another database uses. --- +goose StatementBegin -DO $$ -BEGIN - IF NOT EXISTS (SELECT FROM pg_roles WHERE rolname = 'learning_record') THEN - CREATE ROLE learning_record NOLOGIN; - END IF; -END -$$; --- +goose StatementEnd -GRANT learning_record TO CURRENT_USER; - -GRANT USAGE ON SCHEMA learning_record TO learning_record; -GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA learning_record TO learning_record; -GRANT USAGE, SELECT, UPDATE ON ALL SEQUENCES IN SCHEMA learning_record TO learning_record; --- Tables and sequences later migrations add here get the same grants. -ALTER DEFAULT PRIVILEGES IN SCHEMA learning_record GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO learning_record; -ALTER DEFAULT PRIVILEGES IN SCHEMA learning_record GRANT USAGE, SELECT, UPDATE ON SEQUENCES TO learning_record; - --- Core's tables: facilities for the achievement-location picker and the --- facility_name join, and user_account_history for the audit row an entry --- delete adds in the same transaction. Its id default calls nextval, which --- needs USAGE on the sequence. -GRANT USAGE ON SCHEMA public TO learning_record; -GRANT SELECT ON public.facilities TO learning_record; -GRANT INSERT ON public.user_account_history TO learning_record; -GRANT USAGE ON SEQUENCE public.user_account_history_id_seq TO learning_record; - --- +goose Down --- Down runs in its own transaction and takes the same lock moving back. -SET LOCAL lock_timeout = '5s'; - -ALTER DEFAULT PRIVILEGES IN SCHEMA learning_record REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM learning_record; -ALTER DEFAULT PRIVILEGES IN SCHEMA learning_record REVOKE USAGE, SELECT, UPDATE ON SEQUENCES FROM learning_record; - -ALTER TABLE learning_record.learning_record_entries SET SCHEMA public; - -DROP SCHEMA learning_record; - --- DROP OWNED revokes every grant the role holds in this database. -DROP OWNED BY learning_record; -REVOKE learning_record FROM CURRENT_USER; -DROP ROLE learning_record; diff --git a/backend/src/modules/learningrecord/migrations/README.md b/backend/src/modules/learningrecord/migrations/README.md deleted file mode 100644 index d3709cdd8..000000000 --- a/backend/src/modules/learningrecord/migrations/README.md +++ /dev/null @@ -1,10 +0,0 @@ -# Learning Record migrations - -Goose `.sql` migrations for this module, embedded in the server binary. They run -after core's, and their versions are tracked in `public.goose_db_version_learningrecord`. - -Never edit a migration that has been merged; add a new one. - -Create new tables as `learning_record.`. Migrations run on core's -connection, so an unqualified table lands in `public`, where the -`learning_record` role can't use it. diff --git a/backend/src/modules/learningrecord/models.go b/backend/src/modules/learningrecord/models.go deleted file mode 100644 index 73fe61010..000000000 --- a/backend/src/modules/learningrecord/models.go +++ /dev/null @@ -1,131 +0,0 @@ -package learningrecord - -import ( - "database/sql/driver" - "encoding/json" - "time" - - "github.com/UnlockedLabs/unlocked-contracts/identity" - "gorm.io/gorm" -) - -// DatabaseFields are the columns most tables share. The create and update -// user come from the identity.Claims in the query's context. -type DatabaseFields struct { - ID uint `gorm:"primaryKey" json:"id"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` - DeletedAt gorm.DeletedAt `gorm:"index" json:"-"` - - CreateUserID *uint `json:"create_user_id"` - UpdateUserID *uint `json:"update_user_id"` -} - -func (m *DatabaseFields) BeforeCreate(tx *gorm.DB) error { - if c, ok := identity.FromContext(tx.Statement.Context); ok { - m.CreateUserID = &c.UserID - } - return nil -} - -func (m *DatabaseFields) BeforeUpdate(tx *gorm.DB) error { - if c, ok := identity.FromContext(tx.Statement.Context); ok { - m.UpdateUserID = &c.UserID - tx.Statement.SetColumn("update_user_id", c.UserID) - } - return nil -} - -// StringSlice persists a Go string slice as a JSON text column. -type StringSlice []string - -func (s StringSlice) Value() (driver.Value, error) { - if s == nil { - return "[]", nil - } - b, err := json.Marshal(s) - return string(b), err -} - -func (s *StringSlice) Scan(value any) error { - var raw string - switch v := value.(type) { - case string: - raw = v - case []byte: - raw = string(v) - default: - raw = "[]" - } - return json.Unmarshal([]byte(raw), s) -} - -type LearningRecordEntry struct { - DatabaseFields - UserID uint `gorm:"not null" json:"user_id"` - ClientID string `gorm:"not null" json:"client_id"` - IsDraft bool `gorm:"not null;default:false" json:"is_draft"` - StepIndex int `json:"step_index"` - UiPhase string `json:"ui_phase"` - EditingEntryID *uint `json:"editing_entry_id"` - ProgramName string `json:"program_name"` - FacilityID *uint `json:"facility_id"` - FacilityOther string `json:"facility_other"` - FacilityName string `gorm:"->;-:migration" json:"facility_name"` - CompletionDate string `json:"completion_date"` - Confidence string `json:"confidence"` - Summary string `json:"summary"` - TopSkills StringSlice `gorm:"type:text" json:"top_skills"` - BarrierToCompletion string `json:"barrier_to_completion"` - GoalConnection string `json:"goal_connection"` - Pride string `json:"pride"` - StandoutMoment string `json:"standout_moment"` - AdviceToPeer string `json:"advice_to_peer"` - ChallengeToggle *string `json:"challenge_toggle"` - ChallengeText string `json:"challenge_text"` - SkillTagsBefore StringSlice `gorm:"type:text" json:"skill_tags_before"` - SkillTagsAfter StringSlice `gorm:"type:text" json:"skill_tags_after"` - SkillReflection string `json:"skill_reflection"` - GrowthReflection string `json:"growth_reflection"` - SupportSelections StringSlice `gorm:"type:text" json:"support_selections"` - NextStepSelections StringSlice `gorm:"type:text" json:"next_step_selections"` -} - -func (LearningRecordEntry) TableName() string { return "learning_record_entries" } - -// facility is the read Learning Record makes of core's facilities table: the -// id and name shown in the achievement-location picker. It carries -// DatabaseFields' DeletedAt so GORM's default soft-delete scope excludes a -// deleted facility from that list, the same as models.Facility does in core. -type facility struct { - DatabaseFields - Name string `gorm:"size:255;not null"` -} - -func (facility) TableName() string { return "facilities" } - -// UserAccountHistory is a row of core's user_account_history table. Learning -// Record adds one when a resident deletes an entry. -type UserAccountHistory struct { - UserID uint `json:"user_id" gorm:"primaryKey"` - AdminID *uint `json:"admin_id"` - Action ActivityHistoryAction `json:"action" gorm:"size:255;primaryKey"` - CreatedAt time.Time `json:"created_at" gorm:"primaryKey"` - AttendanceStatus string `json:"attendance_status" gorm:"size:27"` -} - -func (UserAccountHistory) TableName() string { return "user_account_history" } - -// ActivityHistoryAction is a user_account_history action. -type ActivityHistoryAction string - -const LearningRecordDeleted ActivityHistoryAction = "learning_record_deleted" - -func NewUserAccountHistory(userID uint, action ActivityHistoryAction, adminID *uint) *UserAccountHistory { - return &UserAccountHistory{ - UserID: userID, - AdminID: adminID, - Action: action, - CreatedAt: time.Now(), - } -} diff --git a/backend/src/modules/learningrecord/module.go b/backend/src/modules/learningrecord/module.go deleted file mode 100644 index 20abda23a..000000000 --- a/backend/src/modules/learningrecord/module.go +++ /dev/null @@ -1,50 +0,0 @@ -// Package learningrecord is the Learning Record feature module: learning -// record entries. Its code moved here from core in EN-195. -package learningrecord - -import ( - "embed" - "io/fs" - - "github.com/UnlockedLabs/unlocked-core/backend/src/modules/module" -) - -//go:embed migrations -var migrations embed.FS - -// Module is the Learning Record module. -type Module struct{} - -var ( - _ module.Module = Module{} - _ module.Schema = Module{} - _ module.TestModels = Module{} -) - -func (Module) Name() string { return "learningrecord" } - -// Schema is the Postgres schema that holds Learning Record's table. -func (Module) Schema() string { return "learning_record" } - -// server is what the routes share. -type server struct { - store *Store -} - -// Register adds Learning Record's routes. -func (Module) Register(r module.Router, deps module.Deps) { - srv := &server{store: &Store{deps.DB}} - for _, rt := range srv.routes() { - r.Handle(rt) - } -} - -// RegisterJobs does nothing: Learning Record has no background work. -func (Module) RegisterJobs(module.Scheduler, module.Deps) {} - -func (Module) Migrations() fs.FS { return migrations } - -// TestModels is Learning Record's table, for core's SQLite test database. -func (Module) TestModels() []any { - return []any{&LearningRecordEntry{}} -} diff --git a/backend/src/modules/learningrecord/store.go b/backend/src/modules/learningrecord/store.go deleted file mode 100644 index 41128780f..000000000 --- a/backend/src/modules/learningrecord/store.go +++ /dev/null @@ -1,189 +0,0 @@ -package learningrecord - -import ( - "errors" - "net/http" - - "github.com/UnlockedLabs/unlocked-contracts/identity" - - "gorm.io/gorm" - "gorm.io/gorm/clause" -) - -// Store is Learning Record's database. Its queries moved from core's -// database package unchanged (EN-195). -type Store struct{ *gorm.DB } - -// WithUserContext is the store with r's context, so the audit hooks and the -// explicit actor reads in DeleteEntry and UpsertDraft record the caller as -// the row's update user, the same as core's srv.WithUserContext(r) did. -func (srv *server) WithUserContext(r *http.Request) *Store { - return &Store{srv.store.WithContext(r.Context())} -} - -func withFacilityName(db *Store) *gorm.DB { - return db.Model(&LearningRecordEntry{}). - Select("learning_record_entries.*, COALESCE(facilities.name, '') AS facility_name"). - Joins("LEFT JOIN facilities ON facilities.id = learning_record_entries.facility_id") -} - -/* -GetLocationFacility resolves a facility id used as an achievement location, -returning the facility's name and whether it exists at all. The lookup is -unscoped on purpose: an achievement can point at a facility that has since -been soft-deleted, and the foreign key still holds, so rejecting those would -block edits to older records. ok is false only for an id that is not a -facility. -*/ -func (db *Store) GetLocationFacility(id uint) (string, bool, error) { - var names []string - if err := db.Unscoped().Model(&facility{}). - Where("id = ?", id). - Limit(1). - Pluck("name", &names).Error; err != nil { - return "", false, newGetRecordsDBError(err, "facilities") - } - if len(names) == 0 { - return "", false, nil - } - return names[0], true, nil -} - -// GetAllFacilitiesOrdered is every facility Learning Record's location picker -// offers, alphabetical. GORM's default scope excludes a soft-deleted one. -func (db *Store) GetAllFacilitiesOrdered() ([]facility, error) { - var facilities []facility - if err := db.Order("name").Find(&facilities).Error; err != nil { - return nil, newGetRecordsDBError(err, "facilities") - } - return facilities, nil -} - -func (db *Store) GetEntries(userID uint) ([]LearningRecordEntry, error) { - entries := make([]LearningRecordEntry, 0) - if err := withFacilityName(db). - Where("learning_record_entries.user_id = ? AND learning_record_entries.is_draft = false", userID). - Order("learning_record_entries.created_at DESC"). - Find(&entries).Error; err != nil { - return nil, newGetRecordsDBError(err, "learning_record_entries") - } - return entries, nil -} - -func (db *Store) CreateEntry(entry *LearningRecordEntry) error { - if err := db.Create(entry).Error; err != nil { - return newCreateDBError(err, "learning_record_entries") - } - return nil -} - -func (db *Store) UpdateEntry(entry *LearningRecordEntry) error { - result := db.Model(entry). - Where("id = ? AND user_id = ?", entry.ID, entry.UserID). - Updates(entryColumns(entry)) - if result.Error != nil { - return newUpdateDBError(result.Error, "learning_record_entries") - } - return nil -} - -func (db *Store) DeleteEntry(id, userID uint) error { - actorID := userID - if ctx := db.Statement.Context; ctx != nil { - if c, ok := identity.FromContext(ctx); ok { - actorID = c.UserID - } - } - return db.Transaction(func(tx *gorm.DB) error { - if err := tx.Model(&LearningRecordEntry{}). - Where("id = ? AND user_id = ?", id, userID). - Update("update_user_id", actorID).Error; err != nil { - return newUpdateDBError(err, "learning_record_entries") - } - if err := tx.Where("id = ? AND user_id = ?", id, userID). - Delete(&LearningRecordEntry{}).Error; err != nil { - return newDeleteDBError(err, "learning_record_entries") - } - history := NewUserAccountHistory(userID, LearningRecordDeleted, &actorID) - if err := tx.Create(history).Error; err != nil { - return newCreateDBError(err, "user_account_history") - } - return nil - }) -} - -// GetDraft returns the most recently updated draft for the user, or nil. -func (db *Store) GetDraft(userID uint) (*LearningRecordEntry, error) { - var draft LearningRecordEntry - err := withFacilityName(db). - Where("learning_record_entries.user_id = ? AND learning_record_entries.is_draft = true", userID). - Order("learning_record_entries.updated_at DESC"). - First(&draft).Error - if err != nil { - if errors.Is(err, gorm.ErrRecordNotFound) { - return nil, nil - } - return nil, newGetRecordsDBError(err, "learning_record_entries") - } - return &draft, nil -} - -// UpsertDraft inserts or updates a draft row keyed on (user_id, client_id). -// Its ON CONFLICT target doesn't match migration 00069's partial unique -// index, so it 500s on every call, in production too -- see README.md's -// Tests section. -func (db *Store) UpsertDraft(draft *LearningRecordEntry) error { - draft.IsDraft = true - updateMap := draftColumns(draft) - updateMap["is_draft"] = true - updateMap["updated_at"] = gorm.Expr("NOW()") - if ctx := db.Statement.Context; ctx != nil { - if c, ok := identity.FromContext(ctx); ok { - updateMap["update_user_id"] = c.UserID - } - } - if err := db.Clauses(clause.OnConflict{ - Columns: []clause.Column{{Name: "user_id"}, {Name: "client_id"}}, - DoUpdates: clause.Assignments(updateMap), - }).Create(draft).Error; err != nil { - return newCreateDBError(err, "learning_record_entries") - } - return nil -} - -func (db *Store) DeleteDraft(userID uint, clientID string) error { - if err := db.Where("user_id = ? AND client_id = ? AND is_draft = true", userID, clientID). - Delete(&LearningRecordEntry{}).Error; err != nil { - return newDeleteDBError(err, "learning_record_entries") - } - return nil -} - -// entryColumns holds the achievement fields shared by drafts and committed -// entries. The draft-only wizard state (step_index, ui_phase, -// editing_entry_id) is excluded so editing a committed entry cannot reset it. -func entryColumns(e *LearningRecordEntry) map[string]any { - return map[string]any{ - "program_name": e.ProgramName, - "facility_id": e.FacilityID, "facility_other": e.FacilityOther, - "completion_date": e.CompletionDate, "confidence": e.Confidence, - "summary": e.Summary, "top_skills": e.TopSkills, - "barrier_to_completion": e.BarrierToCompletion, "goal_connection": e.GoalConnection, - "pride": e.Pride, "standout_moment": e.StandoutMoment, - "advice_to_peer": e.AdviceToPeer, "challenge_toggle": e.ChallengeToggle, - "challenge_text": e.ChallengeText, "skill_tags_before": e.SkillTagsBefore, - "skill_tags_after": e.SkillTagsAfter, "skill_reflection": e.SkillReflection, - "growth_reflection": e.GrowthReflection, "support_selections": e.SupportSelections, - "next_step_selections": e.NextStepSelections, - } -} - -// draftColumns adds the draft-only wizard state to the shared achievement -// fields; use it for draft upserts only. -func draftColumns(e *LearningRecordEntry) map[string]any { - columns := entryColumns(e) - columns["step_index"] = e.StepIndex - columns["ui_phase"] = e.UiPhase - columns["editing_entry_id"] = e.EditingEntryID - return columns -} diff --git a/backend/src/modules/registry/registry.go b/backend/src/modules/registry/registry.go index 40c501f2b..190b5083a 100644 --- a/backend/src/modules/registry/registry.go +++ b/backend/src/modules/registry/registry.go @@ -8,19 +8,16 @@ import ( "net/http" "os" - "github.com/UnlockedLabs/unlocked-core/backend/src/modules/learningrecord" "github.com/UnlockedLabs/unlocked-core/backend/src/modules/module" "github.com/UnlockedLabs/unlocked-contracts/providerplatforms" ) // All returns every feature module, in the order their routes and jobs are -// registered and their migrations run. Provider Platforms and Programs are -// their own services, so they aren't here. +// registered and their migrations run. Open Content, Provider Platforms, +// Programs and Learning Record run as their own services, so none is here yet. func All() []module.Module { - return []module.Module{ - learningrecord.Module{}, - } + return []module.Module{} } // ProviderPlatformsURL is where the Provider Platforms service is: diff --git a/backend/src/modules/registry/registry_test.go b/backend/src/modules/registry/registry_test.go index d29867f05..267cfb107 100644 --- a/backend/src/modules/registry/registry_test.go +++ b/backend/src/modules/registry/registry_test.go @@ -25,12 +25,10 @@ func TestAllModules(t *testing.T) { t.Errorf("module %q has no migrations folder: %v", name, err) } } - // Open Content, Provider Platforms and Programs run as their own services. - if len(seen) != 1 { - t.Errorf("got %d modules, want 1: Learning Record", len(seen)) - } - if seen["providerplatforms"] || seen["opencontent"] || seen["programs"] { - t.Error("Open Content, Provider Platforms and Programs run as their own services, not as core modules") + // Open Content, Provider Platforms, Programs and Learning Record run as their + // own services. + if len(seen) != 0 { + t.Errorf("got %d modules, want none", len(seen)) } } diff --git a/backend/tests/integration/facility_feature_flags_test.go b/backend/tests/integration/facility_feature_flags_test.go index 4364cddd6..b9bf7ac68 100644 --- a/backend/tests/integration/facility_feature_flags_test.go +++ b/backend/tests/integration/facility_feature_flags_test.go @@ -474,37 +474,3 @@ func TestFacilityFeatureRoutes_EnabledRequired(t *testing.T) { require.NotContains(t, effective, models.ProgramAccess) }) } - -// TestFeatureFlags_MiddlewareEnforcesClaims proves the core regression this -// feature fixes: a route gated on a feature must reject a request whose claims -// don't carry that feature, not just record the toggle in the database. Claims -// here stand in for what auth.go's per-facility resolution would have produced. -func TestFeatureFlags_MiddlewareEnforcesClaims(t *testing.T) { - env = SetupTestEnv(t) - defer env.CleanupTestEnv() - - facility, err := env.CreateTestFacility("Enforcement Facility") - require.NoError(t, err) - - t.Run("student without the learning record is rejected from a route gated on it", func(t *testing.T) { - NewRequest[any](env.Client, t, http.MethodGet, "/api/learning-record/entries", nil). - WithTestClaims(&handlers.Claims{ - Role: models.Student, - FacilityID: facility.ID, - FeatureAccess: []models.FeatureAccess{}, - }). - Do(). - ExpectStatus(http.StatusUnauthorized) - }) - - t.Run("student with the learning record succeeds on the same route", func(t *testing.T) { - NewRequest[any](env.Client, t, http.MethodGet, "/api/learning-record/entries", nil). - WithTestClaims(&handlers.Claims{ - Role: models.Student, - FacilityID: facility.ID, - FeatureAccess: []models.FeatureAccess{models.LearningRecordAccess}, - }). - Do(). - ExpectStatus(http.StatusOK) - }) -} diff --git a/backend/tests/integration/learning_record_routes_test.go b/backend/tests/integration/learning_record_routes_test.go deleted file mode 100644 index c9140e042..000000000 --- a/backend/tests/integration/learning_record_routes_test.go +++ /dev/null @@ -1,206 +0,0 @@ -package integration - -import ( - "fmt" - "net/http" - "testing" - - "github.com/UnlockedLabs/unlocked-core/backend/src/handlers" - "github.com/UnlockedLabs/unlocked-core/backend/src/models" - "github.com/UnlockedLabs/unlocked-core/backend/src/modules/learningrecord" - - "github.com/stretchr/testify/require" -) - -// These tests pin what every Learning Record route answers (EN-195): the -// status and the JSON, who may call it, and the two behaviors invisible in a -// JSON response body (a soft-deleted facility is excluded from the location -// picker, and deleting an entry records who deleted it). They must pass -// unchanged after the move. -// -// Regenerate the golden files after an intended change with: -// -// go test ./tests/integration -run 'TestLearningRecord' -update - -type learningRecordWorld struct { - north, south *models.Facility - resident *models.User -} - -func (w learningRecordWorld) claims(u *models.User) *handlers.Claims { - return &handlers.Claims{UserID: u.ID, Username: u.Username, Role: u.Role, FacilityID: w.north.ID, FacilityName: w.north.Name} -} - -// seedLearningRecord adds two facilities and a soft-deleted third (to prove -// GetAllFacilitiesOrdered excludes it, same as it does today), plus one -// resident. -func seedLearningRecord(t *testing.T, env *TestEnv) learningRecordWorld { - t.Helper() - var w learningRecordWorld - var err error - w.north, err = env.CreateTestFacility("North") - require.NoError(t, err) - w.south, err = env.CreateTestFacility("South") - require.NoError(t, err) - retired, err := env.CreateTestFacility("Retired") - require.NoError(t, err) - require.NoError(t, env.DB.Delete(retired).Error) - w.resident, err = env.CreateTestUser("jdoe", models.Student, w.north.ID, "1001") - require.NoError(t, err) - return w -} - -func TestLearningRecordRoutes(t *testing.T) { - env := SetupTestEnv(t) - defer env.CleanupTestEnv() - w := seedLearningRecord(t, env) - client := noRedirects(env) - resident := w.claims(w.resident) - - tooLong := make([]byte, 121) - for i := range tooLong { - tooLong[i] = 'a' - } - - tests := []struct { - name string - method string - path string - body any - status int - }{ - {"facilities", http.MethodGet, "/api/learning-record/facilities", nil, http.StatusOK}, - {"entries_empty", http.MethodGet, "/api/learning-record/entries", nil, http.StatusOK}, - {"entry_create_both_facility_fields", http.MethodPost, "/api/learning-record/entries", - map[string]any{"facility_id": 1, "facility_other": "Community Center"}, http.StatusBadRequest}, - {"entry_create_bad_facility_id", http.MethodPost, "/api/learning-record/entries", - map[string]any{"facility_id": 9999}, http.StatusBadRequest}, - {"entry_create_facility_other_too_long", http.MethodPost, "/api/learning-record/entries", - map[string]any{"facility_other": string(tooLong)}, http.StatusBadRequest}, - {"entry_create", http.MethodPost, "/api/learning-record/entries", - map[string]any{"client_id": "entry-1", "program_name": "GED Prep", "facility_id": w.north.ID, "completion_date": "2026-01-15", "confidence": "high", "summary": "Finished the program."}, - http.StatusCreated}, - {"entry_create_facility_other", http.MethodPost, "/api/learning-record/entries", - map[string]any{"client_id": "entry-2", "program_name": "Peer Mentoring", "facility_other": "Community Center", "completion_date": "2026-02-01"}, - http.StatusCreated}, - {"entries_after_creates", http.MethodGet, "/api/learning-record/entries", nil, http.StatusOK}, - {"entry_update", http.MethodPut, "/api/learning-record/entries/1", - map[string]any{"program_name": "GED Prep (Advanced)", "facility_other": "New Location", "completion_date": "2026-01-20"}, - http.StatusOK}, - {"entry_update_bad_id", http.MethodPut, "/api/learning-record/entries/abc", map[string]any{}, http.StatusBadRequest}, - {"entry_delete", http.MethodDelete, "/api/learning-record/entries/1", nil, http.StatusOK}, - {"entries_after_delete", http.MethodGet, "/api/learning-record/entries", nil, http.StatusOK}, - {"draft_get_none", http.MethodGet, "/api/learning-record/draft", nil, http.StatusOK}, - // draft_upsert (PUT /api/learning-record/draft) is a pre-existing, - // pinned-as-broken route; see README.md's Tests section. - {"draft_upsert", http.MethodPut, "/api/learning-record/draft", - map[string]any{"client_id": "draft-upsert", "program_name": "Welding Basics"}, http.StatusInternalServerError}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - resp := NewRequest[any](client, t, tt.method, tt.path, tt.body). - WithTestClaims(resident).AsRaw().Do(). - ExpectStatus(tt.status) - expectGolden(t, "learning_record", tt.name, resp.rawBody) - }) - } - - // The GET and DELETE draft routes don't go through UpsertDraft, so a - // draft is seeded directly to cover them independently of that route. - draft := learningrecord.LearningRecordEntry{UserID: w.resident.ID, ClientID: "draft-1", IsDraft: true, ProgramName: "Welding Basics", StepIndex: 1, UiPhase: "survey"} - require.NoError(t, env.DB.Create(&draft).Error) - for _, tt := range []struct { - name string - method string - path string - status int - }{ - {"draft_get_after_seed", http.MethodGet, "/api/learning-record/draft", http.StatusOK}, - {"draft_delete_missing_client_id", http.MethodDelete, "/api/learning-record/draft", http.StatusBadRequest}, - {"draft_delete", http.MethodDelete, "/api/learning-record/draft?client_id=draft-1", http.StatusOK}, - {"draft_get_after_delete", http.MethodGet, "/api/learning-record/draft", http.StatusOK}, - } { - t.Run(tt.name, func(t *testing.T) { - resp := NewRequest[any](client, t, tt.method, tt.path, nil). - WithTestClaims(resident).AsRaw().Do(). - ExpectStatus(tt.status) - expectGolden(t, "learning_record", tt.name, resp.rawBody) - }) - } - - // The HTTP response never shows who recorded the deletion; the audit - // trail (written through Store.WithUserContext, which records the actor - // from the request's identity claims) does. - t.Run("entry_delete_records_audit_history", func(t *testing.T) { - var history struct { - Action string - AdminID *uint - AttendanceStatus *string - } - err := env.DB.Table("user_account_history"). - Select("action, admin_id, attendance_status"). - Where("user_id = ? AND action = ?", w.resident.ID, "learning_record_deleted"). - Order("created_at DESC").Limit(1).Scan(&history).Error - require.NoError(t, err) - require.Equal(t, "learning_record_deleted", history.Action) - require.NotNil(t, history.AdminID) - require.Equal(t, w.resident.ID, *history.AdminID) - // Core wrote this column's zero value before the move; a module struct - // that drops the field writes NULL instead, changing the account-history - // response body. - require.NotNil(t, history.AttendanceStatus) - require.Equal(t, "", *history.AttendanceStatus) - }) - - // The assertion above reads the row directly; this reads the endpoint - // that serves it to an admin, the real consumer of attendance_status, so - // both boundaries are pinned (the same two-level pin programs' test uses). - t.Run("account_history_endpoint", func(t *testing.T) { - admin, err := env.CreateTestUser("lradmin", models.FacilityAdmin, w.north.ID, "") - require.NoError(t, err) - resp := NewRequest[any](client, t, http.MethodGet, - fmt.Sprintf("/api/users/%d/account-history", w.resident.ID), nil). - WithTestClaims(w.claims(admin)).AsRaw().Do(). - ExpectStatus(http.StatusOK) - expectGolden(t, "learning_record", "account_history", resp.rawBody) - }) -} - -// TestLearningRecordRouteAccess pins each route's access level and feature -// flag against every route this module registers. -func TestLearningRecordRouteAccess(t *testing.T) { - env := SetupTestEnv(t) - defer env.CleanupTestEnv() - w := seedLearningRecord(t, env) - client := noRedirects(env) - - routes := []struct { - method, path string - }{ - {http.MethodGet, "/api/learning-record/facilities"}, - {http.MethodGet, "/api/learning-record/entries"}, - {http.MethodPost, "/api/learning-record/entries"}, - {http.MethodPut, "/api/learning-record/entries/1"}, - {http.MethodDelete, "/api/learning-record/entries/1"}, - {http.MethodGet, "/api/learning-record/draft"}, - {http.MethodPut, "/api/learning-record/draft"}, - {http.MethodDelete, "/api/learning-record/draft"}, - } - - for _, rt := range routes { - t.Run(rt.method+" "+rt.path, func(t *testing.T) { - // Every route requires the learning_record feature. - NewRequest[any](client, t, rt.method, rt.path, map[string]any{}). - WithTestClaims(withFeatures(w.claims(w.resident), models.LearningRecordAccess)).Do(). - ExpectStatus(http.StatusUnauthorized).ExpectMessage("Feature not enabled") - - // Every route is anyUser: a resident with the feature gets past - // auth. The body isn't valid JSON; a route that reads one - // (POST/PUT) rejects it before doing anything else. DELETE never - // reads a body, so it still runs its normal delete here. - resp := NewRequest[any](client, t, rt.method, rt.path, "not an object"). - WithTestClaims(w.claims(w.resident)).AsRaw().Do() - require.NotEqual(t, http.StatusUnauthorized, resp.resp.StatusCode, resp.rawBody) - }) - } -} diff --git a/backend/tests/integration/testdata/learning_record/account_history.golden b/backend/tests/integration/testdata/learning_record/account_history.golden deleted file mode 100644 index 8c4d12d60..000000000 --- a/backend/tests/integration/testdata/learning_record/account_history.golden +++ /dev/null @@ -1,26 +0,0 @@ -{ - "data": [ - { - "action": "learning_record_deleted", - "admin_username": "jdoe", - "attendance_status": "", - "class_name": null, - "created_at": "