Skip to content

Expose Slack profile synchronization failures - #1522

Merged
skyfallwastaken merged 1 commit into
mainfrom
fix/slack-profile-sync-api-errors
Aug 4, 2026
Merged

Expose Slack profile synchronization failures#1522
skyfallwastaken merged 1 commit into
mainfrom
fix/slack-profile-sync-api-errors

Conversation

@skyfallwastaken

@skyfallwastaken skyfallwastaken commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary of the problem

Follow-up to #1520. A Slack profile sync could appear successful in GoodJob while retaining the old avatar and name. Slack often reports API failures as HTTP 200 responses with ok: false; the sync silently treated those responses as missing profile data and returned successfully.

Describe your changes

  • Raise a descriptive SlackIntegration::ApiError for HTTP errors and Slack ok: false responses
  • Re-raise non-rate-limit failures from SlackProfileSyncJob after reporting them so GoodJob records the execution as failed rather than successful
  • Continue preserving the previously persisted Slack profile on all failures
  • Added a test for the HTTP 200 missing_scope case

Screenshots / Media

N/A

Copilot AI review requested due to automatic review settings August 4, 2026 18:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@skyfallwastaken
skyfallwastaken enabled auto-merge (squash) August 4, 2026 18:43
@skyfallwastaken
skyfallwastaken merged commit 327355d into main Aug 4, 2026
19 checks passed
@skyfallwastaken
skyfallwastaken deleted the fix/slack-profile-sync-api-errors branch August 4, 2026 18:43
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes Slack profile synchronization failures visible to GoodJob while preserving previously persisted profile data.

  • Adds a descriptive exception for unsuccessful HTTP and Slack API responses.
  • Re-raises reported non-rate-limit failures from the profile synchronization job.
  • Adds coverage for an HTTP 200 missing_scope response, although the previous HTTP 500 regression case is no longer covered.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking test-coverage gap for the distinct non-2xx Slack response branch.

The changed runtime behavior has a coherent failure path and preserves existing profile fields, but the revised test no longer verifies the newly promised handling of ordinary HTTP errors.

Files Needing Attention: test/jobs/slack_profile_sync_job_test.rb

Important Files Changed

Filename Overview
app/jobs/slack_profile_sync_job.rb Re-raises non-rate-limit exceptions after reporting so failed synchronization executions remain visible in GoodJob.
app/models/concerns/slack_integration.rb Introduces ApiError and raises it for unsuccessful HTTP responses or Slack payloads with ok: false.
test/jobs/slack_profile_sync_job_test.rb Covers the newly exposed missing_scope failure and profile preservation, but replaces rather than retains distinct non-2xx coverage.
Prompt To Fix All With AI
### Issue 1
test/jobs/slack_profile_sync_job_test.rb:77-78
**Non-2xx coverage was replaced**

The revised test replaces the HTTP 500 case with an HTTP 200 `ok: false` case, although these exercise separate operands of the new error condition. Retain the HTTP error case and add `missing_scope` separately so regressions that silently accept non-2xx responses or omit their status from `ApiError` remain covered.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "Expose Slack profile API failures" | Re-trigger Greptile

Comment thread test/jobs/slack_profile_sync_job_test.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants