Fixes #38055 - Fix future-dated subscriptions #11243
Conversation
jeremylenz
changed the title
test/scenarios/scenario_test.rb
Outdated
| assert_equal 9, @org.product_contents.length | ||
| assert_equal 342, @org.products.length | ||
| assert_equal 9597, @org.product_contents.length |
There was a problem hiding this comment.
This is what made tests pass locally for me. But I am not sure if this test is correct or not. It does make sense that the number of imported products would vary now that I'm sending include=active, but I wouldn't expect numbers this big. I looked inside the fixture manifest files and didn't see 342 of anything..
There was a problem hiding this comment.
@qcjames53 is getting completely different numbers. Maybe it's a difference between what's in our test databases.
There was a problem hiding this comment.
The product length and content length assertions have a habit of changing every couple times I record VCR. I reset and seeded the test db on master and noticed the same behavior. So the good news is that this at least isn't a regression.
Thinking about this a little, I'm happy to push the test through since this import test is just for our manifest handling, not Candlepin's handling of our API calls. Even without the inconsistency it's a good practice to VCR candlepin's output.
jeremylenz
force-pushed
the
38055-product-all-active-include-what-now
branch
from
6d4255f to
2aec5eb
Compare
jeremylenz
force-pushed
the
38055-product-all-active-include-what-now
branch
from
2aec5eb to
8e2423d
Compare
jeremylenz
force-pushed
the
38055-product-all-active-include-what-now
branch
from
acca93a to
6c0d720
Compare
Co-authored-by: Partha Aji <[email protected]>
jeremylenz
force-pushed
the
38055-product-all-active-include-what-now
branch
from
6c0d720 to
4545dd4
Compare
There was a problem hiding this comment.
I was able to confirm that during manifest import the active=include string works on Candlepin version 4.4.19 and allows inclusion of future-dated subscriptions. As mentioned in another comment, the manifest import test seems to produce inconsistent results from Candlepin depending on the run; this behavior was observed in the existing test so it is not a regression. I asked the team for input on this and we deemed it okay because testing Candlepin API responses is outside of the scope of this test.
Jeremy, you're clear to merge after pushing the smaller recordings. ACKed
@sjha4 thank you for taking a look at this :)
What are the changes introduced in this pull request?
Problem 1:
Candlepin's
GET owners/{owner_key}/productsendpoint, called byKatello::Resources::Candlepin::Product.all, recently added a new param,active. When the param is not included, it apparently causes Candlepin to omit (some?) products with future-dated subscriptions. Sending the paramactive=includeshould restore the previous behavior. This affects Candlepin version 4.4.15 and above.Problem 2:
The "Add Subscriptions" page does not display future-dated subscriptions, e.g. subscriptions whose start date is in the future. Sending the
add_futureparam should fix that.Considerations taken when implementing this change?
I don't think this should break anything, since the param would just be ignored on previous CP versions.
What are the testing steps for this pull request?
Obtain a manifest with a future-dated subscription and no other subscriptions
Import into a new organization
Without this PR: Manifest import fails with
From another org with a valid manifest imported, the "Add Subscriptions" page does not show any future-dated subscriptions available to add.
After:
Manifest import succeeds
You can see any future-dated subscriptions on the Add Subscriptions page. Compare with access.redhat.com > Subscription allocations > your manifest > Add subscriptions. The available subscriptions in Satellite should include future-dated ones just like this page does.