search

campaignmonitor / createsend-python

A Python library for the Campaign Monitor API
http://campaignmonitor.github.io/createsend-python
MIT License
57 stars 63 forks source link

Upgrading to 6.0.0 breaks existing Subscriber implementations #62

Closed xfxf closed 6 years ago

xfxf commented 6 years ago

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:

https://github.com/campaignmonitor/createsend-python/blob/6283291bba8b56ede95e980305bb718870b5ec58/lib/createsend/subscriber.py#L27

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 commented 6 years ago

(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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

https://github.com/campaignmonitor/createsend-ruby/commit/180b658c32d71317c8b7be191d70e2063923f709 https://github.com/campaignmonitor/createsend-php/commit/e343054ee25e4862cd408878c9e555b0a1e047e7

katharosada commented 6 years ago

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 commented 6 years ago

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.