API for messaging-related app config settings

[adamsachs]

Closes #2456

This enables the PATCH /config endpoint to support the following messaging-related config properties to be specified:

  "notifications": {
    "send_request_completion_notification": true,
    "send_request_receipt_notification": true,
    "send_request_review_notification": true,
    "notification_service_type": "string"
  },
  "execution": {
    "subject_identity_verification_required": true
  }
}

in addition to the

{
  "storage": {
    "active_default_storage_type": "s3"
  }

property that was already supported via this API.

The application now leverages the values set for these messaging-related properties, too, using a new ConfigProxy construct.

Any API-set values for these config properties will take precedence over the values set via "traditional" config mechanisms, i.e. fides.toml or ENV var.

https://github.com/ethyca/fidesdocs/issues/56 has been created to track doc updates related to this and other functionality updates.

Code Changes

• updates PATCH /config endpoint to allow for updating messaging-related settings (i.e. notifications settings and execution.subject_identity_verification_required setting)
• creates a ConfigProxy construct that allows for config property resolution, which is required now that we are allowing API overwrites of config properties set via "traditional" config means.
  • update all references to the messaging-related config settings in application code to leverage the new ConfigProxy construct, so that API updates to the config properties are actually used in normal application runtime
  • For now, ConfigProxy only supports the config properties needed for storage and messaging use cases. Going forward, we may look to expand its use for more, or even all of our config properties. It will likely need refactors to support that.
• updates tests accordingly

Steps to Confirm

• nox -s dev -- ui
• use openAPI UI to set the messaging config settings via API, rather than updating their values in the fides.toml or via env var
• ensure sure messaging/notifications work as expected

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

-- Note --

I think I've got pretty good unit test coverage, and I've tested the functionality relatively thoroughly with a live app locally. But we could still used some improved end-to-end automated test coverage for the new functionality. What I am thinking is just leveraging some of our existing privacy request notification tests that use config property changes, but instead of updating the "traditional" config objects, have fixtures that update the API-set config objects instead.

[adamsachs]

test failures look unrelated? some are just cancelled...?

[sanders41]

test failures look unrelated? some are just cancelled...?

I agree, unrelated. I think they will resolve on a re-run.

[codecov]

Codecov Report

Base: 86.23% // Head: 86.33% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (48f0394) compared to base (53e13d5).
Patch coverage: 97.02% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2551      +/-   ##
==========================================
+ Coverage   86.23%   86.33%   +0.09%     
==========================================
  Files         289      290       +1     
  Lines       15796    15921     +125     
  Branches     1986     1999      +13     
==========================================
+ Hits        13622    13745     +123     
- Misses       1786     1788       +2     
  Partials      388      388              

Impacted Files	Coverage Δ
...api/ops/service/privacy_request/request_service.py	91.93% <ø> (ø)
src/fides/core/config/utils.py	100.00% <ø> (ø)
src/fides/api/main.py	79.01% <55.55%> (-1.38%)	⬇️
.../ops/api/v1/endpoints/privacy_request_endpoints.py	89.14% <85.71%> (+0.02%)	⬆️
src/fides/api/ops/api/deps.py	96.66% <100.00%> (+0.51%)	⬆️
...fides/api/ops/api/v1/endpoints/config_endpoints.py	100.00% <100.00%> (ø)
.../ops/api/v1/endpoints/consent_request_endpoints.py	87.41% <100.00%> (+0.08%)	⬆️
...rc/fides/api/ops/api/v1/endpoints/drp_endpoints.py	92.10% <100.00%> (+0.10%)	⬆️
...pi/v1/endpoints/identity_verification_endpoints.py	100.00% <100.00%> (ø)
src/fides/api/ops/models/application_config.py	100.00% <100.00%> (+5.88%)	⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[pattisdr]

Reviewing now for the purposes of thinking about extending this in fidesplus

[ThomasLaPiana]

@adamsachs I'm totally just rubber ducking here so forgive the potentially dumb question, but did you try this using a CONFIG object that is shared everywhere as opposed to our current methodology? I need to figure out a way to test it, but it would look something like:

In [1]: from fides.core.config import get_config
Loaded config from: .fides/fides.toml

In [2]: CONFIG = get_config()

In [3]: CONFIG.dev_mode
Out[3]: False

In [4]: CONFIG.dev_mode = True

In [5]: CONFIG = get_config()

In [6]: CONFIG.dev_mode
Out[6]: True

CONFIG is referring to the same object so it preserves settings changes. Although given the parent/child fides instance stuff maybe it needs to be in a database so that it is accessible from outside of that single python application?

[sanders41]

but did you try this using a CONFIG object that is shared everywhere as opposed to our current methodology?

We would need to be careful here and think this through. If using either a multi-worker setup or auto-scaling with something like kubernetes each worker or pod would have it's own instance of CONFIG. So updating CONFIG.dev_mode on one worker/pod would not update it everywhere. Historically this hasn't been an issue because things are set at startup and static, but with this change (and others like it that are coming) the config values are becoming mutatable at runtime.

[cypress]

Passing run #264 ↗︎

0	3	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge 48f0394 into 871ccf6...
Project: fides	Commit: c0659106e3 ℹ️
Status: Passed	Duration: 00:42 💡
Started: Feb 22, 2023 12:21 AM	Ended: Feb 22, 2023 12:22 AM

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

[adamsachs]

@pattisdr i think i've addressed all your comments in my most recent comments, if you could take another quick look to confirm things look good from your end that'd be great!

otherwise, i think this is just about ready to go, i've gotten main merged in.

@ThomasLaPiana for

but did you try this using a CONFIG object that is shared everywhere as opposed to our current methodology?

i'm not sure i'm totally following the suggestion - maybe we can sync up quickly offline tomorrow?

i do want to try and get this PR across the line as it's holding up the system config work. for what it's worth, this was just meant as an initial iteration on the idea of a config proxy and providing a mechanism for mutable state in our config. i am expecting we'll need to refine some of this as we go, so unless we feel this is going down a path that will be hard to work from moving forward, then i think we should get this in place as a first iteration.

[adamsachs]

the only cocdecov miss here is for some exception handling within main.py, but it's pretty difficult for us to cover main.py execution itself in automated tests, and we're missing a lot of coverage there already -- so i don't think that should hold this up. curious for other thoughts.

[sanders41]

the only cocdecov miss here is for some exception handling within main.py, but it's pretty difficult for us to cover main.py execution itself in automated tests, and we're missing a lot of coverage there already -- so i don't think that should hold this up. curious for other thoughts.

Overall coverage stays the same, and that exception catches all exceptions not specific ones so I'd be ok with missing it. We can follow up later and look at main.py as a whole.

[pattisdr]

Starting second pass shortly...

[pattisdr]

Thanks for this round of changes @adamsachs, looking good, just one small bug found -

[pattisdr]

well done 🏆