Skip to content

Refactor: Class-based views - Eligibility index #2913

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor: Class-based views - Eligibility index #2913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f333f2
refactor(eligibility): update eligibility index view to class-based view
machikoyasuda May 13, 2025
c390004
refactor(eligibility): use FormView instead of View
machikoyasuda May 14, 2025
9078671
fix(tests): fix tests to reflect how CBVs return template names
machikoyasuda May 14, 2025
35225b7
fix(eligibility): add safety checks on agency
machikoyasuda May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benefits/eligibility/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
app_name = "eligibility"
urlpatterns = [
# /eligibility
path("", views.index, name=routes.name(routes.ELIGIBILITY_INDEX)),
path("", views.IndexView.as_view(), name=routes.name(routes.ELIGIBILITY_INDEX)),
path("start", views.start, name=routes.name(routes.ELIGIBILITY_START)),
path("confirm", views.confirm, name=routes.name(routes.ELIGIBILITY_CONFIRM)),
path("unverified", views.unverified, name=routes.name(routes.ELIGIBILITY_UNVERIFIED)),
Expand Down
84 changes: 49 additions & 35 deletions benefits/eligibility/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,53 +7,67 @@
from django.template.response import TemplateResponse
from django.urls import reverse
from django.utils.decorators import decorator_from_middleware
from django.views.generic import FormView

from benefits.routes import routes
from benefits.core import recaptcha, session
from benefits.core.middleware import AgencySessionRequired, RecaptchaEnabled, FlowSessionRequired
from benefits.core.mixins import AgencySessionRequiredMixin, RecaptchaEnabledMixin
from benefits.core.models import EnrollmentFlow
from . import analytics, forms, verify

TEMPLATE_CONFIRM = "eligibility/confirm.html"


@decorator_from_middleware(AgencySessionRequired)
@decorator_from_middleware(RecaptchaEnabled)
def index(request):
class IndexView(AgencySessionRequiredMixin, RecaptchaEnabledMixin, FormView):
"""View handler for the enrollment flow selection form."""
agency = session.agency(request)
session.update(request, eligible=False, origin=agency.index_url)

# 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)

context = {"form": forms.EnrollmentFlowSelectionForm(agency=agency)}

if request.method == "POST":
form = forms.EnrollmentFlowSelectionForm(data=request.POST, agency=agency)

if form.is_valid():
flow_id = form.cleaned_data.get("flow")
flow = EnrollmentFlow.objects.get(id=flow_id)
session.update(request, flow=flow)

analytics.selected_flow(request, flow)

eligibility_start = reverse(routes.ELIGIBILITY_START)
response = redirect(eligibility_start)
else:
# form was not valid, allow for correction/resubmission
if recaptcha.has_error(form):
messages.error(request, "Recaptcha failed. Please try again.")
context["form"] = form
context.update(agency.eligibility_index_context)
response = TemplateResponse(request, "eligibility/index.html", context)
else:
context.update(agency.eligibility_index_context)
response = TemplateResponse(request, "eligibility/index.html", context)

return response
template_name = "eligibility/index.html"
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this template name be defined on line 19, under TEMPLATE_CONFIRM, for consistency?

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 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.

form_class = forms.EnrollmentFlowSelectionForm

def setup(self, request, *args, **kwargs):
"""Initialize view attributes."""
super().setup(request, *args, **kwargs)
self.agency = session.agency(request)

def get_form_kwargs(self):
"""Return the keyword arguments for instantiating the form."""
kwargs = super().get_form_kwargs()
kwargs["agency"] = self.agency
return kwargs

def dispatch(self, request, *args, **kwargs):
"""Initialize session state before handling the request."""
if not self.agency:
return TemplateResponse(request, "200-user-error.html")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@thekaveman thekaveman May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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):

  1. IndexView
  2. AgencySessionRequiredMixin
  3. View (e.g., django.views.View)
  4. 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.


session.update(request, eligible=False, origin=self.agency.index_url)
# 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)
return super().dispatch(request, *args, **kwargs)

def form_valid(self, form):
"""If the form is valid, set enrollment flow and redirect."""
flow_id = form.cleaned_data.get("flow")
flow = EnrollmentFlow.objects.get(id=flow_id)
session.update(self.request, flow=flow)

analytics.selected_flow(self.request, flow)
return redirect(reverse(routes.ELIGIBILITY_START))

def form_invalid(self, form):
"""If the form is invalid, display error messages."""
if recaptcha.has_error(form):
messages.error(self.request, "Recaptcha failed. Please try again.")
return super().form_invalid(form)

def get_context_data(self, **kwargs):
"""Add agency-specific context data."""
context = super().get_context_data(**kwargs)
if self.agency:
context.update(self.agency.eligibility_index_context)
return context


@decorator_from_middleware(AgencySessionRequired)
Expand Down
4 changes: 2 additions & 2 deletions tests/pytest/eligibility/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def test_index_get_agency_multiple_flows(mocker, model_TransitAgency, model_Enro
response = client.get(path)

assert response.status_code == 200
assert response.template_name == "eligibility/index.html"
assert "eligibility/index.html" in response.template_name
assert "form" in response.context_data
assert isinstance(response.context_data["form"], EnrollmentFlowSelectionForm)

Expand All @@ -143,7 +143,7 @@ def test_index_get_agency_single_flow(mocker, model_TransitAgency, model_Enrollm
response = client.get(path)

assert response.status_code == 200
assert response.template_name == "eligibility/index.html"
assert "eligibility/index.html" in response.template_name
assert "form" in response.context_data
assert isinstance(response.context_data["form"], EnrollmentFlowSelectionForm)

Expand Down
Loading