nateleavitt / infusionsoft

Ruby Gem for the Infusionsoft API
MIT License
58 stars 70 forks source link

Bug? ... InvoiceService.addRecurringOrder #58

Closed jaredjackson closed 7 years ago

jaredjackson commented 7 years ago

The method invoice_add_recurring_order doesn't seem to work in the master branch due to the fact that not all the parameters that are listed as required under the Infusionsoft API docs are present in this method.

The missing parameters are: "qty", "price", "allowTax".

My assumption may be off, but either this method doesn't work correctly (because the required params are missing) or those three items aren't actually required parameters. Not sure if anyone else uses the method as is and without issue.

Regardless, I have been using a modified method to support all of the required parameters and the transactions are properly submitted via the api. When I revert back to using the master branch (nateleavitt/infusionsoft), the transactions fail.

This may be due to the fact that my application allows for variable pricing in the recurring order.

Question: would this be a case where the method could be modified to fully support the API docs, or would it be better to add a second method like, invoice_add_custom_recurring_order ... to avoid breaking some application that is currently using it successfully as is?

TravisSperry commented 7 years ago

This had worked for us before in the past with no error so I'm not sure what change caused the error. In our case, the sub gets created even though the unexpected error is thrown.

jaredjackson commented 7 years ago

Yes, I have a branch with the only change being the fix the the invoice_add_recurring_order method.

But I'd like to hear from Nate when he gets a chance to know if he has any insight on how the change might impact current users. If it would be too much of a change, I'd create a different branch adding a custom method for full API support.

jaredjackson commented 7 years ago

Here is the branch.

https://github.com/jaredjackson/infusionsoft/tree/fix/invoice-add-recurring-order

TravisSperry commented 7 years ago

Cool thanks. I can wait. I have a workaround for now. Like I said, the subscription is created even though the error is thrown, so, for now, I'm rescuing and redirecting :/ Making everything appear normal.

nateleavitt commented 7 years ago

Hmm... not sure. I'll check out the branch.

nateleavitt commented 7 years ago

Interesting.. the API must have been changed. I know that this method had been used before without error. Let me do some additional testing. If the error is thrown regardless I think the only way to proceed is to update the method. Deprecation warnings from the Infusionsoft API would be nice :)

jaredjackson commented 7 years ago

It's weird because I've had the same experience as Travis ... Infusion API throws an error but the subscription is still created. I think it is safe to say that something is not quite right on their end, but regardless, the error doesn't seem to occur when the method uses the required params as documented on the Infusion site.

TravisSperry commented 7 years ago

@jaredjackson Are you running on Rails 5?

jaredjackson commented 7 years ago

Yes ... 5.0.2 ... with Ruby 2.3.1

TravisSperry commented 7 years ago

Rails 5/Ruby 2.3.1 could be the source of the issue.Why? I'm not sure. I'm running the same. I know I had to make some updates to how params were handled but that's not an issue with this call. I didn't have issues with a previous version.

jaredjackson commented 7 years ago

I'll test some different rails apps and ruby versions when I get a chance later today.

jaredjackson commented 7 years ago

For what it's worth, Rails 4.2.8 allows me to successfully add a recurring order via the Infusionsoft gem as is without error. So that is a difference between Rails 4.x and 5.x ... not sure why the difference. It may be deeper in the stack of required gems ...

However, since the Infusionsoft API is stating the other params (qty, price, taxable) are required ... and on a personal level since I use a variable pricing scheme ... I'd suggest the gem be updated to reflect the current API docs.

Still, to prevent breakage on current 4.x apps, I might suggest that a second method that fully supports the API be added with a deprecation warning on the current method. Sooner or later the method will need to be updated as Rails progresses. Any thoughts @nateleavitt, @TravisSperry ?

jaredjackson commented 7 years ago

UPDATE .... I just tested another fresh install with Rails 5.0.3 and successfully created a new recurring order with the current Infusionsoft gem (unmodified). So maybe more testing with other versions of Rails to nail it down. 5.0.0.1 works, too.

jaredjackson commented 7 years ago

I think at this point, my theory is that Infusionsoft infers the qty, price, and taxable params from the subscription plan. So even though the API docs say those are required in the API call, the IS side is supplying those values if they aren't submitted through the API call. Still digging.

nateleavitt commented 7 years ago

@jaredjackson I like your idea of updating the current call with a deprecation warning, and adding a second method to reflect the current Infusionsoft API docs. I really want to do a rewrite of this gem.. but it's on my long to-do list :P

TravisSperry commented 7 years ago

@jaredjackson I will try bumping to 5.0.3. Thanks!

jaredjackson commented 7 years ago

In this branch ... https://github.com/jaredjackson/infusionsoft/tree/fix/invoice-add-recurring-order

I kept the original method and added a deprecation warning. Then the new replacement method. Also added tests/VCR cassettes.

nateleavitt commented 7 years ago

Awesome! Created a pull request from your branch: https://github.com/nateleavitt/infusionsoft/pull/60

nateleavitt commented 7 years ago

This has now been merged and deployed in the newest version of the gem.