Skip to content

monitor/classify staged resource additions that are "alongside" already-classified, but not-promoted resources#6179

Merged
gilluminate merged 5 commits intomainfrom
gill/ENG-126/cannot-monitor-classify-staged
May 30, 2025
Merged

monitor/classify staged resource additions that are "alongside" already-classified, but not-promoted resources#6179
gilluminate merged 5 commits intomainfrom
gill/ENG-126/cannot-monitor-classify-staged

Conversation

@gilluminate
Copy link
Copy Markdown
Contributor

@gilluminate gilluminate commented May 29, 2025

Closes ENG-126

Description of Changes

Fixed an issue where tables with both existing classified fields and new field discoveries would only show the "Confirm" action instead of "Monitor" in the Data detection view, preventing users from classifying newly discovered fields before promoting them to datasets.

Code Changes

  • Updated DetectionItemActionsCell to identify resources with classification changes and show "Monitor" button instead of "Confirm" for these scenarios
  • Added hasClassificationChanges logic to check for CLASSIFICATION_ADDITION or CLASSIFICATION_UPDATE in child diff statuses
  • Modified showStartMonitoringAction to include resources with classification changes
  • Modified showConfirmAction to exclude resources with classification changes
  • Added Cypress test coverage for the Monitor action on resources with classification changes
  • Fixed React error messages relating to attributes in MonitorOffIcon component

Steps to Confirm

  1. Run detection on a schema with a table (see: Confluence docs)
  2. Set up a discovery monitor and run detection on a schema with tables
  3. Click "Monitor" on a table to send it to classification
  4. After classification completes, add new fields to the table in the upstream datasource (see the Confluence docs above, toward the bottom of the page.)
  5. Run detection again to discover the new fields
  6. Verify that the table now shows a "Monitor" button in the Data detection view instead of "Confirm"
  7. Click "Monitor" to classify the new fields before promoting the entire table

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
  • 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

@gilluminate gilluminate requested a review from jpople May 29, 2025 17:10
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 30, 2025 3:43pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-privacy-center ⬜️ Ignored (Inspect) May 30, 2025 3:43pm

Comment on lines +19 to +26
cy.intercept("GET", "/api/v1/config*", {
body: {
detection_discovery: {
monitor_celery_tasks_enabled: true,
website_monitor_enabled: true,
},
},
});
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.

lots of errors in Cypress because this was missing

Comment on lines +71 to +73
"child_diff_statuses": {
"classification_addition": 1
},
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.

This is the only difference in this mock compared to the one above it

Comment on lines +9 to +10
fillRule="evenodd"
clipRule="evenodd"
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.

resolving React errors relating to these props

@gilluminate gilluminate force-pushed the gill/ENG-126/cannot-monitor-classify-staged branch from 6482f6d to 34be146 Compare May 29, 2025 17:14
@gilluminate gilluminate requested a review from adamsachs May 29, 2025 17:47
@adamsachs
Copy link
Copy Markdown
Contributor

tested manually using the steps to repro on the ticket, and this is looking good! screenshot below shows the Monitor button on a table that is in the "mixed state" (some discovery/classification results, some new detection additions) showing the Monitor button while in the Data detection view. Clicking on that Monitor button correctly sent the net-new field that was an addition into classification, and eventually to the Data discovery view 👍

image

@gilluminate gilluminate merged commit c765945 into main May 30, 2025
20 checks passed
@gilluminate gilluminate deleted the gill/ENG-126/cannot-monitor-classify-staged branch May 30, 2025 16:34
@cypress
Copy link
Copy Markdown

cypress Bot commented May 30, 2025

fides    Run #12949

Run Properties:  status check passed Passed #12949  •  git commit c76594585f: monitor/classify staged resource `addition`s that are "alongside" already-classi...
Project fides
Branch Review main
Run status status check passed Passed #12949
Run duration 00m 53s
Commit git commit c76594585f: monitor/classify staged resource `addition`s that are "alongside" already-classi...
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
View all changes introduced in this branch ↗︎

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.

3 participants