mitodl / micromasters

Portal for learners and course teams to access MITx Micromasters® programs
https://mm.mit.edu
BSD 3-Clause "New" or "Revised" License
29 stars 17 forks source link

FinancialAidSkipView needs a serializer #2429

Open singingwolfboy opened 7 years ago

singingwolfboy commented 7 years ago

Sentry: https://sentry.io/mit-office-of-digital-learning/micromasters/issues/208583737/

Visiting https://micromasters.mit.edu/api/v0/financial_aid_skip/1/ while logged in triggers a 500 exception on production. Reproducing this locally produces an AssertionError with the following error message:

'FinancialAidSkipView' should either include a serializer_class attribute, or override the get_serializer_class() method.

Traceback:

File "/usr/local/lib/python3.5/site-packages/django/core/handlers/exception.py" in inner
  39.             response = get_response(request)

File "/usr/local/lib/python3.5/site-packages/django/core/handlers/base.py" in _legacy_get_response
  249.             response = self._get_response(request)

File "/usr/local/lib/python3.5/site-packages/django/core/handlers/base.py" in _get_response
  217.                 response = self.process_exception_by_middleware(e, request)

File "/usr/local/lib/python3.5/site-packages/django/core/handlers/base.py" in _get_response
  215.                 response = response.render()

File "/usr/local/lib/python3.5/site-packages/django/template/response.py" in render
  109.             self.content = self.rendered_content

File "/usr/local/lib/python3.5/site-packages/rest_framework/response.py" in rendered_content
  72.         ret = renderer.render(self.data, accepted_media_type, context)

File "/usr/local/lib/python3.5/site-packages/rest_framework/renderers.py" in render
  701.         context = self.get_context(data, accepted_media_type, renderer_context)

File "/usr/local/lib/python3.5/site-packages/rest_framework/renderers.py" in get_context
  635.         raw_data_put_form = self.get_raw_data_form(data, view, 'PUT', request)

File "/usr/local/lib/python3.5/site-packages/rest_framework/renderers.py" in get_raw_data_form
  548.                     serializer = view.get_serializer(instance=instance)

File "/usr/local/lib/python3.5/site-packages/rest_framework/generics.py" in get_serializer
  109.         serializer_class = self.get_serializer_class()

File "/usr/local/lib/python3.5/site-packages/rest_framework/generics.py" in get_serializer_class
  126.             % self.__class__.__name__

Exception Type: AssertionError at /api/v0/financial_aid_skip/1/
Exception Value: 'FinancialAidSkipView' should either include a `serializer_class` attribute, or override the `get_serializer_class()` method.
singingwolfboy commented 7 years ago

https://sentry.io/mit-office-of-digital-learning/micromasters/issues/208583737/

pdpinch commented 7 years ago

Is this a user-facing error? Do you know if this is a regression?

singingwolfboy commented 7 years ago

It's user-facing in the sense that a user could discover it, but I don't think it's preventing users from doing anything on the production site. I discovered this bug while trying to manually test a pull request related to coupons, but I suspect that I uncovered it because the course price was negative.

singingwolfboy commented 7 years ago

However, I should add that this is a totally silent failure in the UI. Workflow:

At the very least, we should display an error message to the user, in the event that something goes wrong like this.

pdpinch commented 7 years ago

So the only time we've seen this happen is when the course price is negative?

But if we wrote a proper serializer we could return an error message to the user in the case of any 500 error from this API?

singingwolfboy commented 7 years ago

So the only time we've seen this happen is when the course price is negative?

Correct. However, I don't know that it couldn't happen with a positive course price.

But if we wrote a proper serializer we could return an error message to the user in the case of any 500 error from this API?

A proper serializer could return an error message, but it wouldn't be displayed to the user unless the Javascript was set up to do so. I just made #2430 to display an error message, independent of the serializer in the backend.