search

getodk / xforms-spec

The XForms-derived specification used in the ODK ecosystem. If you are interested in building a tool that is compliant with the forms rendered by ODK tools, this is the place to start. ✨⚒✨
https://getodk.github.io/xforms-spec/
30 stars 26 forks source link

Add track changes support to audit log #231

Closed yanokwa closed 5 years ago

yanokwa commented 5 years ago

One might expect that we'd want to support a boolean true, but rather we are using the string "true". This approach we are taking is not XPath-friendly, but it is consistent with what we've done with other attributes (namely auto-send or auto-delete). @MartijnR I'd love your thoughts here.

Feature discussion: https://forum.opendatakit.org/t/collect-keep-history-of-changes-to-values-in-the-form/15458 Pull request: https://github.com/opendatakit/collect/pull/3072

lognaturel commented 5 years ago

Agreed with @MartijnR's requests. The naming and namespace seem consistent to me so no concerns there.

To be fully explicit for posterity, my understanding of how booleans work in XPath is that the absence of the attribute or the attribute set to value "" or the attribute set to a number <= 0 would mean the attribute has boolean false but any other value given to the attribute would mean boolean true. That is, in addition to odk:track-changes="true", odk:track-changes="false" would turn on change tracking, as would odk:track-changes="23".

Here, @yanokwa is proposing to continue a convention established here with orx:auto-send and orx:auto-delete to consider "true" as boolean true and anything else as false. This is more intuitive so I'm in support of it. If we didn't already have that convention, I might be more hesitant. Note that this is a static value -- expressions are not evaluated.

Somewhat annoyingly, this deviates from boolean-from-string which, in addition to "true", considers "1" as boolean true.

(@MartijnR I know you got all this, I just want it written down explicitly. Please let me know if I made a mistake, though!)

MartijnR commented 5 years ago

Thanks. I actually missed the question about booleans. I see this as a string value that happens to look a little like a boolean because of the values we accept. Here are a few more: https://www.ibm.com/support/knowledgecenter/en/SSS28S_8.1.0/XFDL/xfdl_r_xforms_submission.html

I thought an actual boolean attribute would basically be presence (true) or absence (false) of the attribute regardless of any value (in HTML the value can be either "" or the name of the attribute). In this case because there was precedent in XForms/XPath for such a non-evaluated on/off options (indent, omit-xml-declaration), we chose the same "true", "false" string values (for e.g. auto-send). I agree that it makes most sense to consider any other value as irrelevant and therefore use the default.

lognaturel commented 5 years ago

Oh good, I'm so happy there's a precedent! Your explanation is perfect.

I'm not sure where I got the < 0 part in my prior comment but 0 or NaN being false seems consistent with the XPath 1 spec. Regardless, I think expecting "true" and considering everything else the default "false" is much easier to explain.