Add External ID Field to Contact Model#128
Conversation
keith-chartmogul
changed the title
chartmogul/api/contact.py
Outdated
| customer_external_id = fields.String(allow_none=True) | ||
| external_id = fields.String(allow_none=True, load_default=None) |
There was a problem hiding this comment.
[minor] Not specifically about this PR, but external_id seems pretty much ambiguous now that it's next to customer_external_id; what external id is the second one referring to? Perhaps it's worth renaming it to something more specific
Also it'd be great if perhaps there is a comment/explanation as to why load_default is needed
There was a problem hiding this comment.
Renamed the schema name for this field to contact_external_id while ensuring that it is still sent to the API as external_id and added a comment regarding the use of load_default.
There was a problem hiding this comment.
No I mean it's how the field is called even in the REST API: there's customer_external_id and there's external_id. I think the update should've been in the REST API itself and the clients should adjust to it. That's what I meant by not specifically about this PR. Now you're sort of introducing something different here: for Python it's contact_external_id but everywhere else it's external_id 😅 right?
I think we could just merge your previous version as-is and decide if we should rename external_id to contact_external_id on the REST API itself, and then all clients. Or maybe this was just subjective, depending on the consensus really
There was a problem hiding this comment.
Hmm, that was definitely not clear from your previous comment. If that is the case and you really feel strongly about renaming the field in the public API then perhaps I should sit on all of these client library PRs until that is renamed and then update these PRs accordingly. 👍
|
I received validation from Dijana that maintaining the |
keith-chartmogul
deleted the
keith/del-1612-python-client-library-add-external-id-field-to-contact-model
branch
3 participants
This PR updates the Contact model to include the
external_idfield. Theexternal_idfield can be either a string or null. The Contact model tests have been updated to demonstrate how theexternal_idfield can be set in both CREATE and UPDATE requests.