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

50: add $select parameter to submissions.get_table #64

Merged
merged 2 commits into from
Apr 13, 2024

Conversation

lindsay-stevens
Copy link
Contributor

@lindsay-stevens lindsay-stevens commented Mar 28, 2024

Closes #50

Substantive change is the addition of $select as a parameter for client.submissions.get_table.

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

The updated tests/test_client.py includes the select parameter to demonstrate how it works. This PR also documents and mostly automates the setup of Central data for existing E2E tests, e.g. creating or updating forms and submissions.

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

The addition of $select is about as straightforward as possible.

For the E2E test changes, I considered writing longer step-by-step documentation but then I thought, I am writing the steps anyway, how hard could it be to write it as code instead to automate it? Turns out it was a bit time consuming to achieve reproducible runs (especially the side-trip into the world of percent-encoding standards and submission ID behaviour) but now the foundation is there for future use.

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?

The user-visible changes are the addition of an OData parameter.

The percent-encoding change is unlikely to cause any grief because the default is to use UUIDs for submission instanceIDs. It's possible that the old encoding might have unnecessarily encoded some characters *'() in form IDs which could plausibly include these characters, but I think these get decoded properly by Central anyway, e.g. JavaScript console.log(decodeURIComponent("*'()%2A%27%28%29")); returns *'()*'(). However the old function quote_plus replaced spaces with a plus + instead of %20 which is maybe OK in query strings but not form IDs etc.

Do we need any specific form for testing your changes? If so, please attach one.

The test changes allow us to write test XLSForms as markdown tables just like pyxform, so actually some XLSX files are removed in favour of markdown strings.

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

No external documentation, as discussed in Slack. But I added a lengthy comment in session.py for future reference.

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 pyxform tests and ruff check pyxform tests to lint code
  • verified that any code or assets from external sources are properly credited in comments

@lognaturel
Copy link
Member

I think in the past one or both of us have adhoc tried some basic examples against a real server, right?

Do you think we should put together a reproducible process for running E2E tests? I'm of two minds -- one one hand, yes, of course, we should always have reproducible, automated processes, especially for testing. In this case, it could help catch regressions/incompatibilities as Central versions change. On the other hand, maybe doing a quick verification and relying on narrower tests is sufficient for now.

@lindsay-stevens
Copy link
Contributor Author

@lognaturel the tests/test_client.py file is the E2E zone, but it's skipped by default because it requires access to Central plus some manual project setup (this is fine for you / me / CI using test/staging, but not for external contributors). All the required forms should be in tests/resources/forms/. I don't think I've written down the test project setup, but will note it down for this PR and entities testing. It probably could be fully automated, so long as there's a way to fully clean up after a test run.

- updated readme with section on testing and setting up for E2E
- automated create/update of forms and submissions for E2E tests
- copied over md_table functionality from pyxform to allow specifying
  test forms as markdown instead of accumulating XLSX files
- added openpyxl as a dev dependency to support conversion of md_table
  XLSForm data to XLSX format
- updated test_client.py / TestUsage.test_direct to use the new
  `$select` parameter, also showing how to select sub-properties
- fixed percent-encoding in `session.py` to match Central behaviour,
  see new docstring comments for details, update tests accordingly.
@lindsay-stevens lindsay-stevens marked this pull request as ready for review April 10, 2024 11:14
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Nice! 🎉

@lognaturel lognaturel merged commit 1e9fca5 into getodk:master Apr 13, 2024
4 checks passed
@lindsay-stevens lindsay-stevens deleted the pyodk-50 branch April 15, 2024 10:01
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.

Add $select to get_table
2 participants