Survey publishing process silently skips groups and questions with bad order values.

akvo / akvo-flow

A data collection and monitoring tool that works anywhere.

http://akvo.org/products/akvoflow/

GNU Affero General Public License v3.0

65 stars 31 forks source link

Survey publishing process silently skips groups and questions with bad order values. #2251

Closed stellanl closed 4 years ago

stellanl commented 7 years ago

If there are duplicated "order" values for groups in a survey or questions in a group, only one of those groups/questions will appear in the published XML file. This is because the lists of groups/questions are kept as TreeMaps with order keys, so later items overwrite earlier ones with identical order values.

I see some options:

To make for more correct surveys, the publishing process should fail, and then notify the user (as it does if S3 is unavailable). This is means they will have to ask support to fix it, and/or we could run structure checks automatically to alert us to these problems.
To make a more fail-tolerant process, the publishing could go ahead and send everything with the bad order values to the app. This will look a bit bad, and may confuse data collectors.
Publishing could start by reordering things if necessary.
We could check for and flag order problems in the UI, so that they are noticed sooner.

Changes probably belong in org.waterforpeople.mapping.app.web.SurveyAssemblyServlet.assembleSurveyOnePass(Long)

janagombitova commented 7 years ago

Related issue regarding wrong order of questions and question groups https://github.com/akvo/akvo-flow/issues/1393

kymni commented 6 years ago

Should be fixed by #2556

janagombitova commented 6 years ago

@kymni in that case I will add the issue to the milestone to make sure to test it and confirm all works well

janagombitova commented 6 years ago

@stellanl and @kymni We do not get bad order values anymore. So this part of the issue is resolved.

I cannot test on UAT1 as I cannot find a survey with such bad values. I could test on UAT2, as there we have a survey matching the criteria. But then the expectation should be that the publishing skips the bad ordered questions, right?

The question is, what about surveys with bad order values? Is there anything automagical we can do for them? Should we?

stellanl commented 6 years ago

A datascript could be written (in about an hour) to repair the order fields. Or we might make it a regular cleanup operation. The chances of messed-up order fields have been reduced, probably significantly, but are not zero.

stellanl commented 6 years ago

I can also intentionally mess up the order in a survey on uat1 from the datastore manager.

janagombitova commented 6 years ago

@stellanl I think it is a good idea to clean up all survey forms on all instances to ensure that the order is correct. Then publishing won't fail and the fix from 1.9.30 will prevent most cases to happen again.

Scheduling the clean up regularly sounds also reasonable. How often would you propose?

stellanl commented 6 years ago

Actually, the recent form-export flap revealed that some (non-time-critical backend) calls to list questions in a group already does an order fixup. I propose to inventory this and any similar things for question groups.

muloem commented 6 years ago

I see some options:

To make for more correct surveys, the publishing process should fail, and then notify the user (as it does if S3 is unavailable). This is means they will have to ask support to fix it, and/or we could run structure checks automatically to alert us to these problems.

I am definitely in favour of some kind of validation of the survey structure before publishing to prevent the problems from going further down the line to the app for instance. In fact I believe there is an issue for it #1324 that we had proposed to work on this kind of validation.

To make a more fail-tolerant process, the publishing could go ahead and send everything with the bad order values to the app. This will look a bit bad, and may confuse data collectors.

If we allow bad data to go this far cleaning up will be that much harder so I prefer avoiding this option.

janagombitova commented 6 years ago

So I see two problems here:

What should we do with forms created before the last release that have broken order? - could be handled in this issue
How to strengthen validation when editing a survey to make sure that only a good survey is publishable? - could be handled in issue #1324

kymni commented 6 years ago

@janagombitova, @stellanl, @muloem given that we handled the issue causing the bad ordering in #2556, should we close this or keep it open to handle potential cases of old questions and questions groups with bad orders?

stellanl commented 6 years ago

While #2556 lessened the risk of bad ordering, it did not completely eliminate it. The creation of a new question and the reordering of the others are still 2 separate http calls, and one of them can fail while the other succeeds.

muloem commented 5 years ago

@kymni I think we should keep this open for now. There is lots of other validation that needs to be handled.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.