Skip to content
This repository has been archived by the owner on Nov 2, 2023. It is now read-only.

Support audit, email, bearing #266

Closed
wants to merge 12 commits into from

Conversation

florianm
Copy link
Contributor

@florianm florianm commented Dec 14, 2021

Blockers

Update 11 Jan 2022: I'll split this PR into individual PRs for isolated issues to facilitate the review process. Reverted this PR to draft and keeping for reference until content is merged via new PRs.

Update 31 Jan 2022: All bits are split into separate PRs. I've closed this PR.

Summary

Questions

  • The XForms spec on metadata shows other metadata fields added to the /data/meta node. The current behaviour of Build, Central, Collect is that forms work fine if these metadata kinds are not added as a node. Should they be though?
  • Should we include other audit parameters as per (XLSForm based) ODK docs? (I haven't tested whether this breaks the XForm)
  • Is the XForms spec on metadata out of date? I see e.g. userID vs. username.
  • What are good defaults for audit logging? I've updated the defaults to 20/60s min/max location age.

Impressions

A simple test form: 290test4.zip
ODK Validate found the form OK. An existing, complex form with added "audit" also passes ODK Validate.

A test form for #268 Translation268.zip. Again, ODK Validate considers the form valid.

Screenshots for auto-send/auto-delete see #126.
Screenshot for contributor link see #82.

ODK Build

Manually modded XML popup for readability (at least at full screen).
image

Audit log

We have millisecond timestamps, locations, and old/new values.
image

Submission

We see the email from ODK Collect metadata at the far right.
image

Way forward

What's the best workflow for getting this upstream? (I'm aware I'm stepping on the feet of the core team here.)
In absence of unit/integration tests, we could merge, release as a patch release 0.4.1, deploy to test.build.getodk.org and let a few folks do some testing.

@florianm florianm added this to the 0.4.1 milestone Dec 14, 2021
@florianm florianm added this to In progress in Roadmap via automation Dec 14, 2021
@florianm florianm self-assigned this Dec 14, 2021
@florianm florianm changed the title Support for metadata kinds audit and email Support audit, email, bearing Dec 14, 2021
Florian Mayer added 7 commits December 28, 2021 10:28
* This still allows to produce a form which will crash Collect
* At least the user is prompted to fill in all translations
* javascripts/underscore.min.js refers to a not included .map
* since no other JS maps are included, removing the reference to the map fixes the warning
* If an extra parameter or label or hint has at least one non-empty string, empty nodes are added for all missing translations
* If an extra parameter is not filled in for any language, no nodes will be added
* Noop: Add some comments in preparation for getodk#126
… contribute

* Add options as dropdown menus to Edit > Form Properties
* Default to Follow ODK Collect form management settings
* Save/restore selected settings
* Style form properties select like text input
* Fix getodk#82: Add "become a contributor" to About menu with a link to Contrib guidelines
* Add margin to form select like to form input
* getodk#160
@yanokwa
Copy link
Member

yanokwa commented Jan 10, 2022

@florianm Thanks for sending this in! I'd love to get these changes merged, but this PR is hard to review as is because

  1. It mixes isolated changes with changes that might have downstream implications (e.g., adding bearing is isolated but adding auto-send/auto-delete is not).
  2. It mixes formatting changes and code changes in the same commit. 9d3619c.
  3. The commit comments aren't always accurate. 8a9a4d4 says it's about adding a language, but there is a change to audit location defaults.

In general, unless the changes are all very related, it should be one narrow PR per issue, so it makes for a faster review and faster QA. Can you please split things out?

@florianm
Copy link
Contributor Author

florianm commented Jan 10, 2022

Should I send several different PRs? Should I start with the noop changes - formatting, help text and so on, or are there any issues you'd QA first, like the translation bug with empty nodes?

@yanokwa
Copy link
Member

yanokwa commented Jan 10, 2022

Several PRs would be great! Thank you 🙏🏾

@florianm
Copy link
Contributor Author

@yanokwa #270 is ready for review. Picked this issue to start with as it's a bugfix for prod (0.4.0).

@issa-tseng
Copy link
Member

in general also i am more inclined to leave formatting as is than change it even when it's wrong, because such changes make history and blame search very difficult.

@florianm
Copy link
Contributor Author

OK that's easy enough to do! I got tempted by the auto-formatter. As Yaw asked, I'll split this PR into smaller PRs for faster review. I can merge and deploy any approved PR to test. I assume once the whole bunch is in and tested, a release to prod might be in order.

@florianm florianm modified the milestones: 0.4.1, 0.4.2 Jan 26, 2022
@florianm florianm moved this from In progress to To do in Roadmap Jan 29, 2022
@florianm florianm closed this Jan 31, 2022
Roadmap automation moved this from To do to Done Jan 31, 2022
@florianm florianm deleted the 209-metadata-kinds branch January 31, 2022 06:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.