Add fetching additional DataSource fields#122
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for fetching additional fields in DataSource API operations by introducing three new optional query parameters: with_processing_status, with_auto_churn_subscription_setting, and with_invoice_handling_setting.
Key changes:
- Updated
DataSourceclass to support new optional fields in responses - Added schema definitions for processing status and invoice handling settings
- Updated tests to validate the new fields are correctly handled
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| chartmogul/api/data_source.py | Added nested schema classes for new fields and updated the _many namedtuple to include the new query parameters |
| test/api/test_data_source.py | Extended test cases to include new optional fields in mock responses and assertions |
| README.md | Updated documentation examples to demonstrate usage of new query parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
566b7d3 to
068a708
Compare
068a708 to
49afff3
Compare
chartmogul/api/data_source.py
Outdated
| _root_key, | ||
| "with_processing_status", | ||
| "with_auto_churn_subscription_setting", | ||
| "with_invoice_handling_setting" |
There was a problem hiding this comment.
[question] How are these arguments used? Shouldn't they be passed somewhere?
There was a problem hiding this comment.
I tried to test it, but couldn't receive the newly added fields (I was connecting with production API).
Here's what I did:
>>> from chartmogul import Config, Ping, DataSource
>>> config = Config('<key>')
>>> Ping.ping(config).get()
<Ping{data='pong!'}>
>>> DataSource.retrieve(config, uuid='ds_587e0972-95cb-11e6-b165-3f128ee5ed8f', with_processing_status=True).then(lambda ds: print(ds))
<DataSource{created_at=datetime.datetime(2016, 10, 19, 7, 12, 7, 661000, tzinfo=datetime.timezone.utc), name='ChartMogul trials', status='idle', system='Custom', uuid='ds_587e0972-95cb-11e6-b165-3f128ee5ed8f'}>
<Promise at 0x7f1578e473b0 fulfilled with None>
>>> DataSource.retrieve(config, uuid='ds_587e0972-95cb-11e6-b165-3f128ee5ed8f', with_processing_status=True).then(lambda ds: print(ds.processing_status))
<Promise at 0x7f1578bfc8c0 rejected with AttributeError("'DataSource' object has no attribute 'processing_status'")>
>>> DataSource.all(config, with_processing_status=True).then(lambda ds: print(ds[0][0].processing_status))
<Promise at 0x7f1578a8c5f0 rejected with AttributeError("'DataSource' object has no attribute 'processing_status'")>
Note that I am not really familiar with this SDK and was just following the README.
Could you please share your testing instructions?
See chartmogul/resource.py#L110-L115
this is not on production yet, you can test against alpha-yellow |
|
@wscourge Still no luck, maybe alpha-yellow was reset? Perhaps it'd be best if I review & test it once you deploy the API changes in production? It should be safe, since they are backwards-compatible. |
|
@loomchild thanks for your test 🙏, I pushed one more commit fixing this behaviour - please take a look now. |
loomchild
left a comment
There was a problem hiding this comment.
LGTM.
I tested on alpha yellow and everything looked correct.
I also tested in production, but I got the following error for some params. Please confirm this is expected:
>>> DataSource.all(config, with_processing_status=True, with_auto_churn_subscription_setting=True,with_invoice_handling_setting=True).then(lambda ds: print(ds))
<Promise at 0x7ad664c407d0 rejected with ValidationError({1: {'invoice_handling_setting': {'update_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_refunded': ['Unknown field.'], 'create_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_written_off': ['Unknown field.'], 'prevent_subscription_for_invoice_voided': ['Unknown field.']}}, 3: {'invoice_handling_setting': {'update_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_refunded': ['Unknown field.'], 'create_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_written_off': ['Unknown field.'], 'prevent_subscription_for_invoice_voided': ['Unknown field.']}}, 6: {'invoice_handling_setting': {'prevent_subscription_for_invoice_written_off': ['Unknown field.'], 'prevent_subscription_for_invoice_voided': ['Unknown field.'], 'create_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_refunded': ['Unknown field.']}}, 7: {'invoice_handling_setting': {'prevent_subscription_for_invoice_written_off': ['Unknown field.'], 'prevent_subscription_for_invoice_voided': ['Unknown field.'], 'create_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_refunded': ['Unknown field.']}}, 8: {'invoice_handling_setting': {'prevent_subscription_for_invoice_written_off': ['Unknown field.'], 'prevent_subscription_for_invoice_voided': ['Unknown field.'], 'create_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_refunded': ['Unknown field.']}}, 9: {'invoice_handling_setting': {'update_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_refunded': ['Unknown field.'], 'create_subscription_when_invoice_is': ['Unknown field.'], 'prevent_subscription_for_invoice_written_off': ['Unknown field.'], 'prevent_subscription_for_invoice_voided': ['Unknown field.']}}})>
| else: | ||
| del params[query_param] | ||
|
|
||
| return params |
There was a problem hiding this comment.
[for later] This code is pretty generic, so we could move it to Resource.
Maybe even there's no need for bool_query_params variable and we can rely only on type, but this would be a bit riskier, since you'd have to assure that other resources and params like has_more still work.
Thank you for testing @loomchild, it is not expected. I spoke with Dijana and we agreed that the I updated it via the last commit to essentially accept any JSON value - please take one more look, thank you 🙇 |
Updates the List Sources and Retrieve a Source to support requesting additional fields with the following query params:
with_processing_status=truewith_auto_churn_subscription_setting=truewith_invoice_handling_setting=trueUsing SDK code:
1) List Sources
2) Retrieve a Source