Define a separate method for form submission

[amitfriedman12]

The Hubspot::Form#submit method only returns boolean values. True for successful submission and false for an unsuccessful submission.

It is useful however to be able to view the actual HTTParty::Response object directly in order to get more information in case of submission failure.

Therefore i extracted the actual form submission into a separate public method that returns the HTTParty::Response object itself. This change does not modify existing functionality in any way.

[SViccari]

@amitfriedman12 Thank you for submitting a PR 🌟 . I agree that it would be helpful to return the HubSpot API response instead of a boolean 👍

Instead of returning the HTTParty::Response object, we can return an object that we own. For example, PR #171 introduces Hubspot::Response with the intent of giving users access to the response body. With this change, instead of adding a new function like submit_response, we can update submit to return a Hubspot::Response.

For additonal context, there's a project board that focuses on the work to be done for a v1 release.

[amitfriedman12]

@SViccari since this is a breaking change, what would be the best way to follow its release? Just follow the project board?

[SViccari]

@amitfriedman12 Following the project board is the best course of action. I've added this concern as an issue and added a ticket to the project board to ensure it's addressed. The v1 release won't be ready for a while but once PR #171 is approved/merged, we can update this method to return a Hubspot::Response and you can use the master branch to pull in the latest changes. (although, just be wary that master will still receive breaking changes as we strive to release v1, which is focusing on making each method's return value consistent.)

If you need this change right away, I'd recommend forking this project and adding the change you need so you're not blocked by our work.

[cbisnett]

What additional data are you looking to get from the response? Based on the documentation it doesn't appear there is any other data than the HTTP status code.

204 when the form submissions is successful 302 when the form submissions is successful and a redirectUrl is included or set in the form settings. 404 when the Form GUID is not found for the provided Portal ID 500 when an internal server error occurs

For API methods like this I think we should have two ways of calling the method:

def submit
  ... do submit ...
end

def submit!
  submit() || raise(RequestError.new("Failed to submit the form"))
end

This way these methods can be called from handlers like:

if form.submit
  ... success ...
else
  ... failure ...
end

And from background tasks like: form.submit! so failure gets bubbled up to the background runner. This is basically what @SViccari recommended.

[SViccari]

@cbisnett Based on the PR description, I believe the goal is see the status code when the request fails. It's not great feedback from the API but it's a little more helpful than just returning false.

In regards to defining one method that raises and one method that doesn't raise, what do you think of just updating the current function submit! to raise? I'm not typically a fan of exception based control flow but I'm onboard with narrowing our focus as we strive to make all the existing functionality consistent for v1. Then, we can add functions that don't raise, giving users the option to call submit or submit!.

On a side note, the HubSpot API docs mention a v3 form endpoint that looks promising as it does return helpful errors. So in a future PR, we can migrate to this endpoint and the raised error would include more details about the failed request.

[cbisnett]

OK that makes sense that a developer would want to know what happened from the return code rather than just that it succeeded or failed. I still think the best solution is to return a success boolean but also provide an interface for retrieving errors that is consistent across all of the Hubspot resource classes. This way you can still use regular control flow to test success or failure, and provide some feedback to the user using the error messages. For some API endpoints error strings are returned, unfortunately for this one, we would have to translate the HTTP status code into an error string, but that's not hard to do.

I'm not suggesting anyone should use exceptions for control flow as that's not a good pattern. What is common though is to be able to use bang methods in cases where a developer may want the failure of a API call to rollback a transaction or cause a background task to explicitly fail. For background tasks, there isn't a way to notify someone that it's failed because it's being run automatically. You can log the failure but that's not ideal because it requires someone to see the failure in the log. Every background job framework I've ever worked with (DelayedJob, Resque, Sidekiq, etc.) has some concept of a dead letter queue where failed tasks get placed so the failure can be investigated and retried or cleared. This is the most common reason why developers use bang methods in background tasks because it causes the framework to move the job to the dead letter queue on failure.

I hadn't seen Hubspot was planning any v3 endpoints. I should reach out to their developers and project managers with feedback on the existing endpoints and all the inconsistencies.