Add support for entity updates

lognaturel commented 10 months ago

Closes https://github.com/XLSForm/pyxform/issues/662

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

The complexity here comes from the fact that the spec allows for implicit actions. Specifically:

if there's just a dataset and label specified, it's implied that submissions should always create entities
if an entity_id is specified, it's implied that submissions should always update entities

That makes the implementation more complex but I continue to think it's worth it because it's more intuitive than having to explicitly specify true() as create or update conditions.

Given that complexity, I decided to pass through the XLSForm values in the json representation (entities_parsing) and then to handle all the cases when building the XML representation (entity_declaration).

I used a truth table to make sure I handled and tested all the combinations of entity_id, create_if and update_if:

    # id    create  update  result
    # 1     0       0       always update
    # 1     0       1       update based on condition
    # 1     1       0       error, id only acceptable when updating
    # 1     1       1       include conditions for create and update, user's responsibility to make sure they're exclusive
    # 0     0       0       always create
    # 0     0       1       error, need id to update
    # 0     1       0       create based on condition
    # 0     1       1       error, need id to update

Hopefully that also helps with review.

I did find it challenging to balance writing narrow, specific tests and feeling confident that all aspects of the form were taken care of with each combination of options.

What are the regression risks?

There's some possible regression risk around entity creation. Hopefully the tests guard against it! The bigger risk is that I have a logic error in the new code.

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

Part of https://github.com/getodk/docs/issues/1678

Before submitting this PR, please make sure you have:

[x] included test cases for core behavior and edge cases in tests
[x] run nosetests and verified all tests pass
[x] run black pyxform tests to format code
[x] verified that any code or assets from external sources are properly credited in comments

lognaturel commented 10 months ago

Thanks so much for the thorough review, all the great suggestions, and particularly for catching that the spec change should be versioned. 🙏

Since I ended up making substantive changes I'd prefer one last sanity check on the last three commits, please! Feel free to merge if you're happy with that.

I think it's worth considering calling the validation logic from EntityDeclaration (during XML processing) either in addition to or instead of from workbook_to_json

This is an interesting suggestion that I'd rather chew on and address in a follow-up PR.

A key benefit to checking during the latter is that it's possible to tell the user where they made a mistake in the XLSForm, but users can only have 1 entity per form so that is kind of redundant.

The current messages use a lot of spreadsheet language too.

by calling the validation during the XML pipeline it can apply to data coming in via the JSON entrypoint

Do we know that it's for sure being used?

lindsay-stevens commented 10 months ago

Thanks! To summarise the related slack thread, the behaviour in this PR is that if a XLSForm uses entity updates then it gets the newer spec version 2023.1.0 and otherwise it gets the older spec version 2022.1.0. This is to allow forms not using the new features to be accepted by older versions of Central. If a form uses entity updates then older Central wouldn't know what to do with them so rejecting the form based on the spec version avoids users setting up forms that don't work as expected.

XLSForm / pyxform