Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch messaging service type db enum to use lowercase values #2746

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Mar 3, 2023

Closes #2680

Code Changes

  • update MessagingServiceType Enum in python codebase to have lowercased values, update any references in code and in tests
    • switch any .upper() calls that coerced user-input values to instead be .lower(), to maintain backward compatibility and continue to support uppercased user input
  • provide a DB migration to change the enum type in the DB schema, as well as a data migration to change any existing enum values in the DB to lowercased
  • update FE code references to user lowercased enum values rather than uppercased

Steps to Confirm

  • ensure new system config UI still works as expected
  • ensure migration works as expected:
    • run server on main (e.g. nox -s dev -- ui) and create a messaging config (e.g. using the system config UI)
    • teardown but do not delete volumes, i.e. nox -s teardown
    • checkout this branch, run server (e.g. nox -s dev -- ui) and ensure your messaging config is still present and functional (can check using the API - GET /api/v1/messaging/default/{service_type}, or checking the messagingconfig db table directly)

Pre-Merge Checklist

Description Of Changes

  • Caveat: there's a very narrow edge case where if someone in the past few days has used the UI on main to configure a messaging config, the UI will have set the notification.notification_service_type property to an uppercased value, e.g. MAILGUN.
    • you may see some500s on calls to GET /api/v1/messaging/default/active and that's what is used by the messaging config UI to load the "current" messaging provider. so the messaging config UI will show up as if you have no messaging provider configured
    • if this happens, then just click on the messaging provider you had configured in the messaging config UI and its details will be loaded. you should be good to go now.

@adamsachs adamsachs force-pushed the 2680-switch-to-use-lower-cased-values-for-messagingservicetype-enum-in-db branch from 8d340b2 to 248d4b3 Compare March 3, 2023 23:20
@cypress
Copy link

cypress bot commented Mar 3, 2023

Passing run #683 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge a263aeb into dcc7dbf...
Project: fides Commit: 5025bd3bc6 ℹ️
Status: Passed Duration: 00:38 💡
Started: Mar 7, 2023 5:11 PM Ended: Mar 7, 2023 5:11 PM

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

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

Patch coverage: 93.33% and no project coverage change

Comparison is base (dcc7dbf) 86.55% compared to head (a263aeb) 86.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2746   +/-   ##
=======================================
  Coverage   86.55%   86.55%           
=======================================
  Files         291      291           
  Lines       16488    16488           
  Branches     2117     2117           
=======================================
  Hits        14272    14272           
  Misses       1817     1817           
  Partials      399      399           
Impacted Files Coverage Δ
...es/api/ops/api/v1/endpoints/messaging_endpoints.py 93.66% <ø> (ø)
...s/schemas/messaging/messaging_secrets_docs_only.py 100.00% <ø> (ø)
.../ops/service/messaging/message_dispatch_service.py 56.84% <ø> (ø)
...pi/ops/service/messaging/messaging_crud_service.py 100.00% <ø> (ø)
...ides/api/ops/service/connectors/email_connector.py 88.88% <66.66%> (+2.46%) ⬆️
src/fides/api/ops/models/messaging.py 90.78% <100.00%> (ø)
src/fides/api/ops/schemas/application_config.py 100.00% <100.00%> (ø)
src/fides/api/ops/schemas/messaging/messaging.py 97.83% <100.00%> (-1.09%) ⬇️
src/fides/core/config/notification_settings.py 90.00% <100.00%> (ø)
... and 1 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.

@adamsachs adamsachs force-pushed the 2680-switch-to-use-lower-cased-values-for-messagingservicetype-enum-in-db branch from 535ca7b to c336494 Compare March 6, 2023 15:42
@adamsachs adamsachs marked this pull request as ready for review March 6, 2023 16:24
@adamsachs adamsachs self-assigned this Mar 6, 2023
@adamsachs adamsachs force-pushed the 2680-switch-to-use-lower-cased-values-for-messagingservicetype-enum-in-db branch from c0c303a to d3e4cfa Compare March 7, 2023 16:46
@adamsachs adamsachs force-pushed the 2680-switch-to-use-lower-cased-values-for-messagingservicetype-enum-in-db branch from d3e4cfa to a263aeb Compare March 7, 2023 16:58
@adamsachs adamsachs merged commit 681317f into main Mar 7, 2023
@adamsachs adamsachs deleted the 2680-switch-to-use-lower-cased-values-for-messagingservicetype-enum-in-db branch March 7, 2023 17: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.

consider switching to use lower-cased values for MessagingServiceType enum in db
3 participants