Skip to content
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

status updates: switch to per-contact-method behavior #2824

Merged
merged 18 commits into from
Mar 10, 2023

Conversation

mastercactapus
Copy link
Member

@mastercactapus mastercactapus commented Feb 27, 2023

Description:
This PR changes the behavior of status updates to be a per-contact-method setting, instead of a global user-level setting.

Which issue(s) this PR fixes:
Related to #2712

Screenshots:
image

Describe any introduced user-facing changes:

  • Users can now control if status updates are enabled for a specific contact method.
  • Status updates will only activate on contact methods that receive an initial notification.

Describe any introduced API changes:

  • New statusUpdates field on UserContactMethod and enableStatusUpdates on UpdateUserContactMethodInput

Additional Context

  • Creating a Slack DM contact method should create an entry in auth_subjects
  • Deleting the contact method should remove it
  • This should be reflected in alert notifications (e.g., ack should work with a CM)
  • "linking" via an alert ack button should result in a contact method being created

Slack DM functionality is behind a feature flag, test with make start EXPERIMENTAL=slack-dm

@mastercactapus mastercactapus changed the title statusupdates: switch to per-contact-method behavior status updates: switch to per-contact-method behavior Feb 27, 2023
@mastercactapus mastercactapus marked this pull request as ready for review March 3, 2023 16:20
@github-actions github-actions bot added the size/l label Mar 3, 2023
@mastercactapus mastercactapus requested a review from Forfold March 7, 2023 15:39
Copy link
Contributor

@Forfold Forfold left a comment

Choose a reason for hiding this comment

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

I had to run make regendb to get things working- not sure if this is expected with the new migration.

I also got an unexpected error when attempting to enable status updates for my SMS contact method via the UI:

AuthSource="SourceTypeAuthProvider{c7d64ef8-309a-40a4-bf75-9cee7ce3fe04}" AuthUserID=33d00c33-04ce-44c8-b729-e0f114257d27 RequestID=da7562d1-8f9c-4734-bde1-1d1171144d8d error="input: updateUserContactMethod sql: expected 4 arguments, got 3" host=sendit.dev.target.goalert.me http_method=POST http_proto=HTTP/1.1 referer="https://sendit.dev.target.goalert.me/cook/users/33d00c33-04ce-44c8-b729-e0f114257d27" remote_addr="127.0.0.1:52093" uri=/cook/api/graphql x_forwarded_for="98.246.168.48, 136.226.55.2, 35.190.115.218, 35.191.10.92, yamux" x_forwarded_host=

@mastercactapus
Copy link
Member Author

@Forfold For the regendb -- that's possible. We've had a lot of migrations in different branches/PRs recently, and switching between them can leave things out of order. It's okay if the order doesn't change after going to master. When reviewing, the critical bit is that new migration files are always at the end of the list when sorted alphabetically.

The SQL error was a bug; I pushed a fix.

Forfold
Forfold previously approved these changes Mar 8, 2023
Copy link
Contributor

@Forfold Forfold left a comment

Choose a reason for hiding this comment

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

lgtm!

@mastercactapus mastercactapus requested a review from m17ch March 9, 2023 16:08
Forfold
Forfold previously approved these changes Mar 9, 2023
@mastercactapus mastercactapus merged commit 077d509 into master Mar 10, 2023
@mastercactapus mastercactapus deleted the feat/status-update-per-cm branch March 10, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants