collect icon indicating copy to clipboard operation
collect copied to clipboard

Fix audit log event ordering

Open seadowg opened this issue 3 years ago • 2 comments

Closes #5245

What has been done to verify that this works as intended?

Added new tests and verified manually where I couldn't (comments inline on that).

Why is this the best possible solution? Were any other approaches considered?

This PR does the bare minimum to fix the issues. There's a nicer solution coincidently in the works (as part of the fix for #5240). Working on that is how I realized there was an issue with the log in the first place.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Things to focus on when checking this:

  • form entry in general
  • audit logging (especially with identify user and track change reason enabled)
  • moving backwards enabled/disabled (with save points)
  • editing forms and resuming from save points

Before submitting this PR, please make sure you have:

  • [x] run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • [x] verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • [x] verified that any new UI elements use theme colors. UI Components Style guidelines

seadowg avatar Aug 12 '22 11:08 seadowg

I know we might need to wait a couple of days for review on this so going to mark this as "needs testing" now.

seadowg avatar Aug 12 '22 11:08 seadowg

In Collect v2022.2.3 in CSV file (e.g. form AuditTestLocationBackground) there is a different order of „question” and „location tracking enabled”. AuditTestLocationBackground.xml.txt In Collect v2022.2.3 first there was "location tracking enabled" and then "question". auditcsv2022 2 3 In PR there is "question" and after that "location tracking enabled" auditcsvpr

dbemke avatar Aug 16 '22 14:08 dbemke

@srujner @dbemke looks like there's another problem with the log when editing a form. Here's an extract (I've cut out the first fill for clarity) of editing a form in v2022.2.3:

form resume,,1660760793169,,,,
jump,,1660760793183,1660760794203,,,
question,/data/what_up,1660760794238,1660760794924,,,
end screen,,1660760794930,1660760795211,,,
form save,,1660760795576,,,,
form exit,,1660760795576,,,,
form finalize,,1660760795577,,,,

And here's the same flow/flow on this branch:

form resume,,1660756947920,,,,
jump,,1660756947922,1660756948229,,,
question,/data/what_up,1660756947948,1660756948229,,,
question,/data/what_up,1660756948995,1660756949755,,,
end screen,,1660756949776,1660756950028,,,
form save,,1660756950378,,,,
form exit,,1660756950379,,,,
form finalize,,1660756950379,,,,

It seems like we end up with a question we don't want in there. I actually found this due to an existing test, but it was only failing intermittently. I'll look at a fix. I'm not sure how much hacking I can do before we need to make a larger change, however.

seadowg avatar Aug 17 '22 18:08 seadowg

@srujner @dbemke that's ready to look at again. I've played around with a few different form filling scenarios (and added some more tests) to check that the event order looks good, but that's definitely the place to start exploring. I still feel like we're missing a lot of test coverage, and probably team knowledge of how the feature works, so best to verify things against older versions. Also, remember to play around with editing forms as well as filling in forms for the first time.

@grzesiek2010 I'll dismiss the review as it'd be good for you take another look. I'm not very happy with de98455, but I'm trying to keep changes as minimal as possible, and all of this is getting moved around for #5240.

seadowg avatar Aug 18 '22 09:08 seadowg

Tested with Success!

Verified on Androids: 8.1, 11

Verified cases:

  • issue #5245 , the case of extra question event and location tracking after question event are no longer reproducing
  • form entry in general
  • audit log order of: start form, questions, location tracking, resume form, jump, save, exit, finalize
  • saving and editing different forms: AuditTestLocationBackground, AuditTest, Track Changes Reason, trackingChangesAndLocationTestForm, Identify User, test audit repeats, Nested repeats for audit
  • regression checks of forms
  • resuming from a save point
  • location and audio permission enabled, disabled
  • moving backwards enabled, disabled

dbemke avatar Aug 18 '22 14:08 dbemke

Tested with Success!

Verified also on Androids: 5.1, 10 and 12

srujner avatar Aug 18 '22 17:08 srujner