-
Notifications
You must be signed in to change notification settings - Fork 437
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
feat(connections): Allow editing of connection display name, customer email, and customer domain via detail view on web app #2478
base: master
Are you sure you want to change the base?
Conversation
…main into connection config
display_name: z.string().optional(), | ||
customer_domain: z.string().optional(), | ||
customer_email: z.string().optional(), | ||
env: z.record(z.unknown()).optional() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right place to put this?
} | ||
|
||
public async updateMetadata(connections: Connection[], metadata: Metadata): Promise<void> { | ||
await db.knex.transaction(async (trx) => { | ||
public async updateMetadata(connections: Connection[], metadata: Metadata, trx?: Knex.Transaction): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functionality here stays the same, but needed to add an optional transaction parameter so this can be batched with updating the connection config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a quick look, works fine 👌🏻
@@ -199,6 +199,7 @@ web.route('/api/v1/integration/:providerConfigKey/connections').get(webAuth, con | |||
web.route('/api/v1/provider').get(configController.listProvidersFromYaml.bind(configController)); | |||
|
|||
web.route('/api/v1/connection').get(webAuth, connectionController.listConnections.bind(connectionController)); | |||
web.route('/api/v1/connection/metadata').patch(webAuth, updateMetadata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think you can create a dedicated endpoint, i.e: PUT api/v1/connection/config
public async replaceConnectionConfig(connection: Connection, config: ConnectionConfig) { | ||
await db.knex | ||
public async replaceConnectionConfig(connection: Connection, config: ConnectionConfig, trx?: Knex.Transaction) { | ||
const query = db.knex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually you can swap the object; it's annoying that knex is not exactly the same type as trx but it works almost transparently
public async replaceConnectionConfig(connection: Connection, config: ConnectionConfig, trx?: Knex.Transaction) {
const query = (trx || db.knex)
<div className="flex space-x-8"> | ||
<div className="flex flex-col w-1/2"> | ||
<span className="text-gray-400 text-xs uppercase mb-2">Customer Email</span> | ||
<EditableField value={customerEmail} onChange={setCustomerEmail} onSave={handleSave} dark /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should enforce email type
onBlur={handleBlur} | ||
onKeyDown={(e) => e.key === 'Enter' && handleBlur()} | ||
autoFocus | ||
className="border-border-gray bg-active-gray text-text-light-gray focus:border-white focus:ring-white block w-full appearance-none rounded-md border px-3 py-1 text-sm placeholder-gray-400 shadow-sm focus:outline-none" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe your changes
Adds functionality to edit connections to the web app. Extends off of this PR.
This is a tentative draft PR so we can discuss the approach in more depth. We don't have a dedicated endpoint to update a connectionConfig, so I've jammed the parameters into the update metadata endpoint which equally doesn't feel right. The obvious approach would be to add a dedicated updateConnectionConfig endpoint, but doing that without causing footguns means we'd need to keep a separate list of potential connectionConfig parameters and whether or not they can be edited.
All this is making me lean towards the idea that perhaps these fields are neither metadata nor connection configuration, and should instead be considered frontmatter - human intelligible metadata to be used in interfaces.
Issue ticket number and link
N/A
Checklist before requesting a review (skip if just adding/editing APIs & templates)