WIP: Answers in the audit file #3024
Conversation
| if (isAuditEnabled()) { | ||
| // Calculate the time and add the event to the auditEvents array | ||
| long end = getEventTime(); | ||
| for (AuditEvent aev : auditEvents) { | ||
| if (!aev.isEndTimeSet() && aev.isIntervalAuditEventType()) { | ||
| addLocationCoordinatesToAuditEventIfNeeded(aev); | ||
| aev.setEnd(end); | ||
| if (answers != null && !answers.isEmpty() && aev.getAuditEventType() == AuditEvent.AuditEventType.QUESTION) { | ||
| Map.Entry entry = answers.entrySet().iterator().next(); |
There was a problem hiding this comment.
This means the answers from questions in a field list will be provided in some arbitrary order because the map is a hashmap, right?
There was a problem hiding this comment.
Yeah, the order should be the same as questions on the screen.
There was a problem hiding this comment.
Ah, right, this is actually worse than I had originally imagined, I think. Isn't it the case that the audit event has a reference and that the answer that gets associated with that audit event might not match the reference to the question? How about passing a map from reference to answer rather than FormIndex to answer? By reference I mean what is stored in the AuditEvent.node field (e.g. /data/name), so that you can just match the event to its answer?
|
I described a concern I have about field lists here. Other than that, I believe this always logs the answer, even if it hasn't changed. I think that's a deviation from what was discussed on the forum but it seems fine to me. |
Yes, if we log answers along with questions how would you do that? Would you leave the answer empty? I thought the empty value should be for nulls... of course, we could use NULL for nulls and empty values if the answer is the same but still it would be misleading. Is this approach that wrong? |
|
I've had some more time to think the blank/null/no change issue and wanted to share my thoughts. We should not use a magic value (e.g., Writing the value on every question is a solution, but I don't love it because analyzing the log of data to find out what has changed is not fun and it's a lot of redundant information. Yes, we could have a system do that analysis, but we should still make it easier. I'm leaning more towards a new event for a value change. Yes, this is more rows, but the tradeoff is that it's super easy to get the change information that we need. It also means that we can eventually record change events that are not associated with viewing a question (e.g., calculates, metadata). Is my reasoning convincing, @lognaturel and @grzesiek2010? |
Same here, that was what I was expecting initially. The unfortunate thing becomes that we then have columns that are only applicable to certain events. The documentation should be very clear on that. I don't think there's an issue filed for this feature. https://forum.opendatakit.org/t/collect-keep-history-of-changes-to-values-in-the-form/15458 is the forum post that discusses it. One other thing that just crossed my mind is that answers could contain commas so they should either always be in quotes or there should be a check that looks for commas and puts the value in quotes if needed. |
|
Ugh! I didn't mean to make this PR ready for review. I can't seem to undo that change, so I've changed the title. Sorry 😭. |
Good point! Ok, I can change this approach tomorrow. To be clear, that new event should be logged like now? once a user navigates back/forward or opens the hierarchy? |
|
@yanokwa, do you have a name for this event? Yes, I think it should happen on navigation out of the screen BUT a row should only be added for values on the screen that are different. That is, if I look at a question, that generates an access event. If I exit the screen and don't change the value, no value changed event is written. Then if I go back to the question and change its value, two events are written out when I exit the screen: the access event and the value change event. I think both should have the exact same start and end times as well as location. This is where the somewhat annoying redundancy comes in. @yanokwa, do you also want to think about this and perhaps summarize for the forum thread since these are all user-facing questions and decisions? Edit: same for a field list. I'd expect each question that changed to generate a value changed event. Any values that did not change would not generate such an event. |
|
I'm closing in favor of #3072 |
auditForm.xml.txt