-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add tests for submission routes #1917
Add tests for submission routes #1917
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.
I love that this adds another test, so going to approve after minor fixes π
There's definitely more scope to test the different submission download types after, like CSV and geojson (this is in a different endpoint I think).
But I just wanted to comment that using basic auth via requests auth=
isn't great. I'm surprised that ODK Central accepts this & support could possibly be removed in the future. In osm-fieldwork we have wrappers for the Central API (as does pyodk). Both of those libraries handle Central authentication via the sessions API (request a session token, then use the token to authenticate).
It's still a good PR though! π«
@spwoodcock I wonder if i should simply make another api call to replace the basic auth with session authentication using: |
No need for that if this approach works π I think it would be more valuable adding
The submission could be created with the pyodk client, then more tests written for json CSV and geojson downloads =) I'll leave you to assess how much time this may take though and if it's worth it. Requirements relates to Tokha could be addressed instead if this will take a long time π Either way is fine! |
0b25603
to
f1b1831
Compare
I haven't included the .pyodk_config.toml file in tests directory. |
As the credentials are only used for testing, it's ok to commit that file. I think we need it for the tests to work (pyodk doesn't accept environment variables unfortunately π₯²) |
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.
This is excellent, well done!
Two tiny changes and all good to merge
Sorry Anuj! I didn't mean the dev server credentials! The credentials you can use are for the locally installed version of ODK Central, which always has user |
5b92bf3
to
f6694ae
Compare
Oops my bad. |
What type of PR is this? (check all applicable)
Related Issue
Example: Resolves #1913
Describe this PR
A fixture for submission has been created and some tests have been added for submission routes.
Checklist before requesting a review