-
Notifications
You must be signed in to change notification settings - Fork 82
Conversation
* Closes getodk#164 * Audit settings are form properties like autosend/delete * Code comments and console logging for clarity * Form props modal widened to fit four new fields
Re labels: More verbose labels could read like Track form changes:
Track device location:
The console log, if retained, could also be worded more consistently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. I believe all my concerns from the prior PR have been addressed. I don't think the labels need to be more verbose. However, there is one option missing which is audit log without any modifiers. That's different from track changes. It's just collecting the flow through the form. Could you perhaps disable all of the modifiers you have now until audit is turned on?
Ah so there is a plain vanilla audit flavour without track changes or location? Should I add another form property "Enable form audit" (yes/no) to toggle the audit bind node, and let the audit modifiers add attributes as appropriate? Or should I merge this with "Track changes" into a form property "Form audit" with options
@lognaturel if I could pick your brain on the above (which impacts if/else logic building the bind node), I could get started and we could refine the wording of the labels later. |
I take it there’s no straightforward way to do what I described with a first option to turn on audit and then a section that is only enabled/visible if audits are on? If not, I guess your combination is ok. Then the location text can maybe make it clear that the flow is also captured. It’s weird because you could set “Disabled” for your new setting and configure location capture. But the basic audit would still be created. If that suits your needs, I’m fine with starting there and revisiting if needed. It would be helpful to make it more clear that the new options added here result in data captured in a separate audit file. |
Thanks for the clarification! I hear your preference as:
Does the XForms standard support https://docs.getodk.org/form-audit-log/#reason-for-changes and https://docs.getodk.org/form-audit-log/#enumerator-identification? I'd expect we could include these options here too seeing Collect supports them since v1.25.0. |
A+++, thanks! |
XML: the "new-to-XForms" attributes
ODK Validate finds no errors, but also did not find an error I corrected since ( Testing on my end: The above form seems to work as advertised.
Last nitpick is a minor bug in the visibility toggles for fresh loaded Build sessions when loading existing forms with saved "yes" attributes - they need to toggle to no, then to yes in order to show the initially hidden form properties. |
* Add new property "form audit" to toggle entire audit on or off. Hides audit attributes if off. * Add new property "identify user" for `odk:identify-user` * Add new property "track form change reasons" for `odk:track-changes-reasons`, hidden unless "track form changes" is "yes" * Add new property "track device location" which toggles visibility for the three location attributes and adds their values (or sensible defaults) to the audit bind node * Rework the logic to build the bind node * Change the style for .modal (less top offset) to fit the full form property modal on screen at 100% * Improve the labels and help text, add links to docs and specs with rel="external" like other links in Build * This commit has been tested on latest Collect and Central, everything seems to work as advertised * The toggling visibility logic needs a last patch for new Build sessions with an existing form opened with saved audit "yes" - should expand right away but does only after toggling to no, then yes again
* If loading a saved form with saved audit options other than off/no, additional properties are sometimes hidden * Bottom right, opposite "Done", is a new "Show all properties" link to un-hide all props * Refactor visibility via CSS class rather than style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! And there will be a corresponding 2xlsform thing?
Absolutely! We could go like this: |
.narrowModal
. Field labels are wider to accommodate the longer new labels. Could use a designer's eye to verify spacing / margins.Impressions
Screen shots and walk-through below.
New form defaults
A new form shows "all off" defaults.
New form, track changes
Enable track changes adds
data/meta/audit
and<bind nodeset="data/meta/audit" type="binary" odk:track-changes="true"/>
.Console log:
New form, track changes, track location
Setting the "Track device location" to e.g. "Balanced", retaining the defaults for min interval and max age:
This adds the three location audit attributes to the bind node
<bind nodeset="data/meta/audit" type="binary" odk:location-priority="balanced" odk:location-min-interval="20" odk:location-max-age="60" odk:track-changes="true"/>
:Console log:
New form, disable track changes, keep location audit
Setting "Track changes" to "No" and changing "Track device location" to "High accuracy":
The
data/meta/audit
node is still there, and the bind node now showstrack changes
asfalse
Console log:
Existing form
In an existing form, the new form properties are not set, defaulting to "all off".
Exporting to XML creates neither a
data/meta/audit
node nor a bind node.Existing form plus track changes
Exported XML now has
data/meta/audit
node and<bind nodeset="data/meta/audit" type="binary" odk:track-changes="true"/>
.Existing form plus track changes plus location audit
Location min interval and max age are kept empty.
Exported XML has
data/meta/audit
node and has expanded<bind nodeset="data/meta/audit" type="binary" odk:track-changes="true" odk:location-priority="balanced" odk:location-min-interval="20" odk:location-max-age="60"/>
with sensible defaults.The console explains what happened:
Existing form with location audit minus track changes
Setting "Track changes" to "No" works as expected:
Exported XML has
data/meta/audit
node and has<bind nodeset="data/meta/audit" type="binary" odk:track-changes="false" odk:location-priority="balanced" odk:location-min-interval="20" odk:location-max-age="60"/>
.We still haven't bothered with min/max attributes and still get the explanation about the defaults.
Saving a new form with audit
A new form with audit properties set and saved does remember these settings when loaded again.