Skip to content

ENG-3118: Add user_assigned_description column to StagedResource#7847

Merged
dsill-ethyca merged 8 commits intomainfrom
ENG-3118/editable-descriptions-idp-monitors
Apr 7, 2026
Merged

ENG-3118: Add user_assigned_description column to StagedResource#7847
dsill-ethyca merged 8 commits intomainfrom
ENG-3118/editable-descriptions-idp-monitors

Conversation

@dsill-ethyca
Copy link
Copy Markdown
Contributor

@dsill-ethyca dsill-ethyca commented Apr 7, 2026

Ticket ENG-3118

Description Of Changes

Adds a nullable user_assigned_description column to the stagedresource table. This enables the frontend to distinguish system-generated descriptions (show sparkle icon) from user-edited descriptions (no sparkle) for IDP monitor resources.

Follows the same pattern as user_assigned_data_uses — nullable column where NULL means system-generated and non-null means user override.

Note: This PR contains only the fides (OSS) model + migration change. The fidesplus PR with schema, service, and test changes will follow and depend on this one.

Code Changes

  • Added user_assigned_description column to StagedResource DB model (core.py)
  • Added alembic migration 6a42f48c23dd to create the column

Steps to Confirm

  1. Run alembic upgrade head — verify user_assigned_description column exists on stagedresource table
  2. Run alembic downgrade -1 — verify column is dropped cleanly
  3. Verify no multiple-head issues: alembic heads returns a single head

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration label to the entry if your change includes a DB migration
    • Add a high-risk label to the entry if your change includes a high-risk change
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • No UX review needed
  • Followup issues:
    • 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
  • Documentation:
    • No documentation updates required

dsill-ethyca and others added 2 commits April 7, 2026 11:22
Adds a nullable `user_assigned_description` column to the `stagedresource`
table. This enables the frontend to distinguish system-generated descriptions
(show sparkle) from user-edited descriptions (no sparkle) for IDP monitor
resources, following the same pattern as `user_assigned_data_uses`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Apr 7, 2026

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

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Apr 7, 2026 8:09pm
fides-privacy-center Ignored Ignored Apr 7, 2026 8:09pm

Request Review

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes fides_db_scan CI failure — new column must be registered
in the dataset file for the DB scan to pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dsill-ethyca dsill-ethyca marked this pull request as ready for review April 7, 2026 16:01
@dsill-ethyca dsill-ethyca requested a review from a team as a code owner April 7, 2026 16:01
@dsill-ethyca dsill-ethyca removed the request for review from a team April 7, 2026 16:02
…label

- Remove redundant `default=None` from column definition to match
  `user_assigned_system_id` pattern
- Add `db-migration` label to changelog entry

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Clean, minimal PR that does exactly what it says. The migration chain is valid, the column definition is consistent with how other user_assigned_* fields are modelled, and the changelog entry correctly carries the db-migration label.

One suggestion worth considering:

The user_assigned_data_uses column has a prose comment explaining its nullable semantics (see lines 581–584 of core.py). This user_assigned_description column carries the same kind of meaningful nullable contract (NULL = system-generated, non-null = user override), so a matching comment would improve long-term maintainability. See the inline note on core.py.

Everything else looks good:

  • down_revision correctly targets d4e5f6a7b8c9 (the password-reset-flow migration, the current head on main)
  • downgrade() is correct — mirrors the upgrade() exactly
  • db_dataset.yml updated to include the new column
  • No schema/API layer changes are expected in this PR per the description; those follow in the dependent fidesplus PR

No blockers — happy to approve once the comment thread is addressed (or acknowledged as intentionally omitted).

default=None,
)
user_assigned_system_id = Column(String, nullable=True, index=True)
user_assigned_description = Column(String, nullable=True)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an explanatory comment matching the pattern established by user_assigned_data_uses above. Since the nullable semantics here carry meaningful business logic (NULL = system-generated description, non-null = user override), a comment would help future readers:

# This field is intentionally nullable to distinguish system-generated descriptions
# (value is None) from user-edited descriptions (non-null value). This enables the
# frontend to show a sparkle icon only for system-generated descriptions.
user_assigned_description = Column(String, nullable=True)

Also a minor nit: the sibling columns user_assigned_data_uses uses default=None explicitly. If you want consistency, you could add default=None here too — though it's not a correctness issue since SQLAlchemy implicitly treats nullable columns as None.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the explanatory comment. Intentionally omitted default=None as it's redundant for nullable String columns — user_assigned_data_uses includes it because of ARRAY-specific empty-vs-null semantics that don't apply here.

from alembic import op

revision = "6a42f48c23dd"
down_revision = "d4e5f6a7b8c9"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: d4e5f6a7b8c9 is the revision ID of xx_2026_04_03_1000_d4e5f6a7b8c9_add_password_reset_flow.py, so the migration chain is correct. The checklist item ✅ holds.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.07%. Comparing base (10d9fc7) to head (8a99821).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7847   +/-   ##
=======================================
  Coverage   85.06%   85.07%           
=======================================
  Files         627      627           
  Lines       40765    40766    +1     
  Branches     4740     4740           
=======================================
+ Hits        34678    34680    +2     
+ Misses       5018     5017    -1     
  Partials     1069     1069           

☔ 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.

Addresses PR review feedback — documents the NULL vs non-null contract
matching the pattern established by user_assigned_data_uses.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dsill-ethyca dsill-ethyca added this pull request to the merge queue Apr 7, 2026
@dsill-ethyca dsill-ethyca removed this pull request from the merge queue due to a manual request Apr 7, 2026
@dsill-ethyca dsill-ethyca added this pull request to the merge queue Apr 7, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 7, 2026
dsill-ethyca and others added 2 commits April 7, 2026 16:02
Update down_revision from d4e5f6a7b8c9 to 22cf8ccaec40 (cloud_infra
migration merged to main), resolving the multiple-heads conflict.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dsill-ethyca dsill-ethyca added this pull request to the merge queue Apr 7, 2026
Merged via the queue into main with commit f98661c Apr 7, 2026
69 checks passed
@dsill-ethyca dsill-ethyca deleted the ENG-3118/editable-descriptions-idp-monitors branch April 7, 2026 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants