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

Bootstrap file generation: add surveyId to the xml file #1927

Closed valllllll2000 closed 7 years ago

valllllll2000 commented 7 years ago

When generating the survey bootstrap file, the folder containing the survey xml file has this structure: surveyId/name_of_survey.xml. It seems according to the code that this is the structure that will always be used but I have found while working on the bug https://github.com/akvo/akvo-flow-mobile/issues/614 that sometimes the folder has this structure: surveyId/survey_name/survey_name.xml. Not sure why this happens but since the app uses the folder name for getting the surveyId which is not very "safe". The best solution, in my opinion would be to add the surveyId field inside the xml file in the SurveyAssemblyServlet.

@stellanl @muloem would like you guys opinion on this, thanks.

stellanl commented 7 years ago

The only reason to not have the surveyId in the xml file is if you wanted to make it easy to change the id by renaming the file. We have the opposite case - nothing works unless it matches the correct survey definition in our db. Of course, any change to file formats creates compatibility issues. We have to think about what happens when a new app reads an old bootstrap file and the other way around. Do we have any idea when/why some bootstrap files have the wrong structure?

valllllll2000 commented 7 years ago

@stellanl so the idea is to change the code in backend to include the survey id in the xml file. The folder naming will stay the same. Old apps reading new xml file will use their old code and ignore the new xml field. The new app version will start using the surveyId from the xml file if present or just the old folder name if not. Checking the code I have no idea why this structure is used and it happens only for WFP (they did say they copied some surveys around).

stellanl commented 7 years ago

Sounds good. You seem to have all the angles covered.

valllllll2000 commented 7 years ago

Test plan

Create a small survey, publish and then go to manuel survey transfer and send it to yourself by email. Open the zip and the form xml file and check that surveyId field is there and corresponds to the folder name. For example if you have a survey with the id 123, inside the zip file you will have 123/survey_name.xml and opening the survey_name.xml you will have a field surveyId=123. Try this with a big survey as well (more than 80 questions).

muloem commented 7 years ago

@valllllll2000 @janagombitova just adding a comment here that the issue seems to be caused by cases where forms have a "/" in the name e.g. Form One/Collecting Data

muloem commented 7 years ago

@valllllll2000 why the test with over 80 questions?

muloem commented 7 years ago

@valllllll2000 maybe we can also add an extra change here... if there are any "/" (and perhaps any characters not allowed in Filename) in the form name we replace them with an underscore just like the spaces.

Like here: https://stackoverflow.com/questions/1184176/how-can-i-safely-encode-a-string-in-java-to-use-as-a-filename/1184185#1184185

valllllll2000 commented 7 years ago

@muloem 1) Because https://github.com/akvo/akvo-flow/blob/develop/GAE/src/org/waterforpeople/mapping/app/web/SurveyAssemblyServlet.java#L138 2) Ok will do it now

muloem commented 7 years ago

1) Because https://github.com/akvo/akvo-flow/blob/develop/GAE/src/org/waterforpeople/mapping/app/web/SurveyAssemblyServlet.java#L138

Ah! that devil in the details! 👍