-
Notifications
You must be signed in to change notification settings - Fork 9
Add disabled, disabled_at, edit_history_summary and errors fields to Invoice #126
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
base: main
Are you sure you want to change the base?
Add disabled, disabled_at, edit_history_summary and errors fields to Invoice #126
Conversation
45cc3fb to
b21f44a
Compare
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 test coverage for query parameter handling in invoice retrieval operations. The tests verify that the retrieve() and all() methods correctly accept and pass query parameters like validation_type, include_edit_histories, and with_disabled to the API.
Key Changes
- Added tests for single query parameter (
validation_type) handling in bothretrieve()andall()methods - Added comprehensive tests verifying multiple query parameters are correctly passed in API requests
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…bled_by to Invoice
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.
Code looks OK, I have some minor comments.
But I don't know how to test it and whether adding extra fields was intentional.
| disabled_at = fields.DateTime(allow_none=True) | ||
| disabled_by = fields.String(allow_none=True) | ||
| edit_history_summary = fields.Dict(allow_none=True) | ||
| errors = fields.Dict(allow_none=True) |
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 there's a functional change in addition to adding tests? Could you clarify here or in title/description.
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.
Thanks for pointing this out, I titled the draft PR when it was WIP and then realised it is going to need this changes too.
Adding these extra fields is intentional.
test/api/test_invoice.py
Outdated
| {"validation_type": ["all"]}, | ||
| ) | ||
|
|
||
| # Struct too complex to do 1:1 comparison |
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.
[minor] Remove unnecessary comment (here and below). Or perhaps there's a way to compare only parts of structures (similar to RSpec match?
test/api/test_invoice.py
Outdated
| json=retrieveInvoiceExample, | ||
| ) | ||
|
|
||
| config = Config("token") # is actually checked in mock |
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 don't understand the comment - what is checked in mock and what does checking in mock mean? Please clarify or remove.
pkopac
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.
Please address @loomchild 's comments.
Testing instructions: