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

Upgrading to 6.0.0 breaks existing Subscriber implementations #62

Closed
xfxf opened this issue Jul 23, 2018 · 6 comments
Closed

Upgrading to 6.0.0 breaks existing Subscriber implementations #62

xfxf opened this issue Jul 23, 2018 · 6 comments

Comments

@xfxf
Copy link

xfxf commented Jul 23, 2018

This may be intentional - in which case I'd advise adding some documentation in README.md about this - but it appears a 'consent_to_track' parameter (for GDPR) has been added as a required field (with no default) to the .update() and .add() API's on Subscriber:

def add(self, list_id, email_address, name, custom_fields, resubscribe, consent_to_track, restart_subscription_based_autoresponders=False):

This has resulted in our app breaking when doing a routine package upgrade. The fix is easy, but this is likely going to bite others.

I'm happy to put in a PR - either for adding some 'beware when upgrading' info to README.md, or having a default for this parameter (with an info.warning() advising the API call should be upgraded).

@xfxf
Copy link
Author

xfxf commented Jul 23, 2018

(On Campaign Monitor's side - is this a simple boolean, or can it also be unset (3 states)? In which case, is 'None' a good default, assuming the API accepts that?)

edit: tried this, I get ClientError: Consent to track value must be one of ('yes', 'no', 'unchanged').

My assertion here is the default (i.e. not set) should be 'unchanged' - please confirm if OK, will put a PR in. Could we also make it so a nullable bool is accepted here (i.e. True, False, None) as well as the current strings?

@katharosada
Copy link
Contributor

Yeah, it was a tough call to introduce a breaking change in the API. Since the changes were made for the purpose of GDPR we were (and are) very wary of assuming consent at any point in the process.

The breaking change was described in the release notes: https://github.com/campaignmonitor/createsend-python/blob/master/HISTORY.md but yes, I agree that it would be useful to have this clearly called out in the README as well.

The API will only accept the values "yes", "no" and "unchanged" so that's what the Python wrapper accept.s I'd be happy to change the Python wrapper to also accept True/False/None and convert to the equivalent string value before making the request. With providing a default value, though, I'll need to double check with the relevant people since that was consciously decided against in the original decision.

@xfxf
Copy link
Author

xfxf commented Jul 24, 2018

I figured it was intentional, given this is an official library and consent should be explicit. We've solved this issue in our code base (we discovered it in advance of merging in a GDPR PR, which uses this parameter, so the fix was just to merge that PR in), but I created this issue as I'm sure it'll bite others who aren't expecting a breaking change by just upgrading their libraries.

More than happy to whip up a PR based on what you're comfortable doing.

@xfxf
Copy link
Author

xfxf commented Jul 24, 2018

Looks like it being explicit / breaking the API was the decision in other language libraries:

campaignmonitor/createsend-ruby@180b658
campaignmonitor/createsend-php@e343054

@katharosada
Copy link
Contributor

Yeah, that decision was consistent across the APIs. The Pythonic-ness of the APIs is up to my discretion though, so I'm happy to review and accept PRs updating the README or adding support for True/False/None

@xfxf
Copy link
Author

xfxf commented Jul 25, 2018

As per #63 I'm going to close this. Given this is a breaking change across all libraries, it's not a problem that is particularly unique to this implementation nor deserves to be at the top of the README.md.

@xfxf xfxf closed this as completed Jul 25, 2018
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

No branches or pull requests

2 participants