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

feat: support all_users & all_sender_channels for segment #164

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

kanat
Copy link
Contributor

@kanat kanat commented Mar 13, 2024

Related ticket:

Related PRs:

Description

This PR enables support for sending a campaign:

  • to all users
  • to all channels where the sender is a member.

CLA

  • I have signed the Stream CLA (required).
  • The code changes follow best practices
  • Code changes are tested (add some information if not applicable)

Description of the pull request

@kanat kanat requested a review from gumuz as a code owner March 13, 2024 16:03
akupila
akupila previously approved these changes Mar 15, 2024
Copy link
Contributor

@akupila akupila left a comment

Choose a reason for hiding this comment

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

LGTM, but Python is not my strong suit so maybe good to wait for somebody else to have a look too

Maybe next time consider adding a short description in the PR for what it does (not just a Jira link). This makes the PR a bit easier to review as you don't need to jump to Jira to get context. It's also easier for people outside Stream, who won't have access to Jira

@kanat
Copy link
Contributor Author

kanat commented Mar 15, 2024

Thank you @akupila ❤️

I've added a description section :)

Copy link
Contributor

@JimmyPettersson85 JimmyPettersson85 left a comment

Choose a reason for hiding this comment

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

LGTM

There's just a small nitpick, you can avoid the duplicate verify_segment_id methods by having this on the base class (it can have concrete implementations) and call super().verify_segment_id() instead.

@kanat kanat dismissed stale reviews from JimmyPettersson85 and akupila via 6bc1695 March 19, 2024 20:21
@kanat kanat merged commit 7d57dcc into master Mar 19, 2024
6 checks passed
@kanat kanat deleted the campaign-feat branch March 19, 2024 20:53
@github-actions github-actions bot mentioned this pull request Mar 19, 2024
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.

4 participants