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

fix messaging 'test connection' workflow on initial save #2751

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

adamsachs
Copy link
Contributor

Helps close #2455

specifically meant to resolve issue reported in this comment

Code Changes

  • i noticed that the messagingDetails param was undefined in the case where the messaging config form loads from "scratch", i.e. there is no messaging default already present for the given service type. this is because the getMessagingConfigurationDetails() query against the backend gets an (expected) 404 in these cases
  • since the TestMessagingProviderConnectionButton constructor only really requires the service_type from the messagingDetails, and we know the service_type already given the context of the form, i just define a dummy messagingDetails object with the service_type inferred from the context of the form, in the cases where we haven't successfully retrieved the messagingDetails from the API

Steps to Confirm

Without fix:

  1. start with a fresh app db (nox -s teardown -- volumes)
  2. nox -s dev -- ui
  3. go to configure your messaging provider (http://localhost:3000/privacy-requests/configure), click on e.g. Mailgun Email, fill in a domain and API key (can be whatever value you want)
  4. notice this error: image (8)

With fix:
repeat steps 1-3, notice that the "test connection" field successfully renders and you are able to successfully submit a messaging connection test (assuming your messaging config details are valid).

Pre-Merge Checklist

Description Of Changes

  • i have no idea if this is a "proper" fix - i have no experience with our FE :) so please take it with a grain of salt and open to any other suggestions! i tried just changing the TestMessagingProviderConnectionButton constructor to just take a service_type: string (since that's all it really needs) but i couldn't get that to play nice in the forms on some quick tries - but i also don't know what i'm doing!

@cypress
Copy link

cypress bot commented Mar 6, 2023

Passing run #643 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge 5cbec21 into 98663d3...
Project: fides Commit: 52fa55e37e ℹ️
Status: Passed Duration: 00:40 💡
Started: Mar 6, 2023 3:08 PM Ended: Mar 6, 2023 3:08 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 6, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (98663d3) 86.71% compared to head (5cbec21) 86.71%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2751   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files         291      291           
  Lines       16315    16315           
  Branches     2067     2067           
=======================================
  Hits        14147    14147           
  Misses       1781     1781           
  Partials      387      387           

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 marked this pull request as ready for review March 6, 2023 17:08
@adamsachs adamsachs merged commit e5edd4f into main Mar 6, 2023
@adamsachs adamsachs deleted the fix-test-connection-first-save branch March 6, 2023 17:08
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.

Storage and messaging configuration test against BE
2 participants