Skip to content

ENG-2875: Add questionnaire_tone_prompt to privacy assessment config#7563

Merged
galvana merged 5 commits intomainfrom
ENG-2875-optimize-slack-notifications
Mar 5, 2026
Merged

ENG-2875: Add questionnaire_tone_prompt to privacy assessment config#7563
galvana merged 5 commits intomainfrom
ENG-2875-optimize-slack-notifications

Conversation

@galvana
Copy link
Copy Markdown
Contributor

@galvana galvana commented Mar 4, 2026

Ticket ENG-2875

Description Of Changes

Add a questionnaire_tone_prompt column to the privacy_assessment_config table for configurable questionnaire tone. Move DEFAULT_ASSESSMENT_MODEL and DEFAULT_CHAT_MODEL constants and their accessor classmethods from the fides OSS model to the fidesplus service layer, where the business logic belongs.

Code Changes

  • src/fides/api/models/privacy_assessment_config.py - Add questionnaire_tone_prompt Text column, remove default model constants and accessor classmethods (moved to fidesplus)
  • src/fides/api/alembic/migrations/versions/xx_2026_03_03_1000_ca2c622bad39_questionnaire_tone_prompt.py - Migration to add the new column
  • tests/api/models/test_privacy_assessment_config.py - Removed (tests for deleted classmethods; replacement tests in fidesplus)

Steps to Confirm

  1. Run the migration: alembic upgrade head
  2. Verify questionnaire_tone_prompt column exists on privacy_assessment_config table
  3. Verify downgrade removes the column

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration label to the entry
    • 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

Note: Fidesplus PR with service layer changes depends on this PR.

Summary by CodeRabbit

  • New Features

    • Added support for configuring a custom tone for questionnaire messages in privacy assessment settings.
  • Chores

    • Updated the database schema to store the new questionnaire tone configuration.
  • Documentation

    • Added a changelog entry documenting the new questionnaire tone option.
  • Tests

    • Removed existing unit tests related to previous model defaults and model-selection helpers.

…s to fidesplus

Add a nullable questionnaire_tone_prompt column to privacy_assessment_config
for custom tone configuration. Move DEFAULT_ASSESSMENT_MODEL and DEFAULT_CHAT_MODEL
constants plus their accessor classmethods to fidesplus service layer where they belong.

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

vercel bot commented Mar 4, 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 Mar 5, 2026 6:47pm
fides-privacy-center Ignored Ignored Mar 5, 2026 6:47pm

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ce560078-e3ba-4c37-a239-2da131421e6b

📥 Commits

Reviewing files that changed from the base of the PR and between f60a844 and bf7e2dc.

📒 Files selected for processing (2)
  • .fides/db_dataset.yml
  • src/fides/api/alembic/migrations/versions/xx_2026_03_03_1000_ca2c622bad39_questionnaire_tone_prompt.py

📝 Walkthrough

Walkthrough

This PR adds a nullable questionnaire_tone_prompt Text column to privacy_assessment_config via an Alembic migration, updates the PrivacyAssessmentConfig model to include the new field while removing two default-model constants and their getters, updates dataset YAML, and deletes the related model tests.

Changes

Cohort / File(s) Summary
Changelog & Dataset
changelog/7563-questionnaire-tone-prompt.yaml, .fides/db_dataset.yml
Adds changelog entry and dataset schema entry for new questionnaire_tone_prompt field under privacy_assessment_config with system.operations data category.
Database Migration
src/fides/api/alembic/migrations/versions/xx_2026_03_03_1000_ca2c622bad39_questionnaire_tone_prompt.py
New Alembic migration adding nullable Text column questionnaire_tone_prompt to privacy_assessment_config with corresponding downgrade to drop it.
Model Update
src/fides/api/models/privacy_assessment_config.py
Adds questionnaire_tone_prompt: Text nullable field; removes DEFAULT_ASSESSMENT_MODEL, DEFAULT_CHAT_MODEL, and their get_assessment_model() / get_chat_model() methods.
Tests
tests/api/models/test_privacy_assessment_config.py
Removes the entire test module covering default/override behavior for assessment/chat model getters.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • lucanovera

Poem

🐰 I hopped through schema, nibbling a line,
A prompt for tone now nestles fine,
Old defaults tucked beneath the ground,
New field sprouts softly, without a sound,
Hooray — the database hums in rhyme!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and directly describes the main change: adding questionnaire_tone_prompt to privacy assessment config, matching the primary objective of this PR.
Description check ✅ Passed The PR description follows the required template with all essential sections completed: ticket reference, description of changes, code changes listed, steps to confirm, and pre-merge checklist items addressed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ENG-2875-optimize-slack-notifications

Comment @coderabbitai help to get the list of available commands and usage tips.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@galvana galvana marked this pull request as ready for review March 5, 2026 07:33
@galvana galvana requested a review from a team as a code owner March 5, 2026 07:33
@galvana galvana requested review from vcruces and removed request for a team and vcruces March 5, 2026 07:33
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR adds a questionnaire_tone_prompt column to the privacy_assessment_config table and moves DEFAULT_ASSESSMENT_MODEL/DEFAULT_CHAT_MODEL constants from the OSS model to the fidesplus service layer.

Changes are clean and minimal:

  • Migration adds a nullable Text column with correct down_revision and symmetric upgrade/downgrade
  • Model change is straightforward; no remaining references to removed constants in the OSS codebase
  • Test file deletion is appropriate — removed tests covered only the deleted classmethods
  • Column naming (questionnaire_tone_prompt) is consistent with existing project patterns (e.g., slack_channel_id, assessment_model_override, reassessment_cron)
  • Changelog entry correctly tagged with db-migration label

Safe to merge — changes are minimal, reversible, and well-scoped.

Confidence Score: 5/5

  • This PR is safe to merge — changes are minimal, reversible, and follow existing project patterns.
  • The PR makes straightforward database schema and model changes with no functional issues. The migration is simple (add/drop nullable column), the model change is clean with no dangling references in the OSS codebase, and the deleted test file mirrors the removed functionality. All changes are appropriately scoped and reversible.
  • No files require special attention

Last reviewed commit: f60a844

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/fides/api/models/privacy_assessment_config.py (1)

64-69: Consider updating the class docstring bullets to include tone configuration.

This helps keep the model’s top-level documentation in sync with the new persisted setting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/fides/api/models/privacy_assessment_config.py` around lines 64 - 69,
Update the class docstring for PrivacyAssessmentConfig to document the new
persisted tone setting: add a bullet describing questionnaire_tone_prompt (Text,
nullable) and its behavior (custom tone prompt for questionnaire messages, falls
back to default when null), so the top-level model docs reflect the new
persisted setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/fides/api/models/privacy_assessment_config.py`:
- Around line 64-69: Update the class docstring for PrivacyAssessmentConfig to
document the new persisted tone setting: add a bullet describing
questionnaire_tone_prompt (Text, nullable) and its behavior (custom tone prompt
for questionnaire messages, falls back to default when null), so the top-level
model docs reflect the new persisted setting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 533ac511-c656-4b7f-a53d-18cc474eea76

📥 Commits

Reviewing files that changed from the base of the PR and between 8bb7361 and f60a844.

📒 Files selected for processing (4)
  • changelog/7563-questionnaire-tone-prompt.yaml
  • src/fides/api/alembic/migrations/versions/xx_2026_03_03_1000_ca2c622bad39_questionnaire_tone_prompt.py
  • src/fides/api/models/privacy_assessment_config.py
  • tests/api/models/test_privacy_assessment_config.py
💤 Files with no reviewable changes (1)
  • tests/api/models/test_privacy_assessment_config.py

Add column comment to migration to match model definition and add
data category annotation for the new column in db_dataset.yml.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@lucanovera lucanovera left a comment

Choose a reason for hiding this comment

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

Confirmed with steps to reproduce.

@galvana galvana added this pull request to the merge queue Mar 5, 2026
Merged via the queue into main with commit 9804587 Mar 5, 2026
58 checks passed
@galvana galvana deleted the ENG-2875-optimize-slack-notifications branch March 5, 2026 20:02
mfbrown pushed a commit that referenced this pull request Mar 12, 2026
…7563)

Co-authored-by: Adrian Galvan <galvana@uci.edu>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants