-
Notifications
You must be signed in to change notification settings - Fork 128
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
[V2] Add starting_on and ending_on to subscription ramps #858
Conversation
spec/recurly/subscription_spec.rb
Outdated
@@ -76,11 +76,15 @@ | |||
ramp_intervals: [ | |||
SubscriptionRampInterval.new( | |||
starting_billing_cycle: 1, | |||
unit_amount_in_cents: 1000 | |||
unit_amount_in_cents: 1000, |
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.
I tried to implement similar testing to what is being done here, but it's not really the same because it looks like the Python version is making a request based on the fixture file while the Ruby client is doing the test set up in the before block and just using the fixture file for comparison. I could be interpreting that incorrectly. Is there another way to test the addition of these fields, or is this enough?
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.
So your read is right, putting the test here he a little odd, since this test is basically testing that a Subscription with ramps serializes to an XML format that could be used to say, make a POST request.(notice how remaining_billing_cycles is not in the XML, since it is not something included in a request, only in a response)
The thing we're implementing is 'Can read starting_on and ending_on from an xml response` I think a correct test would be that you can read the attributes from an XML response, so something like the following
sub - Subscription.from_xml(get_raw_xml 'fixture-you made')
expect(sub.ramp_intervals.first.starting_on).to eq(date_from_fixture)
expect(sub.ramp_intervals.first.ending_on).to eq(date_from_fixture)
expect(sub.ramp_intervals.second.starting_on).to eq(date_from_fixture)
expect(sub.ramp_intervals.secong.ending_on).to eq(nil)
That said.. it seems a little redundant to test these, since we already have tests for resource.rb. I do think I would lean towards a test like this though, since these changes don't require any PG changes. Im pretty sure if you take those two lines around starting_on
and ending_on
out of define_attribute_methods
the test would fail.
I think I would write this test. While doing so, I would also put an expectation for remaining_billing_cycles
and add it to the fixture and see if it fails without that method in define_attribute_methods
`
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.
I reverted the fixture from the original test back to what it was and created a new fixture file for this one. Confirmed that removing starting_on
and ending_on
out of define_attribute_methods
causes the test to fail. Added expectations for starting_billing_cycle
and unit_amount_in_cents
that also fail if those attributes are removed.
7051f31
to
47c99c4
Compare
e01f8c2
to
13d2661
Compare
before do | ||
attributes.merge!( | ||
ramp_intervals: [ | ||
SubscriptionRampInterval.new( | ||
starting_billing_cycle: 1, | ||
unit_amount_in_cents: 1000, | ||
starting_on: '2022-09-23 16:16:34.000000', | ||
ending_on: '2022-10-23 16:16:34.000000' | ||
), | ||
SubscriptionRampInterval.new( | ||
starting_billing_cycle: 2, | ||
unit_amount_in_cents: 2000, | ||
starting_on: '2022-10-23 16:16:34.000000', | ||
ending_on: nil | ||
) | ||
] | ||
) | ||
end |
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.
clarification: What is this doing? If we're getting the sub from_xml
below, why do we need these ramp_interval_attributes?
@@ -0,0 +1 @@ | |||
<subscription><account><account_code>1</account_code><billing_info><month>1</month><number>4111-1111-1111-1111</number><year>2014</year></billing_info><dunning_campaign_id>1234abcd</dunning_campaign_id><email>[email protected]</email><first_name>Verena</first_name><last_name>Example</last_name></account><currency>EUR</currency><customer_notes>Some Customer Notes</customer_notes><imported_trial>true</imported_trial><plan_code>gold</plan_code><ramp_intervals><ramp_interval><ending_on>2022-10-23 16:16:34.000000</ending_on><starting_billing_cycle>1</starting_billing_cycle><starting_on>2022-09-23 16:16:34.000000</starting_on><unit_amount_in_cents>1000</unit_amount_in_cents></ramp_interval><ramp_interval><starting_billing_cycle>2</starting_billing_cycle><starting_on>2022-10-23 16:16:34.000000</starting_on><unit_amount_in_cents>2000</unit_amount_in_cents></ramp_interval></ramp_intervals><shipping_address_id>1234</shipping_address_id><shipping_amount_in_cents>899</shipping_amount_in_cents><shipping_method_code>ups_ground</shipping_method_code><terms_and_conditions>Some Terms and Conditions</terms_and_conditions></subscription> |
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.
Technically the second ramp should have <ending_on nil="nil"></ending_on>
to match an actual v2 api response.
Also, to check remaining_billing_cycles
, in the below spec you would add <remaining_billing_cycles type="integer">0</remaining_billing_cycles>
to the first ramp, and <remaining_billing_cycles nil="nil"></remaining_billing_cycles>
to the second ramp
@@ -5,6 +5,8 @@ class SubscriptionRampInterval < Resource | |||
define_attribute_methods %w( | |||
starting_billing_cycle | |||
unit_amount_in_cents | |||
starting_on |
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.
Sweet, one quick ask( maybe I was unclear in my other comments) it looks like remaining_billing_cycles
should be in define_attribute_methods
and was missed originally. Could you add it here and in the fixture/expectation
|
||
expect(sub.ramp_intervals[0].starting_on).must_equal('2022-09-23 16:16:34.000000') | ||
expect(sub.ramp_intervals[0].ending_on).must_equal('2022-10-23 16:16:34.000000') | ||
expect(sub.ramp_intervals[0].starting_billing_cycle).must_equal('1') |
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.
expect(sub.ramp_intervals[0].remaining_billing_cycles).must_equal('0')
expect(sub.ramp_intervals[0].unit_amount_in_cents).must_equal('1000') | ||
|
||
expect(sub.ramp_intervals[1].starting_on).must_equal('2022-10-23 16:16:34.000000') | ||
expect(sub.ramp_intervals[1].ending_on).must_equal(nil) |
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.
expect(sub.ramp_intervals[1].remaining_billing_cycles).must_equal(nil)
[Full Changelog](2.19.7...2.19.8) **Merged Pull Requests** - Fixed usage of Minitest [#862](#862) ([csampson](https://github.com/csampson)) - [V2] Add starting_on and ending_on to subscription ramps [#858](#858) ([rlew421](https://github.com/rlew421))
Added
starting_on
andending_on
to subscription ramps