ENG-3768: Add access package review model and status

[JadeCara]

Ticket ENG-3768

Description Of Changes

PR 1 of 5 for access packages (production merge plan). Lays the data model foundation — the enum value is inert and the table is empty until subsequent PRs wire up the hook points and business logic.

Code Changes

• Added awaiting_access_review status to PrivacyRequestStatus enum and ACTIVE_REQUEST_STATUSES
• Created AccessPackageReview ORM model (1:1 with PrivacyRequest) for storing redactions and approval metadata
• Added Alembic migration for the new enum value and table
• Added accessPackages feature flag to flags.json
• Added new status to all exhaustive frontend status maps (badge, cells, constants, bulk actions, TS enum)

Steps to Confirm

1. Run the migration and verify the table and enum value exist:

   -- after alembic upgrade head
   SELECT column_name, data_type, is_nullable
   FROM information_schema.columns
   WHERE table_name = 'access_package_review'
   ORDER BY ordinal_position;
   
   -- confirm enum value was added
   SELECT enumlabel FROM pg_enum
   WHERE enumtypid = 'privacyrequeststatus'::regtype
   AND enumlabel = 'awaiting_access_review';

2. Verify downgrade drops the table cleanly:

   -- after alembic downgrade -1
   SELECT EXISTS (
     SELECT FROM information_schema.tables
     WHERE table_name = 'access_package_review'
   );
   -- should return false

3. Submit a privacy request through the Admin UI and confirm it completes normally — the new status and table should have zero impact on existing flows

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
  • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • All UX related changes have been reviewed by a designer
  • No UX review needed
• Followup issues:
  • Followup issues created
  • No followup issues
• Database migrations:
  • Ensure that your downrev is up to date with the latest revision on main
  • Ensure that your downgrade() migration is correct and works
    • If a downgrade migration is not possible for this change, please call this out in the PR description!
  • No migrations
• Documentation:
  • Documentation complete, PR opened in fidesdocs
  • Documentation issue created in fidesdocs
  • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • No documentation updates required

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored	Preview	May 19, 2026 6:44pm
fides-privacy-center	Ignored		May 19, 2026 6:44pm

[github-actions]

Title	Lines	Statements	Branches	Functions
admin-ui		6.96% (3187/45779)	6.35% (1683/26499)	4.83% (655/13553)
fides-js		79.17% (1977/2497)	66.25% (1249/1885)	73.31% (349/476)
privacy-center		82.53% (364/441)	79.74% (189/237)	74.07% (60/81)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.65%. Comparing base (c86fca8) to head (43ba4a9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8183   +/-   ##
=======================================
  Coverage   85.64%   85.65%           
=======================================
  Files         662      663    +1     
  Lines       42978    42995   +17     
  Branches     5030     5030           
=======================================
+ Hits        36809    36826   +17     
  Misses       5063     5063           
  Partials     1106     1106           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JadeCara]

/code-review

[claude]

Code Review — ENG-3768: Add access package review model and status

This is a clean, well-scoped PR1-of-5 that lays the data model foundation without exposing any user-facing behaviour yet. The migration, ORM model, schema enum, and frontend status maps all hang together correctly. A few things worth addressing before or alongside the follow-up PRs:

Notable findings

1. Missing ORM relationship() (inline comment on access_package_review.py)
The model defines the FK column but no SQLAlchemy relationship. Every subsequent PR that reads/writes via the ORM will need it, and retrofitting it then means also touching PrivacyRequest for back_populates. Easier to define it now while the context is fresh.

2. ACTIONED_REQUEST_STATUSES gap in duplicate detection (inline comment on duplication_detection.py)
awaiting_access_review is not in ACTIONED_REQUEST_STATUSES. Once the transition hook is wired up, a second identical submission could slip through duplicate detection rather than being blocked. pending_external (a comparable "actively waiting" state) is included — awaiting_access_review should be too. Track this as a known gap even if fixed in a follow-up.

3. ALTER TYPE … ADD VALUE is not transactional in PostgreSQL (inline comment on migration)
If the CREATE TABLE step fails after the enum DDL succeeds, a rollback won't undo the enum change. The IF NOT EXISTS guard handles clean reruns, but a partial failure leaves the schema in an inconsistent state. Worth checking whether the codebase's other enum migrations use a COMMIT-before-CREATE TABLE pattern and matching it.

4. created_at / updated_at nullable in migration (inline comment)
Both columns are nullable=True in the migration DDL. Minor, but adding nullable=False (with server_default still set) would match what other tables in this repo do and give an extra DB-level safety net.

What looks good

• Unique index on privacy_request_id correctly enforces the 1:1 relationship at the DB level.
• MutableDict.as_mutable(JSONB) on redactions is the right pattern for change-tracking.
• Downgrade correctly drops the table and documents why the enum value stays (PostgreSQL limitation).
• awaiting_access_review correctly included in ACTIVE_REQUEST_STATUSES and all frontend exhaustive maps.
• Feature flag test: false is appropriate for an inert status with no wired-up logic yet.
• @declared_attr __tablename__ pattern matches the Base class convention used throughout the codebase.

---

🔬 Codegraph: connected (49962 nodes)

---

💡 Write /code-review in a comment to re-run this review.

[rayharnett]

Code Review: ENG-3768 — Add access package review model and status

PR #8183 · ENG-3768/access-package-model-status → main
Author: @rayharnett · Status: Open

---

Summary

PR 1 of 5 for the access packages feature. It lays the data model foundation: a new awaiting_access_review enum value, a new AccessPackageReview ORM model/table, an Alembic migration, frontend status mappings, and a feature flag. The PR is intentionally inert — no business logic is wired up yet. This is the right approach for a foundation PR.

---

🚨 Critical Issues (Must Fix)

None.

---

⚠️ Warnings (Should Fix)

1. Bulk actions for AWAITING_ACCESS_REVIEW are too restrictive

File: clients/admin-ui/src/features/privacy-requests/helpers.ts:56

[PrivacyRequestStatus.AWAITING_ACCESS_REVIEW]: [BulkActionType.DELETE],

This status currently only exposes the DELETE bulk action. When the subsequent PRs wire up the review workflow, this should almost certainly include APPROVE and DENY as well. Since this is PR 1 of 5 and the PR description says the enum is "inert," this is acceptable as a placeholder. However, I'd recommend adding a // TODO: ENG-3768 - add APPROVE, DENY when review workflow is wired up comment so the next PR author doesn't miss it.

Severity: Low — it's a known incremental PR, but the omission is easy to forget.

2. Migration downgrade loses audit trail

File: src/fides/api/alembic/migrations/versions/xx_2026_05_14_1649_1e07732ff193_add_access_package_review_table_and_.py:97-99

op.execute(
    "DELETE FROM auditlog WHERE action IN "
    "('access_package_approved', 'access_package_redacted')"
)

The downgrade silently deletes audit log entries for the new access package actions. This is a data loss concern if anyone runs downgrade in production or staging. Consider adding a comment explaining why this is acceptable (e.g., "these actions don't exist in the down-rev enum, so their audit entries would be orphaned") and/or flagging it in the PR description's "Steps to Confirm."

Severity: Medium — unlikely to be hit in practice (this is a dev migration), but audit trail loss is always worth noting.

3. getButtonVisibility doesn't cover AWAITING_ACCESS_REVIEW

File: clients/admin-ui/src/features/privacy-requests/helpers.ts:90-99

export const getButtonVisibility = (
  status: PrivacyRequestStatus,
): ButtonVisibility => {
  return {
    approve:
      status === PrivacyRequestStatus.PENDING ||
      status === PrivacyRequestStatus.DUPLICATE,
    deny:
      status === PrivacyRequestStatus.PENDING ||
      status === PrivacyRequestStatus.DUPLICATE,
    finalize: status === PrivacyRequestStatus.REQUIRES_MANUAL_FINALIZATION,
    delete: true,
  };
};

This function uses a single return object for all statuses rather than a status-specific map. AWAITING_ACCESS_REVIEW gets the default { approve: false, deny: false, finalize: false, delete: true }. This is consistent with the bulk actions mapping (see point #1), but the same TODO comment recommendation applies.

---

💡 Suggestions (Nits / Improvements)

4. Add a comment to the new table in the migration

File: src/fides/api/alembic/migrations/versions/...py:47

op.create_table(
    "access_package_review",
    ...
)

Consider adding sa.CommentedRegion or a post-creation op.execute("COMMENT ON TABLE access_package_review IS '...') to document the table's purpose for future DBA/engineer readers. The ORM docstring is great, but table-level comments survive model renames.

5. Consider adding a __table_args__ with Comment for the ORM model

File: src/fides/api/models/privacy_request/access_package_review.py

The docstring is excellent, but it lives only in Python. If the codebase convention supports it, add a SQLAlchemy __table_args__ = {"comment": "..."} so the comment persists in the database schema.

6. .fides/db_dataset.yml — all columns marked system.operations

File: .fides/db_dataset.yml:24-42

Every column in the new table is marked data_categories: [system.operations]. The redactions column likely contains PII (user data that was redacted). Consider whether it should have a more specific data category (e.g., a custom category or at least [system.operations, user.data]) to ensure it's covered by data classification pipelines.

7. Verify the generated types file is intentional

File: clients/admin-ui/src/types/api/models/PrivacyRequestStatus.ts

The new status was added to this file. Verify this is a hand-maintained file (not auto-generated from an OpenAPI spec). If it is auto-generated, the change needs to be in the OpenAPI spec instead. If it's hand-maintained, consider adding a comment like // Updated manually — sync with PrivacyRequestStatus in schemas/privacy_request.py.

---

✅ Good Practices

1. Inert first PR — The PR description clearly states the enum is inert and the table is empty until subsequent PRs. This is the right incremental approach for a multi-PR feature.

2. Excellent docstring — The AccessPackageReview model's docstring clearly documents the row lifecycle (creation, updates, deletion). This will save future engineers significant context-gathering time.

3. Safe enum migration — Using ADD VALUE IF NOT EXISTS is the right pattern for idempotent enum migrations. It handles the case where the migration has been partially run before.

4. Complete frontend coverage — All five frontend status maps were updated consistently: badge colors, cells, constants, bulk actions, and the TS enum. No dead code paths or missing mappings.

5. Feature flag — The accessPackages flag is set to development: true, test: false, production: false. This is appropriate for a foundation PR that introduces no runtime behavior.

6. Test coverage — The new status is asserted to be in ACTIVE_REQUEST_STATUSES. This is minimal but sufficient for a foundation PR.

7. Changelog entry — Properly formatted with the db-migration label. Clear description.

---

Overall Assessment

🟢 Approve with minor suggestions. This is a clean, well-structured foundation PR. The changes are minimal, focused, and follow existing codebase patterns. The main thing to watch is ensuring the subsequent PRs remember to wire up the bulk actions and button visibility for this status (see warnings #1 and #3).