Allow uploading attachment files alongside a submission

[spwoodcock]

This PR

• Closes Extend submission.create to support attachments #102
• Following on from Add submission attachment API #104 (built on the branch, anticipating the merge)
• Allow passing a list of file paths in the SubmissionService.create() method.
• The files are uploaded as attachments after the submission is created.

If easier, #103 and #104 could be closed and this reviewed & merged all in one go.

What has been done to verify that this works as intended?

I added a test to upload 3 attachment files to one submission:

Why is this the best possible solution? Were any other approaches considered?

• It solves a requirement provided from an existing issue.
• The other approach would be to expect the user to create the submission, then upload attachments separately.
• I think this is a nice addition though, making life significantly easier to creating a submission+attachment combo.

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?

• This shouldn't really impact the existing API.
• It's additive only changes to the submission create.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

• Updates to the pyodk docs!
• Will look into that if this PR is approved.

Before submitting this PR, please make sure you have:

• included test cases for core behavior and edge cases in tests
• run python -m unittest and verified all tests pass
• run ruff format pyodk tests and ruff check pyodk tests to lint code
• verified that any code or assets from external sources are properly credited in comments

[lindsay-stevens]

Please remove from client - the current use case is to upload submission + attachments at the same time, rather than separately.

[lindsay-stevens]

Please follow the pattern in form.create e.g. raise an error rather than just log. Conversion to Path is the responsibility of upload - have a look at form_draft_attachments which uses pv.validate_file_path to handle conversion from string (if necessary).

[lindsay-stevens]

Are you using an old python version? I think the list annotation is OK for all current versions. The previous equivalent was to using typing.List, not builtins. Anyway that's besides the point - we just need something to feed the for loop i.e. Iterable[PathLike | str] (see forms.create).

[lindsay-stevens]

Please follow the forms.create process where it's assumed file paths are passed in. If using bytes is needed please open a ticket for that so we can think about doing that consistently across endpoints that currently accept file paths.

[lindsay-stevens]

Could you please rebase on the main branch? It seems like #104 has some of these changes here but not all of them so I'm not sure I understand the purpose of that PR. Also it seems like #103 was force pushed so I'll need to review again. Generally it will make it easier to review without all the model changes stuff.

Another thing needed before this can be merged is a real (E2E) test case. I've been putting those in test_client.py - it should be able to create what it needs to run, as long as it's a valid project ID and the user has privileges. Do you have an instance of Central that you can test with? Or do you have access to the ODK dev/staging instances? This is needed to verify that it works with current Central, sanity check against the original requirement (#102), and provide example code that use case. Sometimes the use cases are polished up into a module for docs - if you want to do that as well it would be appreciated.

[spwoodcock]

You told me the commit history had too many commits referencing formatting. So I squashed them into one commit per PR 😅

Is there a preferred way you would like commits to be made?

(meaning do you intend to squash them on merge, should I minimise the number of commits if they are going directly to main, or something else?)

[spwoodcock]

Could you please rebase on the main branch? It seems like #104 has some of these changes here but not all of them so I'm not sure I understand the purpose of that PR. Also it seems like #103 was force pushed so I'll need to review again. Generally it will make it easier to review without all the model changes stuff.

Another thing needed before this can be merged is a real (E2E) test case. I've been putting those in test_client.py - it should be able to create what it needs to run, as long as it's a valid project ID and the user has privileges. Do you have an instance of Central that you can test with? Or do you have access to the ODK dev/staging instances? This is needed to verify that it works with current Central, sanity check against the original requirement (#102), and provide example code that use case. Sometimes the use cases are polished up into a module for docs - if you want to do that as well it would be appreciated.

Sure, I can give that a go!

I have multiple ODK instances to test with, no worries. I generally test with a containerised Central.

I am using this code in production now & all seems to be well. Unfortunately I use pytest for all the test cases using this functionality, but I can look at writing some stdlib unittest instead 👍

[spwoodcock]

Also, as mentioned in each PR, they anticipate the merge of the previous as they rely in code from the previous PR.

(1) Pydantic V2 models --> (2) Submission attachment service --> (3) Using the submission attachment service to upload attachments during submission.

So rebasing with main isn't possible unfortunately 😬