Fix entity bug where the wrong properties of an entity are populated

[ktuite]

On Slack, @lognaturel identified a bug where a form field that populates an entity property is not updated if it’s changed by a new form version.

For example, let’s say my form has fields form and form_name. I put a save_to on form_name. Then I realize I made a mistake and change the save_to to form. New submissions using the new form definition still save form_name.

It seemed like the fields being used to parse entity properties from a submission weren't filtered to just the properties in that specific form def/schema.

Looking into it more, this was indeed the case: when looking up fields by the submission's form def id, dataset property fields were joined via the form schema, but the schema doesn't change when just the entity bindings change (but maybe it should??)

Luckily (probably because of this exact situation) the ds_property_fields table still has a formDefId column, which we can go back to using for this join.

(Edit - Done) Leaving in draft mode while I look into

• a bug that may be related about not being able to remove a binding (no bug except in my test -- I was just missing a closing '/' in my xml replace)
• any other places that might be joining property fields on schemaId when it would be better to use the formDefId
  • The only other place schema is used in a join like this is in Dataset.getProperties, but it gets properties for the entire dataset and has its own ways of grouping and ordering the properties. Also, the formDefId is not always available in the query. We have a lot of tests around getting dataset properties with this function so I think it can stay the way it is.

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

Tests.

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

Since the form schema doesn't change when just the dataset bindings change, we can't use the schemaId when joining dataset properties and form fields. Switch back to using the more specific formDefId that we still have on dataset_property_fields.

Maybe the schema should change in these cases.

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?

Addresses a bug.

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

No.

Before submitting this PR, please make sure you have:

• run make test-full and confirmed all checks still pass OR confirm CircleCI build passes
• verified that any code from external sources are properly credited in comments or that everything is internally sourced

[lognaturel]

Pretty subtle!

My very high level sense is that the schema should be the form schema and should not change based on entity binds. It feels conceptually right to me to use the form def to indenting those.

[sadiqkhoja]

I was thinking to remove schemaId from ds_property_fields altogether, but we are using it in another query getProperties. Over there it is used to get the order of the properties and there is an open issue related to property ordering so we can consider removing schemaId during the fix of that issue.

[matthew-white]

@ktuite, just wanted to check about this PR! Is it ready to merge?