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

Allow customer_id to be specified in identify URL #111

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

richdawe-cio
Copy link
Contributor

@richdawe-cio richdawe-cio commented Dec 8, 2023

This is based on work by @jrbeck in #102 and @trwalzer in #88 and updated to work with the main branch. After some discussion within Customer.io Engineering, the functionality into the identify method, with a new customer_id attribute on that. It works in the same way as when it was a separate method.

These changes are necessary to allow identify operations where the person is identified by their email address, not id or cio_id. It updates identify so the identifier can be specified separately from the attributes, using the customer_id attribute. So you can e.g.: select a person based on their email address and set up their id and other attributes. See for example this use-case.

Notes to CIO reviewers:

  • The other client libraries seem to take the customer_id as their first parameter in the identify operation. I'm not sure why the Ruby client library does not. Supporting the customer_id attribute is bringing the Ruby client library.
  • One downside of the approach in this PR is that it's no longer possible to create an attribute customer_id on the person.
  • Perhaps we should break backwards compatibility and do a new version of the library that makes identify work like other client libraries - first arg is customer_id, attributes follow - so that customer_id can be created as a person attribute again.

@richdawe-cio
Copy link
Contributor Author

richdawe-cio commented Dec 11, 2023

@jeremyw @jrbeck, please let me know if you have any feedback on the current version of the PR?

After some feedback within CIO Engineering, I've moved the support for customer_id into the identify method, e.g.: identify(:customer_id => email, :other_attr => other_val). We would prefer not to add an additional method. I believe the current PR supports your use-cases, but it means you can't set the customer_id attribute on a person in CIO.

We also talked internally about making a breaking change in the API, to make the base identify call support the customer id, e.g.: identify(customer_id, attributes). This would be more inline with our other client libraries. But I didn't want to break your integrations without warning.

@jeremyw
Copy link

jeremyw commented Dec 11, 2023

@richdawe-cio This makes sense to me. My initial thought when I saw the bug was to do something similar, but I had considered naming the customer_id argument identifier to match the API documentation. This totally works though, has a smaller impact on clients, and makes more sense if it's consistent with your other client adapters.

Thanks for the work on this! I'm optimistic that we can get this changed rolled out to production early this week if 5.3.0 is published. Already have a branch ready to go based on these changes.

@richdawe-cio richdawe-cio merged commit c29ac0b into main Dec 11, 2023
12 checks passed
@richdawe-cio richdawe-cio deleted the identify_with_customer_id branch December 11, 2023 16:36
@richdawe-cio
Copy link
Contributor Author

@jeremyw I've just published 5.3.0 to RubyGems. Thanks for your help and feedback!

@jeremyw
Copy link

jeremyw commented Dec 12, 2023

@richdawe-cio We are live in production and this definitely looks to have resolved the problem! Thank you!

@richdawe-cio
Copy link
Contributor Author

@jeremyw I'm glad to hear that. Thanks for letting us know.

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.

3 participants