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.

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

Basic support for hubspot workflows #102

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samanthamjohn
Copy link

Adding functionality to add a contact to a workflow by email.

@samanthamjohn
Copy link
Author

@cbisnett Would love a PR review when you get a chance. Thanks so much!

Copy link
Collaborator

@cbisnett cbisnett left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR, especially one with tests! I added a few comments for updates but hopefully this won't be too much work. If you don't have time or don't want to make the updates, let me know and I'll get them finished and merged. Thanks again.

end

# {https://developers.hubspot.com/docs/methods/workflows/add_contact}
def enroll_contact(email:)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the idea of named parameters but since we don’t use those anywhere else I think we should just keep this as a regular parameter.

cassette 'enroll_contact_in_workflow_success'
it "returns a 204 response for a success" do
email = "[email protected]"
workflow = Hubspot::Workflow.new({'name' => "hello workflow", 'id' => 2582322})
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests are great but the one problem I think we have here is that all of the ID's are hard coded. At this point the tests share an account with everyone on the internet using the demo API key. So if someone comes along and deletes these workflows or the demo account gets reset and there are no more views, then these tests will fail. We have theses issues occasionally with other tests. This isn't really a fault with your specific test but more an issue we have to work around with the tests.

I think the easiest way to fix this here is to create the workflow that can be used in each of these tests. I realize this means you'll have to add some more functionality to the Workflow class to be able to create workflows but it will mean the tests are more likely to pass consistently.

end
end

describe '.all' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this to describe '#all' to match the rest of the code base?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants