Refactor: Class-based views - Eligibility index #2913
Conversation
uses both AgencySessionRequiredMixin and RecaptchaEnabledMixin in class-based index view. updates url to reflect this change.
github-actions
bot
added
back-end
| # clear any prior OAuth token as the user is choosing their desired flow | ||
| # this may or may not require OAuth, with a different set of scope/claims than what is already stored | ||
| session.logout(request) | ||
| template_name = "eligibility/index.html" |
There was a problem hiding this comment.
Should this template name be defined on line 19, under TEMPLATE_CONFIRM, for consistency?
There was a problem hiding this comment.
I think these global variables for template names were an artifact of having view functions. With class-based views, one of the main benefits is that everything is more cohesive and together, so I don't think we need to keep using these global variables for template names.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
|
After reading #2870, I started thinking this could be written as a FormView instead. Any thoughts on which way to go? @angela-tran @lalver1 @thekaveman |
I probably have the least hands-on experience with class-based views, so take this with a grain of salt, but I think it's definitely worth looking into using a FormView and whether that makes the code simpler / more readable or not. (There is a form on this view after all.) |
machikoyasuda
force-pushed
the
refactor/2868-cbv-elig-index
branch
from
f0a290e to
c390004
Compare
| def dispatch(self, request, *args, **kwargs): | ||
| """Initialize session state before handling the request.""" | ||
| if not self.agency: | ||
| return TemplateResponse(request, "200-user-error.html") |
There was a problem hiding this comment.
Since this CBV inherits from the AgencySessionRequiredMixin, I don't think this logic is needed here in the dispatch() override.
Presumably if the request got this far, it has an agency in the session.
There was a problem hiding this comment.
Hmm this is a little more nuanced than I previously understood.
We have AgencySessionRequiredMixin which defines its logic in the dispatch() method. The original idea was that CBVs inheriting from this mixin would first have the mixin's dispatch() called (which does the check, sets the self.agency variable) and then if that was successful, fall through to the CBV's dispatch() (or return user_error if not successful).
This is sort of how it works.
How MRO affects dispatch calls
When we have a view like class IndexView(AgencySessionRequiredMixin, View), and view_instance.dispatch() is called, Python looks for the dispatch method in this order (simplified):
IndexViewAgencySessionRequiredMixinView(e.g.,django.views.View)- Other parent classes of
View(if any)
What this means for mixins
E.g. AgencySessionRequiredMixin relies on its own dispatch method being executed to perform the agency check.
Scenario A: The view using the mixin does NOT define dispatch
If IndexView does not have its own dispatch method, then AgencySessionRequiredMixin.dispatch is found and executed. The mixin runs its check, and if an agency is present, super().dispatch() in the mixin correctly passes control to View.dispatch.
Scenario B: The view using the mixin DOES define dispatch
If IndexView does define its own dispatch method (like in this PR), then IndexView.dispatch will be called instead of AgencySessionRequiredMixin.dispatch.
If IndexView.dispatch simply does its own thing and does not call super().dispatch(), then the mixin's dispatch logic is entirely bypassed.
However, if IndexView.dispatch does call super().dispatch() (like in this PR), then control will be passed to AgencySessionRequiredMixin.dispatch (as it's next in the MRO). The mixin can then perform its check, and its own super().dispatch() call will continue the chain to View.dispatch.
TLDR;
If possible, I think we should avoid implementing dispatch in our CBVs. Can this logic move to the get method or similar?
If not possible (e.g. we absolutely need to implement some logic in the CBVs dispatch method), it MUST call super().dispatch() early, before taking any action that would require the results of the mixin's dispatch (e.g. the session check). I don't think this will always (maybe never?) be possible, since the whole point of our *SessionRequiredMixins is to block the request. If we call super().dispatch() early in the view's dispatch, that is essentially like processing the entire request, generating a response, AND THEN checking for the agency in the session.
| def dispatch(self, request, *args, **kwargs): | ||
| """Initialize session state before handling the request.""" | ||
| if not self.agency: | ||
| return TemplateResponse(request, "200-user-error.html") |
There was a problem hiding this comment.
Hmm this is a little more nuanced than I previously understood.
We have AgencySessionRequiredMixin which defines its logic in the dispatch() method. The original idea was that CBVs inheriting from this mixin would first have the mixin's dispatch() called (which does the check, sets the self.agency variable) and then if that was successful, fall through to the CBV's dispatch() (or return user_error if not successful).
This is sort of how it works.
How MRO affects dispatch calls
When we have a view like class IndexView(AgencySessionRequiredMixin, View), and view_instance.dispatch() is called, Python looks for the dispatch method in this order (simplified):
IndexViewAgencySessionRequiredMixinView(e.g.,django.views.View)- Other parent classes of
View(if any)
What this means for mixins
E.g. AgencySessionRequiredMixin relies on its own dispatch method being executed to perform the agency check.
Scenario A: The view using the mixin does NOT define dispatch
If IndexView does not have its own dispatch method, then AgencySessionRequiredMixin.dispatch is found and executed. The mixin runs its check, and if an agency is present, super().dispatch() in the mixin correctly passes control to View.dispatch.
Scenario B: The view using the mixin DOES define dispatch
If IndexView does define its own dispatch method (like in this PR), then IndexView.dispatch will be called instead of AgencySessionRequiredMixin.dispatch.
If IndexView.dispatch simply does its own thing and does not call super().dispatch(), then the mixin's dispatch logic is entirely bypassed.
However, if IndexView.dispatch does call super().dispatch() (like in this PR), then control will be passed to AgencySessionRequiredMixin.dispatch (as it's next in the MRO). The mixin can then perform its check, and its own super().dispatch() call will continue the chain to View.dispatch.
TLDR;
If possible, I think we should avoid implementing dispatch in our CBVs. Can this logic move to the get method or similar?
If not possible (e.g. we absolutely need to implement some logic in the CBVs dispatch method), it MUST call super().dispatch() early, before taking any action that would require the results of the mixin's dispatch (e.g. the session check). I don't think this will always (maybe never?) be possible, since the whole point of our *SessionRequiredMixins is to block the request. If we call super().dispatch() early in the view's dispatch, that is essentially like processing the entire request, generating a response, AND THEN checking for the agency in the session.
closes #2868
This PR converts the Eligibility Index view method from a function-based view into a class-based view (CBV, reference here https://ccbv.co.uk/). This PR uses both mixins from #2875 and #2882.
What this PR does
Notes
assert ['eligibility/index.html'] == 'eligibility/index.html', b/cFormViewreturns a list of template names, instead of a string. I changed the test to match what FormView now does, instead of changing FormView itself to return a string like the test expects.