Auto-resync Slack details upon changes - #1525
Conversation
Greptile SummaryThe PR adds signed Slack
Confidence Score: 4/5The PR should not merge until Slack email reconciliation is ownership-safe and atomic with the background profile save. A Slack callback can permanently delete email records from an account selected only through an email match, and the background synchronization can commit email identity changes even when saving the associated profile subsequently fails. Files Needing Attention: app/models/concerns/oauth_authentication.rb, app/models/concerns/slack_integration.rb, app/jobs/slack_profile_sync_job.rb Important Files Changed
Sequence DiagramsequenceDiagram
participant Slack
participant Events as SlackController
participant Job as SlackProfileSyncJob
participant API as Slack API
participant Email as EmailAddress
participant User
Slack->>Events: signed user_change event
Events->>Job: enqueue user ID
Job->>API: users.info
API-->>Job: current profile and email
Job->>Email: reconcile Slack email
Email-->>Job: transaction commits
Job->>User: save profile and slack_synced_at
alt User save fails
User-->>Job: persistence error
Note over Email,User: Email changes remain committed
end
Prompt To Fix All With AI### Issue 1
app/models/concerns/oauth_authentication.rb:101
**OAuth deletes another account's emails**
If the Slack email is already attached to a different Hackatime user, `email_address.user` selects that user and this `destroy_all` permanently removes their other Slack-sourced email records before replacing their Slack credentials.
### Issue 2
app/models/concerns/slack_integration.rb:80-84
**Email reconciliation commits before profile**
When `user.save!` fails after a Slack email change, this inner transaction has already committed the replacement email and deleted the former email, leaving authentication records updated while the profile fields and `slack_synced_at` remain unsaved.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (3): Last reviewed commit: "Rollback Slack email changes when OAuth ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| user.country_code = country_code_from_ip(ip_address) if user.country_code.blank? | ||
| user.save! | ||
| User.transaction do | ||
| user.email_addresses.source_slack.where.not(email: email).destroy_all |
There was a problem hiding this comment.
If the Slack email is already attached to a different Hackatime user, email_address.user selects that user and this destroy_all permanently removes their other Slack-sourced email records before replacing their Slack credentials.
Knowledge Base Used: Authentication: sessions, API keys, and OAuth
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/concerns/oauth_authentication.rb
Line: 101
Comment:
**OAuth deletes another account's emails**
If the Slack email is already attached to a different Hackatime user, `email_address.user` selects that user and this `destroy_all` permanently removes their other Slack-sourced email records before replacing their Slack credentials.
**Knowledge Base Used:** [Authentication: sessions, API keys, and OAuth](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-authentication.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| transaction do | ||
| email_address ||= email_addresses.create!(email: email, source: :slack) | ||
| email_addresses.source_slack.where.not(id: email_address.id).destroy_all | ||
| email_address.update!(source: :slack) unless email_address.source_slack? | ||
| end |
There was a problem hiding this comment.
When user.save! fails after a Slack email change, this inner transaction has already committed the replacement email and deleted the former email, leaving authentication records updated while the profile fields and slack_synced_at remain unsaved.
Knowledge Base Used: Authentication: sessions, API keys, and OAuth
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/concerns/slack_integration.rb
Line: 80-84
Comment:
**Email reconciliation commits before profile**
When `user.save!` fails after a Slack email change, this inner transaction has already committed the replacement email and deleted the former email, leaving authentication records updated while the profile fields and `slack_synced_at` remain unsaved.
**Knowledge Base Used:** [Authentication: sessions, API keys, and OAuth](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-authentication.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary of the problem
If you change your avatar or display name on Slack, it doesn't get picked up by Hackatime until a) the nightly re-sync completes, or b) you sign out and sign back in. Both are equally bad solutions!
Describe your changes
Listen for
user_changeevents and trigger a re-sync if they're relevant!Screenshots / Media
N/A