Skip to content
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

Fixes #308 Include tenant-id when using files api (#1) #346

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

kedmenec
Copy link
Contributor

Update the files api manager to include tenant-id in the header - previously would get a 403 using this api.
Updated readme command for running tests.
Added more params for the uploading file so its more inline with existing api and works with different backends (eg S3)

kedmenec and others added 3 commits October 30, 2023 09:43
* Work and test

* less change

* less

* Use file objects and update readme

* Feedback

* updates

* tests

* remove

* less

* cleaner test

---------

Co-authored-by: Timbo <[email protected]>
Copy link
Owner

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good; however, the bare exception handling in the tests needs to be cleaned up.

r_get.return_value = None
try:
xero.filesAPI.files.all()
except: # NOQA: E722
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a red flag - bare exceptions are flagged by flake8 for a reason; and in this case, I can't see a reason we should be raising an exception at all. As best as I can make out, we just need a more realistic mock return value.

Copy link
Owner

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@freakboy3742 freakboy3742 merged commit 03e836b into freakboy3742:main Nov 3, 2023
6 checks passed
@kedmenec
Copy link
Contributor Author

kedmenec commented Nov 3, 2023

Thanks for reviewing and merging so quickly. Any idea when the next release will be?

@freakboy3742
Copy link
Owner

There's no reliable release cadence; it's more a case of "when someone asks, or when there's enough of a backlog, whichever happens first" :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants