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

Fixing #272 #652

Merged
merged 2 commits into from
Sep 8, 2016
Merged

Fixing #272 #652

merged 2 commits into from
Sep 8, 2016

Conversation

bjohare
Copy link
Contributor

@bjohare bjohare commented Sep 8, 2016

Proposed changes in this pull request

Fixes #272

Errors in parsing XLSForms are now handled as validation errors in the project creation wizard. This PR also fixes inconsistencies in spelling of questionnaire throughout the platform.

When should this PR be merged

Immediately

Risks

None

Follow up actions

Policies will need to be updated the next time the platform is deployed by running:

./manage.py loadstatic --update-policies

@ian-ross
Copy link
Contributor

ian-ross commented Sep 8, 2016

Maybe I'm being really stupid here, but I just can't get this to work the way I expect it to. I deliberately broke an XLSForms sheet, then tried to use it. You can see the result at https://vimeo.com/181925548 -- that isn't right, is it?

@bjohare
Copy link
Contributor Author

bjohare commented Sep 8, 2016

Hmm.. not good. I'll take another look.

@ian-ross
Copy link
Contributor

ian-ross commented Sep 8, 2016

I'm suspicious that I'm doing something wrong, but I can't figure what it would be. I checked out your branch, did the policy update, restarted the server, and then saw what you see in that video. (Incidentally, I see the same 500 when I edit the project details and upload a broken questionnaire, which is a separate issue.)

@bjohare
Copy link
Contributor Author

bjohare commented Sep 8, 2016

Well if it's not working it's not working.. its strange since PyXForm errors are being caught and re-raised as InvalidXLSForm errors which are then handled in the Project Add wizard.. Oh well! Will look again..

@ian-ross
Copy link
Contributor

ian-ross commented Sep 8, 2016

OK, getting better! Now, if I provide an invalid form, I get an error message when I press the "Finish" button on the last page of the new project wizard, and I get redirected back to the project details page to choose a new form to upload.

However, I thought the idea here was to detect the bad form directly on the details page, so that you would get the error message immediately when you press the "Next" button on the details page, without going through the permissions page first. In what you've done, you only check the questionnaire at the end of the whole wizard process, in the done() method. To get the desired behaviour, you somehow need to check it earlier, at the point of transition between the details and permissions pages.

If you think that's going to be a pain to do, we could merge this PR as is, and open a new issue to get the error detection to exactly the right point. Iterative refinement, and all that...

@ian-ross
Copy link
Contributor

ian-ross commented Sep 8, 2016

(Incidentally, I seem to remember thinking that the process_step() method in the wizard might be the place to do this, but I don't know how much you can realistically do in there. I think some fossicking through the formtools source might be needed to work out exactly how to do this.)

@bjohare
Copy link
Contributor Author

bjohare commented Sep 8, 2016

The difficulty is that there are two layers of validation during XLSForm upload:

  1. PyXForm parses the incoming XLSForm and throws a PyXFormError if the form is invalid. This currently happens during the done() method, but we could potentially do this during process_step().
  2. QuestionnaireManager.create_from_form() runs a second set of checks during Question, QuestionGroup and QuestionOption creation that rely on a reference to the project which doesn't exist until the the wizard done() method is called. However I think these second set of tests are redundant during form upload as PyXForm seems to catch them.., they only become relevant when creating Questions in a yet to be developed UI.

Would be inclined to merge this and open an Issue to investigate the relationship between creating Questions, XLSForm uploads and the proposed Questionnaire creation UI.

@ian-ross
Copy link
Contributor

ian-ross commented Sep 8, 2016

I can live with that. I'll merge now, if you'll create a new issue for those things (you can explain it better than me!).

@ian-ross ian-ross merged commit 0fe8f50 into master Sep 8, 2016
@ian-ross ian-ross deleted the enhancement/#272 branch September 8, 2016 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors in Parsing XLSForm Should Happen Sooner than the Project Overview Page
2 participants