getodk / collect

ODK Collect is an Android app for filling out forms. It's been used to collect billions of data points in challenging environments around the world. Contribute and make the world a better place! ✨📋✨
https://docs.getodk.org/collect-intro
Other
718 stars 1.38k forks source link

ODK Collect 1.28.0 beta does not hounour skip logic for repeated groups #4090

Closed florianm closed 4 years ago

florianm commented 4 years ago

Software and hardware versions

Collect v1.28.0 beta ODK Central 0.6 Form: Turtle Track or Nest 1.0.zip Hw: Lenovo Tab3 (the one with the GPS accuracy bug fixed by using fused location provider in Collect 1.28), Vivo x50 lite (my mobile)

Problem description

This problem does not exist under ODK Collect 1.27.4 (non-beta), but exists under 1.28.0 (beta). It appears that ODK Central 1.28.0 does not recognize form group "relevant" skip logic, and always shows "add another XXX group?" dialogue.

Steps to reproduce the problem

Expected behavior

Skip logic for screens is handled correctly in v1.28.0. Since we have enumerators currently in the field using the Lenovo Tab3, we would love to use the newly fixed GPS accuracy in 1.28.

Other information

Google Play > ODK Collect > leave beta, uninstall > tap furiously around until "install" button returns > install > ODK Collect v1.27.4 is back > expected behaviour is restored.

seadowg commented 4 years ago

@florianm are you able to share the XLSX for the form as well?

seadowg commented 4 years ago

I am seeing a difference in behaviour with this smaller form I made: https://docs.google.com/spreadsheets/d/1pc9LPOR8s5C2uFjMj1JbuF6_PPLQ_Q2l6X6ytAZvul0/edit?usp=sharing

In 1.27.4:

  1. Open form
  2. Hit "No"
  3. Goes to end of form

In master:

  1. Open form
  2. Hit "No"
  3. Prompts to add "Repeat"

It's like the default first repeat is missing (because of the relevant) but that it still prompts to add another for some reason. Very weird. Not sure what in this release could have caused the change but will keep investigating.

Edit: Documented behaviour is here: http://xlsform.org/en/#only-add-repeats-in-certain-conditions

florianm commented 4 years ago

Thanks for the quick reply! The form was built in ODK Build > XForm > ODK Central. The form has received 10971 submissions since Aug 2019. Worked a treat so far (including the "relevant" expressions). Both in ODK Build, and in the XSLX version exported from ODK Build, my "relevant" clauses are plain XPath expressions like

/data/nest/eggs_counted = 'yes'

whereas the XSLX docs use escaped variable names like

${has_child} = 'yes'

I've published the XForm def at https://sandbox.central.getodk.org/#/projects/44/forms/build_Turtle-Track-or-Nest-1-0_1559789920

Zip with .odkbuild, .xml and .xslx versions: Turtle Track or Nest.zip

seadowg commented 4 years ago

Ok it looks like it's probably the JavaRosa upgrade as the bug is fixed if I use 2.17.1 instead of 3.0.0 on master. Most likely we need to solve the issue there. I'll keep digging!

seadowg commented 4 years ago

Looks like it could be related to this change: https://github.com/getodk/javarosa/pull/581

lognaturel commented 4 years ago

Thanks so much for checking and reporting, @florianm! Looks like we're close to a fix.

florianm commented 4 years ago

Thanks for the rapid turnaround! Just for our planning - should we tell the field team to wait for the update or to downgrade to stable?

seadowg commented 4 years ago

@florianm we'll probably have a fix by the end of today but putting out a new beta once merged could take anywhere between 24 hours to a week due to potential review and propagation time. I'd say you can safely expect a fixed beta next week but harder to guarantee earlier. We can ping you when it's "out" 😀

florianm commented 4 years ago

Thanks for the update, Callum! Ping would be much appreciated in case I miss it.

lognaturel commented 4 years ago

Once again, huge thanks for reporting this. The fix we have only addresses part of the issue, unfortunately. I don't yet have a sense of how long fixing the rest might take but I'll keep you updated. I hopefully will have an estimate in the next 3-4 hours.

lognaturel commented 4 years ago

@florianm would you be able to share the XLSForm? That would help with troubleshooting. You can attach here, link to your repo, or send to me privately. (I think you do this in Build -- a Build XLSForm export is great)

florianm commented 4 years ago

I've published the XForm def at https://sandbox.central.getodk.org/#/projects/44/forms/build_Turtle-Track-or-Nest-1-0_1559789920

Zip with .odkbuild, .xml and .xslx versions: Turtle Track or Nest.zip

@lognaturel here it is, inside the zip!

florianm commented 4 years ago

Are there simple fixes to the form def we could implement?

lognaturel commented 4 years ago

Thank you, @florianm!

Are there simple fixes to the form def we could implement?

I'm afraid I don't know yet. The form has a lot of logic and I'm not sure what part is leading to unexpected behavior. Will look for form design changes too and let you know if I find anything. It's definitely a bug somewhere that needs to be fixed.

EDIT: whoops, sorry I didn't see the additional files in my first read through the thread.

lognaturel commented 4 years ago

Still working on it @florianm and will keep you posted as I get a better sense of ETA.

lognaturel commented 4 years ago

Progress so far is at getodk/javarosa#603. I do have a test that isolates the root cause. I'm hoping that with a clear head tomorrow I can come up with a solution.

lognaturel commented 4 years ago

We have a fix but I think Monday would be the earliest we'd have a beta out. I can post an extra-beta APK if that's helpful, @florianm.

florianm commented 4 years ago

Thanks so much for the updates and for the fix! Our impacted field team runs their last survey today, and they have chosen to stick with the additional dialogues rather than downgrade. So for us a fix next week would be absolutely fine. That will still give other teams a chance to upgrade before heading out again.

Re form logic, the skip logic impacted here is

I'm happy to update our XPath expressions or any other underlying form plumbing if there's a better way to express above logic. E.g. using quoted ${field code} instead of /data/group/field XPaths.

lognaturel commented 4 years ago

if there's a better way to express above logic.

I can't think of any alternatives. It's really just that we introduced a bug. If you're curious, there's a difference in how concrete repeated instances (the 1st, 2nd, etc) and the repeat prompt are represented internally and we had essentially lost the prompt case for relevance. That was easy enough to fix but it revealed a related issue when two elements have the same relevance and one of them is a repeat. We have automated tests for these cases now and are grateful to you for bringing them to our attention.

I just published a beta with the fix. Based on our recent experience with the other betas, I'm optimistic that it will be available within a few hours.

lognaturel commented 4 years ago

@florianm I want to make sure that you saw that the updated beta is out. We believe we are now ready for release. Do check it out if you have a moment!

florianm commented 4 years ago

Cheers, just saw the beta 5 available. Will test shortly!