chore: Add orgId to IntegrationProvider #6014
Conversation
Dschoordsch
changed the title
|
@mattkrick @BartoszJarocki could you take a look and tell me what you think about that change? |
| ALTER TABLE "IntegrationProvider" | ||
| ADD CONSTRAINT "scope_org_has_only_orgId" CHECK | ||
| ("scope" <> 'org' OR ("orgId" IS NOT NULL AND "teamId" IS NULL)), | ||
| ADD CONSTRAINT "scope_team_has_only_reamId" CHECK |
There was a problem hiding this comment.
teamId instead of reamId
| BEGIN | ||
| ALTER TABLE "IntegrationProvider" | ||
| DROP CONSTRAINT "scope_org_has_only_orgId", | ||
| DROP CONSTRAINT "scope_team_has_only_reamId", |
There was a problem hiding this comment.
teamId instead of reamId
Dschoordsch
changed the title
Dschoordsch
changed the title
|
@BartoszJarocki could you take a look again? |
There was a problem hiding this comment.
Code looks good but I can't add integrate mattermost anymore. I'm getting the following error when I try to set the webhook URL:
Trace: {
error: '{"data":null,"errors":[{"message":"there is no unique or exclusion constraint matching the ON CONFLICT specification","locations":[{"line":4,"column":3}],"path":["addIntegrationProvider"]}]}'
}
It works on master so it'd be great if you could take a look @Dschoordsch
|
@BartoszJarocki thanks for finding this bug! I was missing a unique constraint. I tested with adding and updating a mattermost webhook and adding and removing Jira server auth. |
|
Teams can change orgs 🙂 Today, if a team changes an org:
IMO, anytime the ER model goes full circle with two 1:Ms on the same entity, things get messy That said, if you prefer updating Also if there's a good place to store this reasoning in the repo let me know. It'd be good to have it stored there to explain why it's built the way it is. |
|
|
Makes sense, the UI is gonna be real important with regards to how intuitive sharing feels. The closest design we have is how we do templates today: |
|
@Dschoordsch doing some PR housekeeping. would you like to keep this one open? |
|
pinging @Dschoordsch on next steps for this. shall we close this but leave the branch alive so we can pick it back up when we prioritize? |
|
There is no good way of attaching an org scoped IntegrationProvider to any specific team from the Organization Settings. I think this indicates that this is necessary after all. I think you're right about a team moving orgs and I agree the currently team scoped integration providers (MS Teams and Mattermost webhook providers) should move with the team. |
Integration providers are added with different scopes. Currently it is cumbersome to query for org scope since only the `teamId` is part of the integration. This migration would add the `orgId` for org scope and add constraints that the correct id is set depending on scope. TODO: The queries accessing the provider need to be adapted to the new structure.
Dschoordsch
force-pushed
the
add_orgId_to_IntegrationProvider
branch
from
658791d to
0e70020
Compare
WalkthroughThe recent changes significantly enhance the management of integration providers by shifting focus from team identifiers to organisational identifiers. This update includes modifications to interfaces, functions, and validations to ensure operations align with an organisational context. Key adjustments involve replacing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Database
Client->>API: Request to add integration provider
API->>Database: Validate teamId and orgId
alt orgId exists
Database-->>API: Return valid orgId
API->>Database: Insert integration provider with orgId
Database-->>API: Confirm insertion
API-->>Client: Success response
else orgId does not exist
API-->>Client: Error response
end
sequenceDiagram
participant Client
participant API
participant Database
Client->>API: Request to fetch shared integration providers
API->>Database: Query using orgIds
Database-->>API: Return integration providers
API-->>Client: Response with integration providers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/server/postgres/queries/src/upsertIntegrationProviderQuery.sql (1)
16-17: EnsureorgIdis included in theproviderobject passed toupsertIntegrationProviderQuery.The
orgIdfield is missing from theIUpsertIntegrationProviderInputinterface. To correctly handle the new structure of the SQL query, add theorgIdfield to the interface and ensure it is populated when constructing theproviderobject.
- File:
packages/server/postgres/queries/upsertIntegrationProvider.ts- Lines to update:
- Add
orgIdtoIUpsertIntegrationProviderInputinterface.- Ensure
orgIdis included in theproviderobject passed toupsertIntegrationProviderQuery.run.Analysis chain
LGTM! Ensure correct usage of the updated SQL query.
The changes to the SQL query align with the PR objectives. The
orgIdfield has been added to theINSERTstatement and theON CONFLICTclause. Ensure that all usages of this query are updated accordingly to handle the new structure.Also applies to: 31-32, 34-34
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the updated `upsertIntegrationProviderQuery`. # Test: Search for the query usage. Expect: All usages should handle the new structure. rg --type sql -A 5 'upsertIntegrationProviderQuery'Length of output: 598
Script:
#!/bin/bash # Description: Verify all usages of the updated `upsertIntegrationProviderQuery` in JavaScript and TypeScript files. # Test: Search for the query usage in JS/TS files. Expect: All usages should handle the new structure. rg --type js,ts -A 5 'upsertIntegrationProviderQuery'Length of output: 83
Script:
#!/bin/bash # Description: Verify all usages of the updated `upsertIntegrationProviderQuery` in JavaScript and TypeScript files. # Test: Search for the query usage in JS files. Expect: All usages should handle the new structure. rg --type js -A 5 'upsertIntegrationProviderQuery' # Test: Search for the query usage in TS files. Expect: All usages should handle the new structure. rg --type ts -A 5 'upsertIntegrationProviderQuery'Length of output: 1390
Script:
#!/bin/bash # Description: Extract relevant parts of the TypeScript file to verify handling of `orgId`. # Extract the content of the TypeScript file where `upsertIntegrationProviderQuery` is used. rg --type ts -A 10 'upsertIntegrationProviderQuery' packages/server/postgres/queries/upsertIntegrationProvider.tsLength of output: 707
Script:
#!/bin/bash # Description: Search for the definition and construction of the `provider` object in the TypeScript file. # Extract lines where `provider` is defined or constructed. rg --type ts 'provider' packages/server/postgres/queries/upsertIntegrationProvider.tsLength of output: 258
Dschoordsch
changed the title
packages/server/graphql/mutations/helpers/notifications/MattermostNotifier.ts
Show resolved
Hide resolved
packages/server/graphql/mutations/helpers/notifications/MSTeamsNotifier.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/server/graphql/public/typeDefs/TeamSubscriptionPayload.graphql (1)
Inconsistencies found due to the removal of
UpdateIntegrationProviderSuccessfromTeamSubscriptionPayload.The
UpdateIntegrationProviderSuccessfield is still referenced in several parts of the codebase. To avoid inconsistencies or errors, please review and update the following files:
packages/client/subscriptions/OrganizationSubscription.tspackages/server/graphql/types/UpdateIntegrationProviderPayload.tspackages/client/mutations/UpdateIntegrationProviderMutation.tspackages/server/graphql/mutations/updateIntegrationProvider.tspackages/server/graphql/public/typeDefs/OrganizationSubscriptionPayload.graphqlpackages/server/graphql/public/typeDefs/UpdateIntegrationProviderSuccess.graphqlpackages/server/graphql/public/typeDefs/UpdateIntegrationProviderPayload.graphqlPlease ensure that these references are either updated or removed as appropriate to maintain consistency in the codebase.
Analysis chain
Line range hint
1-63:
Confirm the removal of theUpdateIntegrationProviderSuccessfield.The
UpdateIntegrationProviderSuccessfield has been removed from theTeamSubscriptionPayloadGraphQL type definition. Ensure that this change aligns with the design objectives and does not introduce inconsistencies or errors in the GraphQL schema or client code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of the `UpdateIntegrationProviderSuccess` field does not introduce inconsistencies or errors. # Test: Search for the usage of `UpdateIntegrationProviderSuccess` in the codebase. Expect: No occurrences of the removed field. rg --type graphql 'UpdateIntegrationProviderSuccess'Length of output: 84
Script:
#!/bin/bash # Description: Verify that the removal of the `UpdateIntegrationProviderSuccess` field does not introduce inconsistencies or errors. # Test: Search for the usage of `UpdateIntegrationProviderSuccess` in the codebase. Expect: No occurrences of the removed field. rg 'UpdateIntegrationProviderSuccess'Length of output: 1208
Integration providers are added with different scopes. Currently it is
cumbersome to query for org scope since only the
teamIdis part of theintegration. This migration would add the
orgIdfor org scope and addconstraints that the correct id is set depending on scope.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements