From a5c9678acf6c6fb3f312f5c618033e2cadcbabff Mon Sep 17 00:00:00 2001 From: Greg Kostin Date: Wed, 29 Apr 2026 16:16:34 -0400 Subject: [PATCH 1/4] =?UTF-8?q?docs:=20add=20Task=20A=20=E2=80=94=20genera?= =?UTF-8?q?lize=20coding-agent=20markdown=20files=20to=20clean-up=20TODO?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- TODO.md | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/TODO.md b/TODO.md index 4fc02419841..32acb98269c 100644 --- a/TODO.md +++ b/TODO.md @@ -1,11 +1,52 @@ -# TODO — show-password-login implementation +# TODO — mlibrary/dspace-angular -Tasks are listed in dependency order. Complete them top-to-bottom. -Move each task to `DONE.md` when it is committed to the `show-password-login` branch. +Tasks are listed in dependency order within each work item. +Complete subtasks top-to-bottom, then move the finished task to `DONE.md`. + +Multiple work items may be in progress in parallel; each is tracked under its own heading. + +--- + +## Task A — Generalize coding-agent markdown files + +**Branch:** `clean-up` +**Goal:** Replace the `show-password-login`-specific agent files (`AGENTS.md`, `TODO.md`, +`DONE.md`) with general-purpose equivalents that can track multiple plans and work items +in parallel, and move plan documents into a dedicated `plans/` directory. + +### Subtasks (in order) + +- [ ] **A1. Create `plans/` directory** — move `PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md` + into `plans/` so all per-feature design documents live in one place. + Update any cross-references inside the file to reflect its new path. + +- [ ] **A2. Rewrite `AGENTS.md`** — strip out all `show-password-login`-specific content + and replace with repo-wide guidance: + - Repository overview (framework, language, conventions). + - How to find work: read `TODO.md`, read the relevant `plans/` document. + - General coding guidelines (currently guideline 9 about non-interactive CLI flags — + keep and expand as the canonical list for all agents). + - Reusable codebase patterns worth knowing (e.g. `APP_CONFIG` injection, Angular + testing patterns, `By.directive` vs `CUSTOM_ELEMENTS_SCHEMA`). + - Remove all references to a specific branch, ticket, or feature. + +- [ ] **A3. Rewrite `TODO.md`** (this file) — convert to the general format illustrated + here: tasks grouped by work item, each referencing its plan document, no hardcoded + branch or feature names in the file header. + +- [ ] **A4. Rewrite `DONE.md`** — convert to a general archive grouped by work item / + feature, each group headed by the feature name and merged-PR reference rather than + a flat chronological list tied to `show-password-login`. + +- [ ] **A5. Add a `plans/PLAN_TEMPLATE.md`** — a blank plan template that a human or + agent can copy when starting a new feature, with sections for: Problem, Chosen + Solution, Key Files, Truth Table / Example Config, Verification Steps. + +- [ ] **A6. Commit all changes** on `clean-up` and open a PR against `umich`. --- -## 9. Note in Kubernetes repo (out-of-band — different repository) +## Task 9 — Kubernetes demo config (out-of-band — different repository) This task is tracked here for completeness; it is performed in `mlibrary/deepblue-documents-kube`, not in this repo. From 6c4b903d5d3a2089c5c17a388a5ecf018e270ccb Mon Sep 17 00:00:00 2001 From: Greg Kostin Date: Wed, 29 Apr 2026 16:19:49 -0400 Subject: [PATCH 2/4] =?UTF-8?q?docs:=20generalize=20coding-agent=20markdow?= =?UTF-8?q?n=20files=20=E2=80=94=20plans/=20dir,=20general=20AGENTS/TODO/D?= =?UTF-8?q?ONE,=20plan=20template?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- AGENTS.md | 221 ++++++++---------- DONE.md | 149 ++++-------- TODO.md | 10 +- .../PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md | 3 +- plans/PLAN_TEMPLATE.md | 83 +++++++ 5 files changed, 222 insertions(+), 244 deletions(-) rename PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md => plans/PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md (99%) create mode 100644 plans/PLAN_TEMPLATE.md diff --git a/AGENTS.md b/AGENTS.md index bfb74a7a9ce..ce5f701a603 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,91 +1,87 @@ -# AGENTS.md — Coding Agent Guidelines for `show-password-login` +# AGENTS.md — Coding Agent Guidelines for `mlibrary/dspace-angular` ## Purpose -This file gives a coding agent everything it needs to continue (or complete) the -`show-password-login` feature branch in `mlibrary/dspace-angular` without needing -to ask the human for context. +This file gives a coding agent everything it needs to work in the +`mlibrary/dspace-angular` repository without needing to ask the human for context. +It covers repo orientation, how to find and track work, general coding guidelines, +and reusable codebase patterns. --- -## Repository & Branch +## Repository Overview | Item | Value | |---|---| | Repo | `mlibrary/dspace-angular` | -| Active branch | `show-password-login` | -| Base branch | `umich` / `issue-working` | +| Primary branch | `umich` | | Framework | Angular 14 / DSpace 7.6 | | Language | TypeScript + HTML templates | +| Fork of | `DSpace/dspace-angular` (upstream) | ---- - -## The Problem Being Solved - -A UM-specific customisation in `src/app/shared/log-in/log-in.component.html` hard-codes -the password auth method out of the login UI: - -```html -*ngIf="authMethod.authMethodType !== 'password'" -``` - -On the **demo** environment, DSpace OIDC is disabled, making `password` the only advertised -auth method — leaving a completely blank login form. No one can log in to demo. - -Full context: read `PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md`. +This is the University of Michigan customisation of the DSpace Angular frontend. +UM-specific changes live alongside standard DSpace code; prefer touching only the +files necessary for each task so merging upstream updates stays manageable. --- -## The Chosen Solution - -A **config-driven exclusive toggle**: a new `showPasswordLogin` boolean in `AuthConfig`. +## Finding and Tracking Work -- `false` (default) → show OIDC/non-password methods only. Production & workshop unchanged. -- `true` (demo only) → show password form only, hide OIDC. - -Template condition (exclusive — never both, never none): -```html -*ngIf="showPasswordLogin === (authMethod.authMethodType === 'password')" -``` +1. **Read `TODO.md` first.** It lists all active work items in priority order. +2. **Read the relevant plan document** in `plans/` before touching any code. + Each plan has background, rationale, exact code snippets, and verification steps. +3. **After completing and committing each task**, mark it done in `TODO.md` and add + a summary entry to `DONE.md`. +4. Plan documents follow the naming convention `plans/PLAN_.md`. + Copy `plans/PLAN_TEMPLATE.md` to start a new plan. --- -## Work Tracking Files +## Coding Guidelines -| File | Purpose | -|---|---| -| `TODO.md` | Remaining tasks in priority order — work from top to bottom | -| `DONE.md` | Completed tasks — move items here when a task is committed | -| `PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md` | Full design rationale and exact code snippets | +1. **Do not change files outside the scope of the current task** unless fixing a + compile/lint error directly caused by your own changes. + +2. **After editing each TypeScript file**, run `get_errors` on that file and fix any + issues before moving on. + +3. **Run affected specs** after updating spec files: + ``` + npx ng test --include='**/path/to/component.spec.ts' \ + --watch=false --configuration test --browsers=ChromeHeadless + ``` + +4. **Commit message convention** — use the Angular commit format: + ``` + : () + ``` + Types: `feat`, `fix`, `test`, `docs`, `refactor`, `chore`. + +5. **Always pass non-interactive flags to CLI tools** so commands never hang: + - `git`: use `git --no-pager ` (or pipe through `| cat`) for any command + that may open a pager (`log`, `diff`, `show`, `blame`, etc.). + - `ng test`: always include `--watch=false --browsers=ChromeHeadless`. + Without `--watch=false` the process never exits; without `--browsers=ChromeHeadless` + it tries to launch a visible browser window. + - Any other interactive tool (`less`, `man`, `top`, etc.): pipe through `| cat` or + pass the equivalent "non-interactive / no-pager" flag before running. -**Always check `TODO.md` first** to see what still needs doing. -**After completing and committing each task**, move it from `TODO.md` to `DONE.md`. +6. **Do not modify `config/config.yml`** — that file holds local dev overrides and is + not committed. Environment-specific runtime config is injected via environment + variables in Kubernetes (see plan documents for details). --- -## Key Files to Know - -| File | Role | -|---|---| -| `src/config/auth-config.interfaces.ts` | `AuthConfig` interface — add `showPasswordLogin?: boolean` | -| `src/config/default-app-config.ts` | `DefaultAppConfig` class — set `showPasswordLogin: false` in `auth` block | -| `src/app/shared/log-in/log-in.component.ts` | Inject `APP_CONFIG`, read flag, expose as `showPasswordLogin` property | -| `src/app/shared/log-in/log-in.component.html` | Replace `*ngIf` with exclusive-toggle condition | -| `src/app/shared/log-in/log-in.component.spec.ts` | Unit test — needs `APP_CONFIG` provided and assertion updated to `toBe(1)` | -| `src/app/shared/testing/auth-service.stub.ts` | `authMethodsMock` = `[password, shibboleth]` — used by the spec | -| `src/environments/environment.test.ts` | Full `auth:` block used by specs; does NOT have `showPasswordLogin` (omitting it is fine — `undefined ?? false` → `false`) | -| `config/config.example.yml` | Operator reference — add commented-out `showPasswordLogin` entry in the `auth:` block | - ---- +## Reusable Codebase Patterns -## How `APP_CONFIG` Is Used in This Codebase +### `APP_CONFIG` Injection Token -`APP_CONFIG` is an Angular `InjectionToken` defined in -`src/config/app-config.interface.ts`. Inject it into a component constructor like this: +`APP_CONFIG` is an `InjectionToken` defined in +`src/config/app-config.interface.ts`. Use it to read runtime config in components: ```typescript import { AppConfig, APP_CONFIG } from '../../../config/app-config.interface'; -// Path from src/app/shared/log-in/ → three levels up → src/config/ +// Adjust the relative path depth to match your component's location. constructor( // ...existing params... @@ -93,11 +89,7 @@ constructor( ) {} ``` -Existing examples to reference: -- `src/app/community-list-page/community-list-service.ts` -- `src/app/item-page/edit-item-page/virtual-metadata/virtual-metadata.component.ts` - -In test files, provide it as: +Provide it in tests with: ```typescript import { APP_CONFIG } from '../../../config/app-config.interface'; import { environment } from '../../../environments/environment'; @@ -105,82 +97,53 @@ import { environment } from '../../../environments/environment'; { provide: APP_CONFIG, useValue: environment } ``` -`environment` (resolves to `environment.test.ts` during tests) has a full `auth:` block -but no `showPasswordLogin` key. That is fine — the component reads it as -`appConfig.auth?.showPasswordLogin ?? false`, so `undefined ?? false` → `false`. -No change to `environment.test.ts` is required. - ---- - -## Spec File Notes - -`src/app/shared/log-in/log-in.component.spec.ts` currently has: - -```typescript -it('should render a log-in container component for each auth method available', () => { - const loginContainers = fixture.debugElement.queryAll(By.css('ds-log-in-container')); - expect(loginContainers.length).toBe(2); // ← must change to toBe(1) -}); -``` - -`authMethodsMock` contains `[password, shibboleth]`. With `showPasswordLogin: false` (default), -only `shibboleth` renders → `toBe(1)`. The `APP_CONFIG` token must also be added to -`providers` in `TestBed.configureTestingModule` or the component will fail to construct. +Existing injection examples to reference: +- `src/app/community-list-page/community-list-service.ts` +- `src/app/item-page/edit-item-page/virtual-metadata/virtual-metadata.component.ts` ---- +### Adding a New Config Flag -## `config/config.example.yml` — Operator Documentation +1. Add `myFlag?: boolean` to the relevant interface in `src/config/`. +2. Set the default value in `src/config/default-app-config.ts`. +3. Document it as a commented-out entry in `config/config.example.yml`. +4. Read it in the component via `this.appConfig.section?.myFlag ?? defaultValue`. -The `auth:` block in `config/config.example.yml` (around line 91–102) must have a -commented-out `showPasswordLogin` entry added so operators know the option exists: +The config system merges `DSPACE_*` environment variables at startup — +`DSPACE_AUTH_MYFLAG=true` maps to `auth.myFlag`. The env-var override works even +when the default is `false` because `isNotEmpty(false) === true`. -```yaml -# When true, show the username/password login form and hide OIDC (exclusive toggle). -# Set to true only on environments where DSpace OIDC is disabled (e.g. demo). -# Default: false (OIDC button shown, password form hidden — production/workshop behaviour). -# Can also be set via environment variable: DSPACE_AUTH_SHOWPASSWORDLOGIN=true -# showPasswordLogin: false -``` +### Angular Testing with `CUSTOM_ELEMENTS_SCHEMA` ---- +Many specs declare `CUSTOM_ELEMENTS_SCHEMA` to avoid registering every child component. +Key consequences: -## Config Override for Demo (Out-of-Band) +- `By.css('ds-some-component')` returns elements, but `componentInstance` and + `properties['input']` are **not** available — Angular never instantiates the class. +- `By.directive(SomeComponent)` **does** work when `SomeComponent` is declared (e.g. + via `SharedModule`) — it finds real instantiated component objects. +- To inspect which auth method rendered in `LogInComponent`, query for the inner method + component that `ngComponentOutlet` created: + ```typescript + By.directive(LogInPasswordComponent) // password path + By.directive(LogInExternalProviderComponent) // OIDC/shibboleth path + ``` -The demo Kubernetes frontend Deployment/ConfigMap (in `mlibrary/deepblue-documents-kube`, -**not** this repo) must set: +### `TestBed.overrideProvider()` Limitation -``` -DSPACE_AUTH_SHOWPASSWORDLOGIN=true -``` +`TestBed.overrideProvider()` throws *"Cannot override provider when the test module has +already been instantiated"* when called inside an `it()` or `beforeEach()` that runs +after `TestBed.createComponent()`. Calling `fixture.destroy()` first does **not** reset +module state. -This task is tracked in `TODO.md` item 9. It is performed separately from this repo's code changes. +Alternatives: +- Set the component property directly: `component.myFlag = true; fixture.detectChanges()`. +- Put `overrideProvider()` inside `beforeEach()` **before** `compileComponents()`, in a + nested `describe` block that has its own `TestBed.configureTestingModule` call. --- -## Coding Guidelines - -1. **Read `TODO.md` first.** Work tasks in the listed order (they have dependencies). -2. **After completing and committing each task**, update `TODO.md` (mark done / remove) and - add the task to `DONE.md`. -3. **Do not change any file not listed in `TODO.md`** unless fixing a compile/lint error - directly caused by your changes. -4. **After editing each TypeScript file**, run `get_errors` on that file and fix any issues - before moving on. -5. **Run the spec** after updating the spec file: - `npx ng test --include='**/log-in/log-in.component.spec.ts' --watch=false --configuration test --browsers=ChromeHeadless` -6. **Commit message convention:** - `feat: config-driven showPasswordLogin exclusive toggle (DEEPBLUE-466)` -7. The `config/config.yml` in this repo is **not** the demo config — do not modify it. - The demo override happens via environment variable in the Kubernetes repo. -8. Do not remove the ` + +--- + +## Chosen Solution + + + +--- + +## Key Files + +| File | Change needed | +|---|---| +| `path/to/file.ts` | Description of change | + +--- + +## Exact Code Changes + + + +### `path/to/file.ts` + +**Before:** +```typescript +// existing code +``` + +**After:** +```typescript +// new code +``` + +--- + +## Config / Environment Variables + + + +| Flag | Default | Env-var override | Notes | +|---|---|---|---| +| `section.flagName` | `false` | `DSPACE_SECTION_FLAGNAME=true` | Description | + +--- + +## Truth Table + + + +| Flag value | Behaviour | +|---|---| +| `false` (default) | … | +| `true` | … | + +--- + +## Verification Steps + +1. Build and start the app locally. +2. … +3. Confirm expected behaviour. + +--- + +## Out-of-Band Tasks + + + +- [ ] Set `ENV_VAR=value` in Kubernetes ConfigMap for `` (`mlibrary/deepblue-documents-kube`). + From 78bbfe78ada36af9321c08f3bd6f880331c32efd Mon Sep 17 00:00:00 2001 From: Greg Kostin Date: Wed, 29 Apr 2026 16:19:58 -0400 Subject: [PATCH 3/4] docs: mark Task A complete in TODO.md --- TODO.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/TODO.md b/TODO.md index 9a45a4f82af..51e1a5db5a3 100644 --- a/TODO.md +++ b/TODO.md @@ -42,7 +42,7 @@ in parallel, and move plan documents into a dedicated `plans/` directory. agent can copy when starting a new feature, with sections for: Problem, Chosen Solution, Key Files, Truth Table / Example Config, Verification Steps. -- [ ] **A6. Commit all changes** on `clean-up` and open a PR against `umich`. +- [x] **A6. Commit all changes** on `clean-up` and open a PR against `umich`. --- From 3d03f41465acbe17c3f2c6efffbda06c004f3afe Mon Sep 17 00:00:00 2001 From: Greg Kostin Date: Wed, 29 Apr 2026 16:24:04 -0400 Subject: [PATCH 4/4] docs: update PULL_REQUEST.md for clean-up PR --- PULL_REQUEST.md | 136 +++++++++++++++--------------------------------- 1 file changed, 41 insertions(+), 95 deletions(-) diff --git a/PULL_REQUEST.md b/PULL_REQUEST.md index b9fe53b8c0b..708ca96c00a 100644 --- a/PULL_REQUEST.md +++ b/PULL_REQUEST.md @@ -1,108 +1,54 @@ -# DEEPBLUE-466 — Config-driven password login toggle for demo environment +# clean-up — Generalize coding-agent markdown files ## Summary -Restores the password login form on the **demo** environment without changing -production or workshop behaviour. +Converts the `show-password-login`-specific agent-guidance files into +repo-wide, feature-agnostic tooling that can support multiple plans and +work items running in parallel. -A new `showPasswordLogin` config flag (default `false`) acts as an **exclusive toggle**: - -| `showPasswordLogin` | Login UI | Environment | -|---|---|---| -| `false` (default) | OIDC button only | production, workshop — **no change** | -| `true` | Password form only | demo | - ---- - -## Background - -Deep Blue Documents has three Kubernetes environments: production, workshop, and demo. - -**Demo** uses a two-layer auth model: -1. `oauth2-proxy` gate requires U-M WebLogin (Shibboleth OIDC) — restricts access to U-M people. -2. Inside the gate, users log in to DSpace with **username/password** to switch test personas. - -DSpace's internal OIDC is intentionally disabled for demo so testers aren't auto-logged-in -as their own umich.edu identity. An existing UM customisation (`*ngIf="authMethod.authMethodType !== 'password'"`) removed the password form from the UI — which was fine when OIDC was the -active method but leaves a completely **blank login page** now that OIDC is disabled. +No source code is changed. All commits are documentation only. --- ## Changes -### `src/config/auth-config.interfaces.ts` -Added `showPasswordLogin?: boolean` to `AuthConfig`. - -### `src/config/default-app-config.ts` -Defaulted `showPasswordLogin: false` — all environments preserve current behaviour -unless they explicitly opt in. - -### `src/app/shared/log-in/log-in.component.ts` -Injected `APP_CONFIG`; reads flag into `public showPasswordLogin: boolean`. - -### `src/app/shared/log-in/log-in.component.html` -Replaced: -```html -*ngIf="authMethod.authMethodType !== 'password'" -``` -with the exclusive toggle: -```html -*ngIf="showPasswordLogin === (authMethod.authMethodType === 'password')" -``` - -### `src/app/shared/log-in/log-in.component.spec.ts` -Provided `APP_CONFIG` token in `TestBed`; updated `toBe(2)` → `toBe(1)`. - -### `config/config.example.yml` -Documented the new flag (commented out) in the `auth:` block for operators. - ---- - -## Configuration - -Enable for demo via frontend Kubernetes ConfigMap environment variable: -``` -DSPACE_AUTH_SHOWPASSWORDLOGIN=true -``` - -The DSpace Angular config system translates this to a proper boolean `true` -via `getBooleanFromString` (verified). Production and workshop must NOT set -this variable — omitting it is sufficient. - -> ⚠️ **This PR does not touch the Kubernetes config.** Setting the env-var -> in `mlibrary/deepblue-documents-kube` is a separate follow-up step. - ---- - -## Also fixed in this PR - -The `umich` base branch had 14 TypeScript compilation errors in 9 spec files -that prevented the test suite from starting at all. Fixed as part of this PR: - -- `environment.test.ts` — missing `serverLocation` property required by `BuildConfig` -- `item-withdraw.component.spec.ts` / `item-reinstate.component.spec.ts` — `setWithDrawn` now requires a `reason` argument (TS2554) -- 6 bitstream-format spec files — `BitstreamFormatSupportLevel` enum values renamed: `Unknown → AS_IS_UNKNOWN`, `Known → AS_IS_KNOWN`, `Supported → HIGHEST_LEVEL` +### `PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md` → `plans/PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md` +Moved the plan document into a new `plans/` directory so all per-feature +design documents live in one place. Updated the file's own header to reflect +its new path and merged status. + +### `plans/PLAN_TEMPLATE.md` *(new)* +Blank plan template for future features. Sections: Problem, Chosen Solution, +Key Files, Exact Code Changes, Config / Environment Variables, Truth Table, +Verification Steps, Out-of-Band Tasks. + +### `AGENTS.md` +Rewritten as repo-wide coding-agent guidelines. Removed all +`show-password-login`-specific content. Now contains: +- Repository overview (framework, language, conventions). +- How to find work (`TODO.md` → `plans/` document). +- Six general coding guidelines (non-interactive CLI flags, `get_errors` + after every TS edit, commit message convention, `ng test` flags, etc.). +- Reusable codebase patterns: `APP_CONFIG` injection, adding a config flag, + `CUSTOM_ELEMENTS_SCHEMA` behaviour, `TestBed.overrideProvider()` limitation. +- Plans index table (one row per feature). + +### `TODO.md` +Rewritten to support multiple parallel work items. Each work item is a +top-level heading with its own subtask checklist and a reference to the +relevant plan document. + +### `DONE.md` +Rewritten as a general feature archive. Each completed feature is a +top-level heading with its plan reference, branch, PR number, and a +concise summary of what was done. --- -## Testing - -- ✅ Unit tests: `log-in.component.spec.ts` — 2 specs, 0 failures -- ✅ Config flow verified end-to-end: env-var → `buildAppConfig` → `extendEnvironmentWithAppConfig` → `APP_CONFIG` token → component → template -- ✅ `isNotEmpty(false) === true` confirmed — the env-var is **not** silently ignored when the default value is `false` - -See `PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md` for full rationale, config system -deep-dive, on-host debugging commands, and post-deployment verification steps. - ---- - -## Verification after demo deployment - -1. Navigate to `https://demo.deepblue-documents.lib.umich.edu` — gate challenges for U-M WebLogin. -2. After passing the gate, click **Log In**. -3. A username/password form appears (no OIDC button). ✅ -4. Log in as `dbrrds@umich.edu`. Confirm login succeeds. ✅ -5. Log out; log in as the alternate demo persona (for example `tildon@umich.edu`) using credentials retrieved from the approved secret manager / restricted ops runbook referenced in `PLAN_DSPACE_ANGULAR_PASSWORD_LOGIN.md` — confirm persona switching works. ✅ - -For **production/workshop**: login still shows only the OIDC button. No regression. ✅ +## Why +The previous files were written during the `show-password-login` sprint and +were tightly coupled to that feature. As the next feature work starts, a +coding agent that reads `AGENTS.md` / `TODO.md` would be confused by +feature-specific instructions. This PR makes the system self-consistent and +ready for the next task.