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

Handle error thrown by urllib #1765

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Handle error thrown by urllib #1765

merged 1 commit into from
Jan 16, 2020

Conversation

DavisRayM
Copy link
Contributor

@DavisRayM DavisRayM commented Jan 10, 2020

Changes / Features implemented

  • Handle BadStatusLine error raised when "a server responds with a HTTP status code that we don’t understand."

Steps taken to verify this change does what is intended

  • Tested on a local server of onadata
  • Added a test test_error_raised_xform_url_upload_urllib_error

Side effects of implementing this change

N/A

Closes #1759

@DavisRayM DavisRayM changed the title Handle error throen by urllib Handle error thrown by urllib Jan 10, 2020
@DavisRayM DavisRayM changed the title Handle error thrown by urllib [WIP]Handle error thrown by urllib Jan 10, 2020
@DavisRayM DavisRayM changed the title [WIP]Handle error thrown by urllib Handle error thrown by urllib Jan 14, 2020
Copy link
Contributor

@ivermac ivermac left a comment

Choose a reason for hiding this comment

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

Looks good. Can we add a test for it by mocking the function that triggers BadStatusLine? We have an example of how to mock an exception by using a side_effect as seen below:

@mock.patch('onadata.libs.data.query._execute_query',
side_effect=raise_data_error)
def test_get_on_date_field_with_invalid_data(self, mock_execute_query):
data = {'field_name': 'date'}
request = self.factory.get('/charts', data)
force_authenticate(request, user=self.user)
response = self.view(
request,
pk=self.xform.id)
self.assertEqual(response.status_code, 400)

@DavisRayM
Copy link
Contributor Author

Looks good. Can we add a test for it by mocking the function that triggers BadStatusLine? We have an example of how to mock an exception by using a side_effect as seen below:

@mock.patch('onadata.libs.data.query._execute_query',
side_effect=raise_data_error)
def test_get_on_date_field_with_invalid_data(self, mock_execute_query):
data = {'field_name': 'date'}
request = self.factory.get('/charts', data)
force_authenticate(request, user=self.user)
response = self.view(
request,
pk=self.xform.id)
self.assertEqual(response.status_code, 400)

Added.

ivermac
ivermac previously approved these changes Jan 15, 2020
Copy link
Contributor

@ivermac ivermac left a comment

Choose a reason for hiding this comment

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

lgtm!

return {
'type': 'alert-error',
'text': _(u'An error occured while trying to retrieve the form.')
}
Copy link
Member

Choose a reason for hiding this comment

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

What would the action that would be required by the user to take if they encounter this error?
Would the error message in https://github.com/onaio/onadata/pull/1765/files#diff-018f1a90f318bf1748393be886fee2c0R502-R503 be sufficient in this case? If it would be sufficient could we add BadStatusLine to except (BadStatusLine, MemoryError, OSError)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error message would be perfect for this scenario. I'll update my PR.

@WNjihia WNjihia added the QA+ PR passed QA testing label Jan 16, 2020
@WNjihia
Copy link

WNjihia commented Jan 16, 2020

Testing checks out @DavisRayM

@ukanga ukanga merged commit 3faabd3 into master Jan 16, 2020
@ukanga ukanga deleted the 1759-check-submitted-files branch January 16, 2020 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA+ PR passed QA testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check submitted file attributes before processing
4 participants