-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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, remove recipients #72
Conversation
@@ -460,3 +509,4 @@ def get_template(self, templateId): | |||
.format(accountId=self.account_id, | |||
templateId=templateId) | |||
return self.get(url) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be why flake8 fails
Nice work ! Is it possible to add some tests ? |
data = { | ||
'signers': [{'recipientId': x} for x in recipientIds] | ||
} | ||
return self.delete(url, data=data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple test would be to check all that formatting:
def test_delete_envelope...(self):
client = ...
with patch('DocusignClient.delete') as delete:
client.delete_enveleop_recipients(...)
This way we have coverage of this unit of code. The integration with docusign process is another test strategy that could be done with in-house integration plateform.
@smcoll I planned to release the 1.0 in few days, do you want to change something on this PR, or at least rebase it on master, just to be sure ? |
closed in favor of #93 |
Here is a proposal for #70. i wish that the
add_envelope_recipient
shortcut would return a JSON object for the one recipient, but given the way DocuSign returns a response of all recipient types, i thought that might get a little wonky.