Add client.submissions.edit to edit an existing submission #19
Conversation
There was a problem hiding this comment.
The edit method looks great. I like the validation, error checking and types. 👍
I have some questions and suggestions around some of the other methods introduced. I'm really glad we have the generic HTTP verb methods and I think that for our designed methods we should try to move away from those and take the opportunity to be more descriptive.
pyodk/endpoints/submissions.py
Outdated
| def post( | ||
| self, | ||
| xml: str, | ||
| form_id: Optional[str] = None, |
There was a problem hiding this comment.
I'm surprised to see form_id optional here and on other signatures here. Was this maybe left over from the manager-based chaining?
There was a problem hiding this comment.
More or less, yeah. It's a similar idea as project_id, where you could set a default on the Service class instance, then that'd be used as a default for any calls using that instance. The parameter validation checks that either the default or an input is set. Please let me know what you'd like to change if anything.
There was a problem hiding this comment.
When we discussed this item, @lognaturel was going to consider whether this was useful to keep in or not. So for now it is still in.
There was a problem hiding this comment.
I have no strong feelings about this so let's leave it in.
|
@lindsay-stevens and I discussed and the next step is to only document and expose |
- add client.submissions.edit to update submission data and
optionally add a comment at the same time.
- validation improvements:
- use pydantic validators instead of writing the same.
Also benefits by returning a type-cast value if it was
possible to do so, e.g. "1" is a castable number so return 1.
- instead of checking 200 response status only, use the
built-in "raise_for_status" for use with all verbs, which
checks for status codes >=400 and <600.
- switched from sandbox to staging for fix for the Comments.post response (no actorID, comment as array). It also now returns a createdAt like Comments.list does, so add that to the expected data model. - Updated integration test UUID for new test data in staging.
- mark most modules as internal (underscore prefix), and specify public code in __all__ - users can still import but these are the PEP8 conventions - addresses feedback about what the public API of pyodk is / should be. - rename Submissions HTTP method names to edit/review - make client.comments private and instead put Comment methods on Submissions - addresses feedback around naming / code organisation
lindsay-stevens
force-pushed
the
pyodk-8
branch
from
67c481a to
7c85442
Compare
- move __version__ to a separate file and read/exec the file at install - also fixes import chain requiring inline import in session.py - problem was ultimately the __init__ import of Client > session > toml breaks install because when executing setup.py, toml potentially isn't installed yet.
- submissions.py: - put submission.create's device_id kwarg through string validator - use prefix variable to simplify URLs list - use _default_kw() and fp_ids dicts to simplify handling form_id/project_id - validators.py: - in wrapper func, raise for any type error (e.g. int, bool)
|
Thanks so much, @lindsay-stevens. |
Closes #8
raise_for_status, which checks for status codes >=400 and <600.What has been done to verify that this works as intended?
Tests that return type is as expected when feeding in mock test fixture data.
Note that currently the Central sandbox returns unexpected Comment data in response to POST. It includes an empty
actorId, and the comment body text is split into an array. So until that is fixed,comment.postandsubmission.edit(if a comment is sent) will raise a validation error when processing the response data.Why is this the best possible solution? Were any other approaches considered?
Implementation is as discussed / specified in #8. In terms of leaving the validation issue there, presumably this will be fixed up shortly. In the meantime users could catch the error and ignore it. This seems preferable to not validating the response for now, and remembering to go back and add it again once the central issue is fixed.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Only as described above.
Do we need any specific form for testing your changes? If so, please attach one.
No, there's test / mock fixture data.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
The README is updated with the new methods. I also added in the first line from each method in case the verb / method names alone weren't clear about what they do.
Before submitting this PR, please make sure you have:
testsnosetestsand verified all tests passpython bin/pre_commit.pyto format / lint code