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

Exposes submissions url to enketo #1526

Merged
merged 31 commits into from
Mar 11, 2019
Merged

Exposes submissions url to enketo #1526

merged 31 commits into from
Mar 11, 2019

Conversation

WinnyTroy
Copy link
Contributor

@WinnyTroy WinnyTroy commented Dec 21, 2018

This PR contains the functionality to expose Enketo's single submission url.

@WinnyTroy WinnyTroy changed the title [WIP]Exposes submissions url to enketo Exposes submissions url to enketo Dec 21, 2018
onadata/libs/tests/utils/test_viewer_tools.py Outdated Show resolved Hide resolved
onadata/libs/utils/viewer_tools.py Outdated Show resolved Hide resolved
onadata/libs/utils/viewer_tools.py Outdated Show resolved Hide resolved
onadata/libs/utils/viewer_tools.py Outdated Show resolved Hide resolved
onadata/libs/tests/utils/test_viewer_tools.py Show resolved Hide resolved
onadata/libs/tests/utils/test_viewer_tools.py Outdated Show resolved Hide resolved
@@ -367,3 +366,27 @@ def get_enketo_preview_url(request, username, id_string, xform_pk=None):
raise EnketoError(response['message'])

return False


def get_submission_url(request, username, id_string, xform_pk=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function is not named properly. It is supposed to return the Enketo URL for a single submit survey, probably get_single_submit_url() would be more appropriate. The function docs should also reflect that.


def get_submission_url(request, username, id_string, xform_pk=None):
"""Return submission url of the submission instance."""
enketo_url = settings.ENKETO_URL + "/api/v2/survey/single/once"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assign "/api/v2/survey/single/once" to a static variable or better yet a settings variable, see examples above in the enketo_url() function.

enketo_url = settings.ENKETO_URL + getattr(settings, 'ENKETO_SINGLE_SUBMIT_PATH', "/api/v2/survey/single/once")

data = response.json()
submission_url = data['single_url']
return submission_url
elif response.status_code == 400:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are raising the same exception with the same error message we probably should only have only the else clause. Check enketo_url() function, could be something that might need to be refactored into a function that can be used by both.

Copy link
Member

@ukanga ukanga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we do not have an issue for this pull request. From what I can tell in the message, we are supposed to be adding an API endpoint that will allow us to create the single submit Enketo URL. This PR only includes the functionality to make an API request to Enketo but does not expose the feature in the API. Can we:

  • Expose the functionality on an API endpoint
  • Update API docs on making use of this functionality.

@@ -110,6 +110,7 @@ pyxform==0.12.2
raven==6.9.0
recaptcha-client==1.0.6
requests==2.20.1 # via datapackage, django-oauth-toolkit, httmock, sphinx, tableschema, tabulator
requests-mock==1.5.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use pip-tools to generate the pip requirements file. As such we also need to add the required modules into the setup.py under install_requires otherwise this package will not be included next time we regenerate the pip files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted. will include the packages there as well.


with self.assertRaises(EnketoError):
get_single_submit_url(
request, username='Milly', id_string="tag_team", xform_pk=1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request is not being mocked as was done in test_get_single_submit_url(), what EnketoError exception is being tested exactly? Can we apply a mock to the request object for very specific EnketoError exception? We could also check the exception message.

@WinnyTroy WinnyTroy force-pushed the Expose_submissions_endpoint branch 2 times, most recently from 7ab7184 to 402b4a3 Compare January 13, 2019 13:41
@WinnyTroy WinnyTroy force-pushed the Expose_submissions_endpoint branch 5 times, most recently from b93828a to 3c110a8 Compare January 23, 2019 11:26
@lincmba lincmba force-pushed the Expose_submissions_endpoint branch 2 times, most recently from 941511c to a758205 Compare January 23, 2019 12:38
@WinnyTroy WinnyTroy force-pushed the Expose_submissions_endpoint branch 2 times, most recently from 4e9f3b6 to 7ba2cdf Compare January 24, 2019 14:30
@lincmba lincmba force-pushed the Expose_submissions_endpoint branch 2 times, most recently from c73721b to 391b09f Compare February 19, 2019 08:30
@lincmba lincmba force-pushed the Expose_submissions_endpoint branch from 9d18048 to 391b09f Compare March 1, 2019 12:28
@WinnyTroy WinnyTroy force-pushed the Expose_submissions_endpoint branch 3 times, most recently from 0b7559b to 73c46b1 Compare March 1, 2019 12:52
@lincmba lincmba force-pushed the Expose_submissions_endpoint branch from 73c46b1 to ec457c2 Compare March 1, 2019 13:04
@ukanga ukanga merged commit 9f57942 into master Mar 11, 2019
@ukanga ukanga deleted the Expose_submissions_endpoint branch March 11, 2019 19:16
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.

3 participants