Skip to content

Add External ID Field to Contact Model #128

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.

Sign up for GitHub

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

Merged
keith-chartmogul merged 3 commits into from
Mar 13, 2026
Merged

Add External ID Field to Contact Model #128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3e748a
[del-1612] Added support for 'external_id' field in Contact model req…
keith-chartmogul Mar 12, 2026
5afbe7f
Responded to PR feedback
keith-chartmogul Mar 13, 2026
0db7934
Reverted some changes from previous commit and bumped library version…
keith-chartmogul Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions chartmogul/api/contact.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class _Schema(Schema):
customer_uuid = fields.String(allow_none=True)
data_source_uuid = fields.String(allow_none=True)
customer_external_id = fields.String(allow_none=True)
external_id = fields.String(allow_none=True, load_default=None)
Copy link
Copy Markdown
Contributor

@briwa briwa Mar 13, 2026

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Contributor Author

@keith-chartmogul keith-chartmogul Mar 13, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@briwa briwa Mar 13, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@keith-chartmogul keith-chartmogul Mar 13, 2026

Choose a reason for hiding this comment

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

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. 👍

first_name = fields.String(allow_none=True)
last_name = fields.String(allow_none=True)
position = fields.Int(allow_none=True)
Expand Down
56 changes: 56 additions & 0 deletions test/api/test_contact.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"customer_uuid": "cus_00000000-0000-0000-0000-000000000000",
"data_source_uuid": "ds_00000000-0000-0000-0000-000000000000",
"customer_external_id": "external_001",
"external_id": "contact_external_id_001",
"first_name": "First name",
"last_name": "Last name",
"position": 9,
Expand All @@ -25,6 +26,7 @@
"uuid": "con_00000000-0000-0000-0000-000000000000",
"customer_uuid": "cus_00000000-0000-0000-0000-000000000000",
"data_source_uuid": "ds_00000000-0000-0000-0000-000000000000",
"external_id": "contact_external_id_001",
"first_name": "First name",
"last_name": "Last name",
"position": 9,
Expand All @@ -42,6 +44,18 @@

allContacts = {"entries": [contact], "cursor": "cursor==", "has_more": False}

contactWithoutExternalId = {k: v for k, v in contact.items() if k != "external_id"}

contactWithNullExternalId = {
**contact,
"external_id": None,
}

createContactWithNullExternalId = {
**createContact,
"external_id": None,
}


class ContactTestCase(unittest.TestCase):
"""
Expand Down Expand Up @@ -93,6 +107,30 @@ def test_create(self, mock_requests):
self.assertEqual(mock_requests.last_request.qs, {})
self.assertEqual(mock_requests.last_request.json(), createContact)

@requests_mock.mock()
def test_create_with_null_external_id(self, mock_requests):
mock_requests.register_uri(
"POST", "https://api.chartmogul.com/v1/contacts", status_code=200, json=contactWithNullExternalId
)

config = Config("token")
result = Contact.create(config, data=createContactWithNullExternalId).get()
self.assertEqual(mock_requests.call_count, 1, "expected call")
self.assertEqual(mock_requests.last_request.json(), createContactWithNullExternalId)
self.assertIsNone(result.external_id)

@requests_mock.mock()
def test_create_without_external_id(self, mock_requests):
mock_requests.register_uri(
"POST", "https://api.chartmogul.com/v1/contacts", status_code=200, json=contactWithoutExternalId
)

config = Config("token")
result = Contact.create(config, data=createContactWithNullExternalId).get()
self.assertEqual(mock_requests.call_count, 1, "expected call")
self.assertEqual(mock_requests.last_request.json(), createContactWithNullExternalId)
self.assertIsNone(result.external_id)

@requests_mock.mock()
def test_merge(self, mock_requests):
mock_requests.register_uri(
Expand Down Expand Up @@ -133,6 +171,24 @@ def test_modify(self, mock_requests):
self.assertEqual(mock_requests.last_request.json(), jsonRequest)
self.assertTrue(isinstance(expected, Contact))

@requests_mock.mock()
def test_modify_with_null_external_id(self, mock_requests):
mock_requests.register_uri(
"PATCH",
"https://api.chartmogul.com/v1/contacts/con_00000000-0000-0000-0000-000000000000",
status_code=200,
json=contactWithNullExternalId,
)

jsonRequest = {"external_id": None}
config = Config("token")
result = Contact.modify(
config, uuid="con_00000000-0000-0000-0000-000000000000", data=jsonRequest
).get()
self.assertEqual(mock_requests.call_count, 1, "expected call")
self.assertEqual(mock_requests.last_request.json(), jsonRequest)
self.assertIsNone(result.external_id)

@requests_mock.mock()
def test_retrieve(self, mock_requests):
mock_requests.register_uri(
Expand Down
Loading