Action Builder connector #826
Conversation
ydamit
added
high priority
There was a problem hiding this comment.
Exciting! Seems like a killer connector.
A few places where I think we could make the API a little more user friendly and want to make sure the structuring of tags doesn't lead to unexpected results. And then would love to see if there's a way to simplify one of the tests.
|
|
||
| params = {"page": page, "per_page": per_page, "filter": filter} | ||
|
|
||
| campaign = self._campaign_check(campaign) |
There was a problem hiding this comment.
nit: usually best to do validations that throw first thing in the function. Also, seems like campaign is a required arg. Seems like it should be optional
|
|
||
| return self.api.get_request(url=url, params=params) | ||
|
|
||
| def _get_entry_list( |
There was a problem hiding this comment.
this makes me think of a list of an object called an entry. Perhaps _get_all_records or if entry is actually a word that AB uses, maybe _get_all_entries
There was a problem hiding this comment.
Good point. I copied it pretty much wholesale from the Action Network connector, so we may eventually want to change that one, too.
| return_list = [] | ||
|
|
||
| # Keep getting the next page until record limit is exceeded or an empty result returns | ||
| while True: |
There was a problem hiding this comment.
Long term, I'm curious if we could draw on some pagination libraries. Seems like this is probably pretty common logic
There was a problem hiding this comment.
No objection from me, just so long as they can handle OSDI weirdness!
|
|
||
| return self.api.post_request(url=url, data=json.dumps(data)) | ||
|
|
||
| def upsert_entity( |
There was a problem hiding this comment.
My instinct is to make this an internal function and have the public api specify which entity is being updated. Might make the function validation simpler as well
There was a problem hiding this comment.
I agree, though perhaps a little grudgingly. Both Action Network and Action Builder use a common endpoint for inserting and updating: the signup helper. Action Network has much simpler upsert logic, because records are unique on additional data: email and/or mobile number. Action Builder has no such uniqueness, so you're right that these should be separate. I'm just grumbly because I need to make sure this change is reflected in the sync that's live, but that's on me for running a sync on branch like that, before the connector is even merged.
There was a problem hiding this comment.
I could also see an argument for both. Having convenience methods like upsert_person (or whatever it would be) that are just syntactic sugar on the more general upsert_entity. But I do think having the more semantically clear parts of the API is important
There was a problem hiding this comment.
I've split it into insert_entity_record() and update_entity_record(), each with their own validation and tests. I think that's probably as far as we should go in this pass, but am definitely open to introducing _person() versions in future updates that hard-code the entity type to the one that's guaranteed present in any Action Builder setup.
There was a problem hiding this comment.
oh interesting. we may have missed each other. I think upsert is a better public api to use than an insert/update split. I was suggesting the _person, _campaign versions.
we might be reaching the limits of asyncronous review and may need to grab some time
|
|
||
| return self.upsert_entity(identifiers=identifiers, data=data, campaign=campaign) | ||
|
|
||
| def upsert_connection(self, identifiers, tag_data=None, campaign=None): |
There was a problem hiding this comment.
unless there's an insane number of entities and therefore types of connections, would also lean towards having a more explicit public api of like upsert_person_campaign_connection and using this as a private method to keep things dry
There was a problem hiding this comment.
I'm not sure I completely understand the suggestion here. Just in case there's any misunderstanding about connections and how they're applied, I'll try to summarize here.
Connections can exist between any two entity records, regardless of whether the entity type is person or not. The system is strict about there only ever being one connection record between two specific entity records, and the connection helper endpoint is pretty much the only way to create or update connections (other than the signup helper, which doesn't really work differently in this regard). So you don't actually ever know for sure (unless you make wasteful additional API calls) whether you're inserting or updating.
Connection records can accept tags, much the same way as entity records. These can help illuminate important information about the relationship between entities. For instance, between two people records, you could have a tag indicate the human relationship between them, or on a connection between a person and an organization, you could have one that indicates the person's role at that organization.
Given all this, I have a hard time imagining what public method we could write that would be more intuitive than this one, and use this one as internal. But very open to anything I'm just not seeing!
| from test.utils import assert_matching_tables | ||
|
|
||
|
|
||
| class TestActionBuilder(unittest.TestCase): |
| self.fake_field_1 = "Fake Field 1" | ||
| self.fake_section = "Fake Section 1" | ||
|
|
||
| self.fake_tags_list_1 = { |
There was a problem hiding this comment.
this setup gets pretty involved. you might consider some generator functions that accept args and spit out these more complicated objects
There was a problem hiding this comment.
You're probably right, but I'm hoping it's ok to save that for the next version of updates to this connector.
There was a problem hiding this comment.
It's worth thinking about creating some general guidance for implementing factory functions to create test data (and maybe even make some default factory functions that create common objects that people can subclass/build off of). Right now most connectors rely on creating json or python dicts, in the tests like Yotam's done here or imported from separate files, and either way it's messy and tedious to write and generally not ideal.
But factory methods are a relatively advanced python concept so we haven't wanted to say "just do this" without giving contributors support. If we can make some good guidance/templates and some default classes, we could slowly transition over connectors (especially highly used connectors, and new connectors) to use factory functions and that would probably be better for the project/community overall.
Edit: moved this to an issue (#835) where it won't get lost (or block/confuse this PR]
| return incoming, compare | ||
|
|
||
| @requests_mock.Mocker() | ||
| def test_upsert_entity(self, m): |
There was a problem hiding this comment.
there's a lot going on in this test along with the compare_to_incoming. I'm not sure I really follow what's happening so might be easier to grab 10 min and talk through it
There was a problem hiding this comment.
Happy to do that. In a nutshell, though, compare_to_incoming() is just trying to make sure to exclude out any keys (including in nested dicts) from an incoming dict that don't also appear on the existing dict to which the incoming dict is to be compared. In other words, this if an incoming dict has extra keys anywhere, don't include them in the comparison.
|
|
||
| @requests_mock.Mocker() | ||
| def test_add_tags_to_record(self, m): | ||
| m.post(f"{self.api_url}/people", json=self.tagging_callback) |
There was a problem hiding this comment.
interesting! I've not used requests_mock. is this mocking the json method? what is the request param that the tagging_callback is receiving?
There was a problem hiding this comment.
I'm also just wrapping my head around it. The idea as I understand it is to intercept specific HTTP requests and force a desired response. This is done by setting any of the properties available in a requests response object. So you'll notice that most of the tests above just return a text, and to fit that expectation, it's json.dumps() applied to whatever Python object was prepared for the purpose of that test.
In more complicated cases such as this test, we can pass request (the details of the request) and context (the details of the response) to a callback function, to return something that requires more processing. Specifically here, we're extracting just the list of dicts that should appear under the add_tags key of the top-most dict in the request data, and returning that to compare to what we expect that tagging data to look like.
|
@willyraedy , I think I've incorporated everything we discussed today. Let me know, though, if I left anything out or if you notice anything new that requires attention! |
|
Hi @willyraedy, it looks like yotam made some changes par your comments. Wondering if you had any other suggestions for him for review! |
|
Hi @willyraedy! Just wondering if there are too many things on your plate at the moment, if I should just go over this PR with Kasia tomorrow during our pairing! Thanks! |
|
@sharinetmc Looking at it now! |
|
Looks like this is ready to merge, so I'm going to merge it. Thank you for the contribution @ydamit! |
A very initial submission of a class for an Action Builder connection. Woefully bare bones, this is the minimum viable product necessary for a working Action Builder template sync in the TMC environment.