-
Notifications
You must be signed in to change notification settings - Fork 81
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
Questionnaire API #938
Questionnaire API #938
Conversation
52d67c1
to
ad59de8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spotted a couple of issues that need to be investigated before merging. Other than those, it's looking pretty good..
QUESTION_TYPES = dict(Question.TYPE_CHOICES) | ||
|
||
|
||
class XFormRenderer(BaseRenderer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XFormRenderer
doesn't populate the meta/instanceID
element of the rendered xform. This is required to detect duplicate xform submissions. The meta/instanceID
element could be populated either by adding a uuid
directly to the meta/instanceID
element in the same way as the version
is currently populated or by adding something like <bind calculate="concat('uuid:', uuid())" nodeset="/amwkg2a86v48hcaykrc7myng/meta/instanceID" readonly="true()" type="string"/>
to the xform model.
QUESTION_TYPES = dict(Question.TYPE_CHOICES) | ||
|
||
|
||
class XFormRenderer(BaseRenderer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions in the rendered xform are not in the the order defined in the xlsform. This matters I think as the form designer probably wants questions answered in order. Also elements like title
which provide an introduction to the form in ODK Collect are appearing in the middle of the question sequence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed this. The order of questions and groups is now determined through the field index
, which I added to both Question
and QuestionGroup`. This also adds a migration to populate the index field for existing questionnaires.
) | ||
instance.md5_hash = self.get_hash( | ||
instance.filename, instance.id_string, instance.version | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 209 - 211 are not required here, they've been moved to the render
method in the XFormRenderer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I left it in there on purpose because this is the part that throws exceptions that are shown to the user as error messages. If we remove it here, we will have to rewrite error handling for XLSForms. I'm not sure if it is worth doing it, since we are thinking about replacing XLSForms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok.. sounds good.
def create(self, request, *args, **kwargs): | ||
if request.method.upper() == 'HEAD': | ||
return Response(headers=self.get_openrosa_headers(request), | ||
status=status.HTTP_204_NO_CONTENT,) | ||
print('REQUEST') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
@@ -41,10 +41,15 @@ class XFormSubmissionViewSet(OpenRosaHeadersMixin, | |||
parser_classes = (FormParser, MultiPartParser,) | |||
serializer_class = XFormSubmissionSerializer | |||
|
|||
def dispatch(self, *args, **kwargs): | |||
print(args[0].__dict__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
0aaad8d
to
61c4389
Compare
@oliverroick sounds like a plan! |
bcfe91a
to
ca4986a
Compare
ca4986a
to
66bc9b5
Compare
d2d3571
to
46c0f86
Compare
- Block questionnaires from uploading - Render XForm from questionnaire instance - Add XForm rendering to API view - Extend API to update questioanaires
46c0f86
to
db8bf8e
Compare
Proposed changes in this pull request
When should this PR be merged
For sprint 11
Risks
The migration to populate
index
fields forQuestion
andQuestiongroup
should be tested on staging against the production database to make sure it works with all XLSForms we currently have in production.This PR changes the way XForms are created and provided to GeoODK. I have tested with most questionnaires that we use in production but there is a chance that I haven't though of all possible edge cases. Loading forms into GeoODK, collecting and submitting data should be carefully tested on staging.
Follow up actions
Missing permissions were added to policies, when deploying
./manage.py loadpolicies --update
needs to be run. \cc @amplifi