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

Fixed missing „form resume” in audit log events when moving backward in not allowed in Collect settings #5708

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Aug 19, 2023

Closes #5253

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

I've added automated tests and verified the fix manually.

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

There is nothing to discuss here.

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?

It should just fix the issue. I don't think it can affect anything else.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

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

seadowg
seadowg previously approved these changes Sep 15, 2023
CollectHelpers.killAndReopenApp()
}

MainMenuPage().startBlankForm("One Question Audit")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd make sense for the test to save and exit the form as well to check the audit log is in the state we think it should be after the session has ended (as that's what would get uploaded).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

val device = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())

// kill
device.pressRecentApps()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow I'd not realised you could do this! I think I tried something similar, but kept ending up with the test exiting as soon the app was killed.

A couple of things here:

  1. I think that killAndReopenApp could just be a method on Page. To be really purist about it, it could take both a package name to restart and a destination page to assert on (here you'd pass "org.odk.collect.android" and a MainMenuPage instance).
  2. It looks like this doesn't actual restart the process (Application#onCreate is not called again). We do already have a helper for simulating that (CollectHelpers.simulateProcessRestart could be called after the swipeUp though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that killAndReopenApp could just be a method on Page. To be really purist about it, it could take both a package name to restart and a destination page to assert on (here you'd pass "org.odk.collect.android" and a MainMenuPage instance).

Done but do the package name I used BuildConfig.APPLICATION_ID so that we don't have to hardcode it.

It looks like this doesn't actual restart the process (Application#onCreate is not called again). We do already have a helper for simulating that (CollectHelpers.simulateProcessRestart could be called after the swipeUp though.

Maybe the process is not restarted to keep the test running... never mind I've added CollectHelpers.simulateProcessRestart().

@seadowg seadowg dismissed their stale review September 15, 2023 14:24

Wrong button!

Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Would just like to see a couple of tweaks to tests.

@dbemke
Copy link

dbemke commented Sep 27, 2023

The "form resume" event is present but the last question before killing the app is missing. On the master version the last question (before killing) also appears only once but there isn't the form resume question so then seems ok. Should there be two events for the last question- event before killing the app and event after going back to the app? So I'm not really sure if it's a separate issue or the PR.

Steps to reproduce:

  1. Open Audit Test form.
  2. Fill questions 1,2,3.
  3. While on the question 3 kill the app.
  4. Go back to the form.
  5. Tap next and fill questions 4,5,6 and send the form.
    text3missing

Text 3 question is missing before the "form resume" event. It appears after the event (going back to the app).

@grzesiek2010
Copy link
Member Author

It works fine to me if on master that question is also logged only once. I think this will be fixed in #5721

@dbemke
Copy link

dbemke commented Sep 27, 2023

Tested with Success!

Verified on the device with Android 10

Verified cases:

The mentioned issue will be tested again along with #5721

@srujner
Copy link

srujner commented Sep 27, 2023

Tested with Success!

Verified on the device with Android 13

@grzesiek2010 grzesiek2010 merged commit 3afa5b7 into getodk:master Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing „form resume” in audit log events when moving backward in not allowed in Collect settings
4 participants