-
Notifications
You must be signed in to change notification settings - Fork 9
Add fetching additional DataSource fields #122
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
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] How are these arguments used? Shouldn't they be passed somewhere?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 🙇 |
loomchild
left a comment
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.
LGTM, sorry for delay.
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