Subscription Cancel With Message #40

Merged
merged 1 commit into from Aug 20, 2014

Projects

None yet

4 participants

@carsonreinke
Contributor

No description provided.

@hedgehog

Does this need to be be adjusted to allow for JSON format, or is it immaterial?

Owner

Well, if so, looks like this method is wrong also (unless I am missing something):

def charge(attrs = {})
  post :charges, {}, attrs.to_xml(:root => :charge)
end
@speric
Member
speric commented Nov 12, 2013

@carsonreinke I know it's been a while, but are you still interested in having this merged in? If so, can you add some tests?

@carsonreinke
Contributor

@speric Sure thing, I'll add some tests.

@jeremywrowe jeremywrowe commented on the diff Aug 14, 2014
lib/chargify_api_ares/resources/subscription.rb
@@ -13,8 +13,13 @@ def save
super
end
- def cancel
- destroy
+ def cancel(cancellation_message = nil)
+ if cancellation_message.nil?
+ destroy
+ else
+ #Destory does not support body, must work around it to send verb DELETE
+ self.connection.post(element_path, {:cancellation_message => cancellation_message}.to_xml(:root => :subscription), self.class.headers.merge({'X-Http-Method-Override' => 'DELETE'}))
@jeremywrowe
jeremywrowe Aug 14, 2014 Contributor

You can pass a params hash to the delete method. See here http://api.rubyonrails.org/v2.3.8/classes/ActiveResource/Base.html#M001263

@carsonreinke
carsonreinke Aug 14, 2014 Contributor

Only via a query string or path, not the actual body, which is needed to provide the cancellation message, unless something changed for this: https://docs.chargify.com/api-subscriptions.

@jeremywrowe
jeremywrowe Aug 14, 2014 Contributor

Correct. I am suggesting using params: { cancellation_message: "Migrating to a more expensive plan" } as the second parameter to the delete method. This should add the query parameter cancel_message. I would prefer not doing a post and overriding the method type.

@carsonreinke
carsonreinke Aug 14, 2014 Contributor

Oh well yes, if Chargify now will take the query string parameter instead, that would be the easiest. Could be an issue with lengthy cancellation messages though, not sure if Net::HTTP has any length limits.

@jeremywrowe
jeremywrowe Aug 20, 2014 Contributor

After further thought, let's just go with your implementation.

@jeremywrowe jeremywrowe merged commit 8bc616c into chargify:master Aug 20, 2014
@carsonreinke
Contributor

Thanks, sorry, just was too lazy to ever write a test for this.

@jeremywrowe
Contributor

No problem, I wouldn't say you are being lazy re tests. Right now it is hard for a non Chargify employee to write tests since the testing environment assumes some things. I plan on changing that. Let me know if there are any other pain points in the gem. I am gathering a list of things to do and making sure that integration and contributing becomes easier and easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment