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.

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

Add upsert_person method to ActionNetwork #734

Merged
merged 4 commits into from
Aug 31, 2022
Merged

Add upsert_person method to ActionNetwork #734

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
295eaa8
Add upsert_person method to ActionNetwork, add deprecation warnings t…
shaunagm Aug 25, 2022
7c7d43c
Fix logging, un-deprecate update_person
shaunagm Aug 25, 2022
1da4dba
Add mocked test of upsert_person
shaunagm Aug 25, 2022
2c69cf1
Linted
shaunagm Aug 25, 2022
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
29 changes: 25 additions & 4 deletions parsons/action_network/action_network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,14 @@ def get_person(self, person_id):
"""
return self.api.get_request(url=f"people/{person_id}")

def add_person(self, email_address=None, given_name=None, family_name=None, tags=None,
languages_spoken=None, postal_addresses=None, mobile_number=None,
mobile_status='subscribed', **kwargs):
def upsert_person(self, email_address=None, given_name=None, family_name=None, tags=None,
languages_spoken=None, postal_addresses=None, mobile_number=None,
mobile_status='subscribed', **kwargs):
"""
Creates or updates a person record. In order to update an existing record instead of
creating a new one, you must supply an email or mobile number which matches a record
in the database.

`Args:`
email_address:
Either email_address or mobile_number are required. Can be any of the following
Expand Down Expand Up @@ -200,8 +204,25 @@ def add_person(self, email_address=None, given_name=None, family_name=None, tags
logger.info(f"Entry {person_id} successfully added to people.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal, but should probably change this language to reflect the upsert functionality.

return response

def add_person(self, email_address=None, given_name=None, family_name=None, tags=None,
languages_spoken=None, postal_addresses=None, mobile_number=None,
mobile_status='subscribed', **kwargs):
"""
Creates a person in the database. WARNING: this endpoint has been deprecated in favor of
upsert_person.
"""
logger.warning("Method 'add_person' has been deprecated. Please use 'upsert_person'.")
# Pass inputs to preferred method:
self.upsert_person(email_address=email_address, given_name=given_name,
family_name=family_name, languages_spoken=languages_spoken,
postal_addresses=postal_addresses, mobile_number=mobile_number,
mobile_status=mobile_status, **kwargs)

def update_person(self, entry_id, **kwargs):
"""
Updates a person's data in Action Network. WARNING: this endpoint has been deprecated in
favor of upsert_person and may not work properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we know that the person-specific endpoint does work with PUT requests, do we still think that we want to deprecate this? As an alternative, we could just remove the tagging functionality. For instance, NGPVAN has both upsert and update methods. On the other hand, maybe keeping things simpler is a better approach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think the person-specific endpoint is easier to use? If so, we should probably keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no reason to believe it is, but I'm afraid I also don't have any reason to believe it isn't. The only thing I know for sure is that the method we're changing to upsert (i.e. the one that uses the helper endpoint) is definitely used, and I have no notion of whether update_person() is used at all. Honestly, I think it matters very little, so I was hoping you'd have a structural, conventional, or even esthetic reason to help make the call.

Copy link
Collaborator Author

@shaunagm shaunagm Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, yeah. I think conventionally/aesthetically you minimize duplicate calls, but otoh we don't want to break things for people. Not sure which matters more.

`Args:`
entry_id:
The person's Action Network id
Expand Down Expand Up @@ -235,8 +256,8 @@ def update_person(self, entry_id, **kwargs):
https://actionnetwork.org/docs/v2/people#put
custom_fields:
A dictionary of any other fields to store about the person.
Updates a person's data in Action Network
"""
logger.warning("Method 'update_person' has been deprecated. Please use 'upsert_person'.")
data = {**kwargs}
response = self.api.put_request(url=f"{self.api_url}/people/{entry_id}",
json=json.dumps(data), success_codes=[204, 201, 200])
Expand Down