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

Ability to delete integrations #5143

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

Kelsey-Ethyca
Copy link
Contributor

@Kelsey-Ethyca Kelsey-Ethyca commented Jul 31, 2024

Closes #PROD-2373

Description Of Changes

Add the ability to delete an integration in the new integration view with a confirmation modal. This uses the existing modal used to deleted integrations on the Connection manager page.

Code Changes

  • update existing integration deletion modal copy
  • update existing integration deletion to show in menu or button

Steps to Confirm

  • confirm you can delete an integration with modal from the system > integration tab
  • confirm you can delete an integration with modal from the connection manager page
  • confirm you can delete an integration with modal from the new integration page
  • confirm the modal matches the designs here.

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • 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
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any 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!

Copy link

vercel bot commented Jul 31, 2024

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 Jul 31, 2024 6:15am

@Kelsey-Ethyca
Copy link
Contributor Author

Copy link

cypress bot commented Jul 31, 2024



Test summary

4 0 0 0


Run details

Project fides
Status Passed
Commit 2fb7e95643 ℹ️
Started Jul 31, 2024 6:23 AM
Ended Jul 31, 2024 6:24 AM
Duration 00:37 💡
OS Linux Ubuntu -
Browser Electron 106

View run in Cypress Cloud ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

Copy link
Contributor

@jpople jpople left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed can delete integrations and confirmation modal appears from all three locations. Few questions/nitpicks:

  • Can we add a Cypress test for deleting from the new integration detail view?
  • Can we add a toast for deleting from the connection manager/new integration view? There's one already for deleting from the system integrations tab. (You can use the useQueryResultToast hook for this)
  • Do we want deletion on the new integrations menu to only be done from the detail view? I'd sort of expect it to be possible from list view
  • Related, there are actually two DeleteConnectionModal components now that are identical aside from their trigger elements, one for the system tab and the one you changed here which is used in both of the "orphaned connection" views. Would it be possible to merge those, or at least extract the modal itself into a shared component?

@Kelsey-Ethyca
Copy link
Contributor Author

* Can we add a Cypress test for deleting from the new integration detail view?

Yes, I can I created a new ticket for this.

* Can we add a toast for deleting from the connection manager/new integration view?  There's one already for deleting from the system integrations tab.  (You can use the `useQueryResultToast` hook for this)

Yes, this is in the new ticket.

* Do we want deletion on the new integrations menu to only be done from the detail view?  I'd sort of expect it to be possible from list view

I can add this to the ticket as well but this is the only place in the current ticket/design.

* Related, there are actually two DeleteConnectionModal components now that are identical aside from their trigger elements, one for the system tab and the one you changed here which is used in both of the "orphaned connection" views.  Would it be possible to merge those, or at least extract the modal itself into a shared component?

I wanted to do this but it was going to be a decent amount of work. I will add it to the ticket as a nice to have.

https://ethyca.atlassian.net/browse/PROD-2596

@Kelsey-Ethyca Kelsey-Ethyca merged commit 78c1fa8 into main Aug 13, 2024
13 checks passed
@Kelsey-Ethyca Kelsey-Ethyca deleted the PROD-2373-ability-to-delete-integrations branch August 13, 2024 14:28
Copy link

cypress bot commented Aug 13, 2024



Test summary

4 0 0 0


Run details

Project fides
Status Passed
Commit 78c1fa8
Started Aug 13, 2024 2:40 PM
Ended Aug 13, 2024 2:40 PM
Duration 00:36 💡
OS Linux Ubuntu -
Browser Electron 106

View run in Cypress Cloud ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

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.

2 participants