Skip to content

Conversation

@gmartinimighty
Copy link

No description provided.

Copy link

@roqmarcelo roqmarcelo left a comment

Choose a reason for hiding this comment

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

LGTM

# @option taggings [String] :tag_id
# @option taggings [String] :subscriber_id
def bulk_tag_subscribers(taggings = [])
def bulk_add_to_subscribers(taggings = [])
Copy link

Choose a reason for hiding this comment

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

Not a huge deal, but I chose this name to reflect Kit documentation:

https://developers.kit.com/v4#bulk-tag-subscribers

Otherwise, I think your name is better. We're in the Tags scope, so it's a little redundant to explicitly mention 'tag' in the method name.

I could be convinced to go either way. If we do want to match documentation, the remove method should be called bulk_remove_tags_from_subscribers, which is honestly not a great method name. Again, yours is better, IMO

Copy link
Author

Choose a reason for hiding this comment

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

I will leave the name as is because I think it is closer to the pattern implemented on this file

@gmartinimighty gmartinimighty merged commit 3c40874 into adin/tag-sync-PoC Jan 9, 2025
1 check passed
@gmartinimighty gmartinimighty deleted the gmartini/BAN-8680-add-bulk-remove-tag-from-subscriber branch January 9, 2025 15:52
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.

5 participants