Linking existing connector to system requires credential re-entry

[adamsachs]

Bug Description

When using the "link connector" button on the "integrations" tab of a System, a user needs to re-enter the credentials (secrets) for the existing connector, even if the connector already had credentials associated with it -- if you do not, you'll encounter errors about not having supplied the necessary credential/secret fields (i.e., this is not merely just the FE not showing the credentials).

Additionally, I'm noticing that users can get into what seems like a confusing/problematic state if they abort this connector linking operation before it's complete, e.g. after hitting the credential limitation above and not proceeding. in this state, the "original" connector no longer exists, but it also hasn't successfully been linked to the system...and so it feels like this puts the user in a bit of a "no man's land".

loom showing the behavior:
https://www.loom.com/share/be06515b066040889f163d87602ac527

Steps to Reproduce

1. run fides with at least one existing connector that's not tied to a system (e.g. nox -s "fides_env(test)")
2. test the connection on one of these connectors and ensure that it succeeds, which confirms that credentials are properly stored on the connector at this point
3. create a new system, navigate to the "integrations" tab, choose the "link connector" option and choose the connector from the steps above
4. notice that you need to re-enter credentials for the connector. if you try to save the connection without re-entering credentials, then an error is thrown and you're prevented from saving.
5. notice that if you then navigate away (i.e. "abort" the operation), then the connector's been removed from the general/legacy connector screen, but it's still not integrated/connected to the system.

Expected behavior

Users shouldn't need to re-enter credentials, they should be sustained or "migrated" from the original "standalone" connector to the "linked" connector.

Screenshots

See loom linked to above.

[adamsachs]

on some further investigation, it actually looks like this is not nearly as bad as it seemed at first glance - we're just guiding the user into making an extra API call and then surfacing an error when there's nothing really to complain about! to explain:

• when the user clicks "link" initially to link the connector to the system, the FE makes a PATCH /api/v1/system/{system_key}/connection call that correctly associates the given connector with the System. from what i can tell, this call does maintain the secret information associated with the connector/connection. at this point, i think we should display a success message to the user, and just indicate that the "linking" operation is completed, because it is.
• instead, no success message is shown, and a connector edit form with mostly empty fields is presented to the user, along with an activated "save" button at the bottom of the form. to me, this indicates to the user that they need to at least click save on the bottom of the form, in order to complete the "link" operation. i don't think this is true - i believe the "link" was already successful, and this form that's come up is just a basic "edit" form on the already-linked connector.
• if/when the user does click "save", then UI also tries to make a PUT /api/v1/connection/postgres_connector/secret?verify=false call with the "secret"/credential info that's been input (or not) in the UI form by the user. if no secret information has been input by the user, so this call fails with a 422 (as expected), and that error message is surfaced to the user, which makes it seem as if the entire save/linking operation had failed - when it was really just this additional, unnecessary PUT call to add secret information -- which the user didn't even need to do!
• in addition to reading the BE code behind this and seeing that we've got some related automated test coverage, i confirmed that the initial PATCH /api/v1/system/{system_key}/connection call correctly preserves the credential/secret info from the original connection by clicking the 'Test connection' button on the integrations page with the "half-saved" linked connector - it succeeds, which indicates that our credentials were properly preserved.

all in all, i think we just need to tweak the UX here a bit. my suggestion would be that we make it very explicit to the user that just by selecting "link connector" and selecting the desired connector, the link operation is successful; we need to make it clearer then that the subsequent edit form that appears is for any additional edits to the now-linked connector, i.e. it's a totally separate operation. with the current UX, all of this feels all bundled together as one operation, and it's unclear if/when the initial "link connector" operation succeeds.

but i'll let others decide on the best UX going forward, that's just my 2 cents. as it stands, this is in no way a blocker, we just need to clear this up a bit for end users.

[rsilvery]

Ah okay! Very helpful context. So, we're currently working on having the fields show that values exist - should be done when @TheAndrewJackson returns. And we can file a ticket for the success toast and probably tackle that quickly. Right @Kelsey-Ethyca ?

[Kelsey-Ethyca]

Closing and opening #3644