Slack integration management and team sorting improvements#333
Open
spencerhcheng wants to merge 6 commits intomainfrom
Open
Slack integration management and team sorting improvements#333spencerhcheng wants to merge 6 commits intomainfrom
spencerhcheng wants to merge 6 commits intomainfrom
Conversation
…g join - Users without organization_id can connect/disconnect Slack - Personal Slack integration is deleted when user joins an organization - Admins and super_admins can always manage Slack integration
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @spencerhcheng's task —— View job PR Code ReviewTasks:
Issues FoundBackend (
|
- Button now only renders for admins, super_admins, or users without org - Members in org see message instead of button - Prevents confusion for users who can't connect Slack
|
Claude finished @spencerhcheng's task —— View job PR Code ReviewTasks:
Issues FoundBackend (
|
Contributor
Greptile OverviewGreptile SummaryThis PR implements Slack integration management for users without organizations and automatic cleanup when users join organizations. However, there are critical issues that will prevent the code from running: Major Issues:
What Works:
Required Fixes:
Confidence Score: 0/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
302
to
309
| from app.models.slack import SlackIntegration | ||
| personal_slack = db.query(SlackIntegration).filter( | ||
| SlackIntegration.user_id == current_user.id, | ||
| SlackIntegration.organization_id.is_(None) | ||
| ).first() | ||
| if personal_slack: | ||
| db.delete(personal_slack) | ||
| logger.info(f"Deleted personal Slack integration for user {current_user.email} joining org {invitation.organization_id}") |
Contributor
There was a problem hiding this comment.
SlackIntegration model does not have an organization_id column. This query will fail at runtime with AttributeError.
Check the model at backend/app/models/slack_integration.py - it only has: id, user_id, slack_token, slack_user_id, workspace_id, webhook_url, token_source, created_at, updated_at.
If you need to track organization-level vs personal Slack integrations, you'll need to:
- Add a migration to add
organization_idcolumn toslack_integrationstable - Update the
SlackIntegrationmodel to include this column
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/api/endpoints/invitations.py
Line: 302:309
Comment:
`SlackIntegration` model does not have an `organization_id` column. This query will fail at runtime with `AttributeError`.
Check the model at `backend/app/models/slack_integration.py` - it only has: `id`, `user_id`, `slack_token`, `slack_user_id`, `workspace_id`, `webhook_url`, `token_source`, `created_at`, `updated_at`.
If you need to track organization-level vs personal Slack integrations, you'll need to:
1. Add a migration to add `organization_id` column to `slack_integrations` table
2. Update the `SlackIntegration` model to include this column
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
395
to
402
| from app.models.slack import SlackIntegration | ||
| personal_slack = db.query(SlackIntegration).filter( | ||
| SlackIntegration.user_id == current_user.id, | ||
| SlackIntegration.organization_id.is_(None) | ||
| ).first() | ||
| if personal_slack: | ||
| db.delete(personal_slack) | ||
| logger.info(f"Deleted personal Slack integration for user {current_user.email} joining org {invitation.organization_id}") |
Contributor
There was a problem hiding this comment.
Same issue as above - SlackIntegration.organization_id does not exist and will cause runtime error
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/api/endpoints/invitations.py
Line: 395:402
Comment:
Same issue as above - `SlackIntegration.organization_id` does not exist and will cause runtime error
How can I resolve this? If you propose a fix, please make it concise.| try: | ||
| # Delete user's personal Slack integration if they have one | ||
| # (organizations should have a single shared Slack workspace) | ||
| from app.models.slack import SlackIntegration |
Contributor
There was a problem hiding this comment.
logger is used but not imported. Add at the top of the file:
Suggested change
| from app.models.slack import SlackIntegration | |
| import logging | |
| logger = logging.getLogger(__name__) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/app/api/endpoints/invitations.py
Line: 302:302
Comment:
`logger` is used but not imported. Add at the top of the file:
```suggestion
import logging
logger = logging.getLogger(__name__)
```
How can I resolve this? If you propose a fix, please make it concise.- Add missing logger import in invitations.py - Update error messages to be more accurate (organization admins) - Add comments clarifying atomic transaction behavior - Fix: Changed 'Only admins can disconnect' to 'Only admins can manage' - Fix: Changed 'Only admins can connect' to 'Only organization admins can connect'
|
Claude finished @spencerhcheng's task —— View job PR Code ReviewTasks:
Critical Issues FoundBackend (
|
- Add organization_id column to slack_integrations table (nullable) - Create database migration script - Add foreign key constraint to organizations table - NULL indicates personal integration, set value indicates org-owned - Fixes AttributeError that would occur when querying organization_id
|
Claude finished @spencerhcheng's task —— View job PR Code ReviewTasks:
Issues FoundBackend (
|
- Add organization_id attribution to SlackIntegration on OAuth callback - NULL for personal integrations (users without org) - Set to user's organization_id for org-owned integrations - Ensures proper tracking of integration ownership
|
Claude finished @spencerhcheng's task —— View job PR Code ReviewTasks:
Issues FoundBackend (
|
- Fix import path: Use relative import from ...models.slack_integration - Update error message to 'Only organization admins can manage' - Sanitize workspace name to prevent XSS in toast notifications - Strip HTML/script tags from workspace name before display
|
Claude finished @spencerhcheng's task —— View job PR Code ReviewTasks:
Issues FoundFrontend (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes