Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include instanceId in client audit csv export #335

Merged
merged 3 commits into from
Mar 12, 2021
Merged

include instanceId in client audit csv export #335

merged 3 commits into from
Mar 12, 2021

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Mar 4, 2021

This addresses issue #333 and forum comment https://forum.getodk.org/t/update-the-audit-log-structure/31887. Now each row of client audit events contains an extra uuid column referring to the unique ID of the corresponding submission.

The new uuid column id named that because of the forum post, but it should possibly be named KEY or instanceID to be more consistent with other exported files.

Turns out, although there's a complicated query in model/query/client-audits.js spanning many tables, the submissions table with instanceId column is already one of the tables active in that big join so this doesn't impact the database performance.

@lognaturel
Copy link
Member

lognaturel commented Mar 5, 2021

Yay for your first PR! 🎉

Oops, I wasn't very explicit in the issue. The new column should be the first one and it should be named instance ID. This is to exactly match what Briefcase exports for the same dataset so folks can go between the two and use the same analysis pipeline. Hopefully we can eventually deprecate Briefcase but for now there are a few things people need from it that are not in Central such as splitting select multiples and stripping group names (docs).

You and @issa-tseng will no doubt discuss the details. I have a couple high-level/process suggestions. Github has a nifty "draft" feature you might consider using for PRs that are not ready to merge. You can also convert a PR once it's up. The contribution docs have some guidance on writing commit messages (totally fine to amend commits and force push to a branch even if there's a PR on it).

@ktuite
Copy link
Member Author

ktuite commented Mar 5, 2021

I've moved the instanceID field to the first column! And updated my commit message to better match style guidelines.

I've also tried it on more realistic data (submissions collected through Collect app with auditing enabled).

Central export screenshot
Screen Shot 2021-03-05 at 12 32 52 PM

I've noticed a couple other things:

  1. Briefcase audit export has different columns/headers. It doesn't explicitly say "old value"/"new value" but it does have an extra unnamed change reason column. It also doesn't include the GPS audit info available from Central export.

Briefcase export screenshots
different headers
Screen Shot 2021-03-05 at 12 37 16 PM

change reason present
Screen Shot 2021-03-05 at 12 43 12 PM

  1. Because of how the Collect app appears to handle client audits as attachments all named audit.csv, the media export folder includes a single audit.csv file. This might be the source of the forum commenter (https://forum.getodk.org/t/update-the-audit-log-structure/31887) saying:

I have seen that the audit CSV file included in the *.zip now merges the audit data for all submissions, which is great (while it seems to me in previous versions it was only available for the last submission).

Screen Shot 2021-03-05 at 12 45 08 PM

@lognaturel
Copy link
Member

lognaturel commented Mar 5, 2021

Thanks for actually trying the Briefcase export to compare! I think you're mostly running into getodk/briefcase#740. Briefcase just concatenates all the CSVs and uses a fixed header which definitely should be addressed at some point. I would expect the location columns if https://docs.getodk.org/form-audit-log/#location-tracking is enabled on the Briefcase side but it's not clear to me whether maybe they're after all the change columns and not visible because they have no values and no header? I use something like this app so Collect gets locations that aren't my home when testing.

the media export folder includes a single audit.csv file

Yes, the files are technically apparently in the zip but most zip clients won't make them accessible. The combined log is definitely higher priority but at some point it might be nice to do something like put audits in their own folder and add a submissionID prefix to the filenames.

I see a bunch of test failures on CI. Are those expected and also failing locally or a CI special?

@ktuite
Copy link
Member Author

ktuite commented Mar 8, 2021

Tests have been updated now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants