refactor privacy declaration upsert to maintain constant IDs

[adamsachs]

Closes #3187

Code Changes

• change privacy declaration upsert logic used by /system APIs to update PrivacyDeclaration records in place when "matched" using data_use and name as matching criteria
• add in BE validation to prevent a request from specifying "duplicate" PrivacyDeclarations based on the above matching criteria
  • this validation is now important on the BE since our upsert logic now relies on this assumption being true
• add automated test coverage around this use cases
  • there may still be some edge cases we need to cover here...

Steps to Confirm

• i did some testing in the UI with these changes, while checking the DB records, and things generally looked good. but would appreciate others hitting this hard with the API and with the UI
  • NOTE - as of now, if you change the "data use" or the "processing activity" of an existing declaration in the UI, it's going to create a new DB record. that's expected, and we will look to prevent this edge case by making those fields "read only" in the UI after creation.

Pre-Merge Checklist

• All CI Pipelines Succeeded
• Documentation:
  • documentation complete, PR opened in fidesdocs
  • documentation issue created in fidesdocs
• Issue Requirements are Met
• Relevant Follow-Up Issues Created
• Update CHANGELOG.md
• For API changes, the Postman collection has been updated

Description Of Changes

See issue for more context. This is needed to get custom fields working with privacy declarations, i.e. "data use declarations".

[cypress]

Passing run #1674 ↗︎

0

4

0

0

0

Details:

Merge bd63960 into 590b89b...
Project: fides	Commit: 130b9d85d2 ℹ️
Status: Passed	Duration: 01:04 💡
Started: May 1, 2023 1:45 AM	Ended: May 1, 2023 1:47 AM

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[codecov]

Codecov Report

Patch coverage: 88.88% and project coverage change: -0.23 ⚠️

Comparison is base (00ed06d) 87.62% compared to head (bd63960) 87.39%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3188      +/-   ##
==========================================
- Coverage   87.62%   87.39%   -0.23%     
==========================================
  Files         313      313              
  Lines       18178    18240      +62     
  Branches     2347     2365      +18     
==========================================
+ Hits        15928    15941      +13     
- Misses       1823     1867      +44     
- Partials      427      432       +5     

Impacted Files	Coverage Δ
src/fides/api/ctl/routes/system.py	84.05% <88.88%> (-0.32%)	⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pattisdr]

Thanks for clarifying some of my comments here, looks good to me

[adamsachs]

i'm pretty clearly hitting the line that codecov says i'm missing in my test_api.py tests i've added, so not sure why it's complaining. anyway, going to go ahead and merge now - thanks for the speedy review on this @pattisdr!

[adamsachs]

i'm pretty clearly hitting the line that codecov says i'm missing in my test_api.py tests i've added, so not sure why it's complaining. anyway, going to go ahead and merge now - thanks for the speedy review on this @pattisdr!

actually, a quick thought on the codecov "miss" -- maybe because the test code asserts the errored HTTP status codes without actually doing any explicit catching of the HTTPException, it's showing up as a false negative? perhaps that's just some oddity in the way our test API client works and interacts with codecov?

it's a total shot in the dark, and i'm not very concerned (especially with this PR specifically), but @ThomasLaPiana maybe something you have thoughts on, since i've noticed some false negative codecov misses recently. anyway, still merging here!

[ThomasLaPiana]

i'm pretty clearly hitting the line that codecov says i'm missing in my test_api.py tests i've added, so not sure why it's complaining. anyway, going to go ahead and merge now - thanks for the speedy review on this @pattisdr!

actually, a quick thought on the codecov "miss" -- maybe because the test code asserts the errored HTTP status codes without actually doing any explicit catching of the HTTPException, it's showing up as a false negative? perhaps that's just some oddity in the way our test API client works and interacts with codecov?

it's a total shot in the dark, and i'm not very concerned (especially with this PR specifically), but @ThomasLaPiana maybe something you have thoughts on, since i've noticed some false negative codecov misses recently. anyway, still merging here!

I think you're right on the money there! It is probably wanting to test the code after the catch? But anyway, seems fine to me :)

These are merely guidelines, so we're still capable of making the decision that it's "good enough"!