-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feat: in-person eligibility policies #2689
base: main
Are you sure you want to change the base?
Changes from all commits
17beaf8
8d37703
91c9236
082addd
dd7e06d
0a4f797
f0fa30e
21e0f69
87f9644
d27a53e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
from django.db import migrations | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("core", "0035_enrollmentflow_system_name_choices"), | ||
] | ||
|
||
def migrate_in_person_flows(apps, schema_editor): | ||
EnrollmentFlow = apps.get_model("core", "EnrollmentFlow") | ||
for flow in EnrollmentFlow.objects.all(): | ||
in_person = "in_person" # value of EnrollmentMethods.IN_PERSON as of this migration | ||
if in_person in flow.supported_enrollment_methods: | ||
if flow.system_name not in [ | ||
"senior", | ||
"medicare", | ||
"courtesy_card", | ||
]: # the keys in `in_person.context.eligibility_index` as of this migration | ||
flow.supported_enrollment_methods.remove(in_person) | ||
flow.save() | ||
|
||
operations = [migrations.RunPython(migrate_in_person_flows)] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
from .claims import ClaimsProvider | ||
from .transit import TransitAgency | ||
from benefits.core import context as core_context | ||
from benefits.in_person import context as in_person_context | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -257,8 +258,12 @@ def enrollment_success_template(self): | |
else: | ||
return self.enrollment_success_template_override or f"{prefix}--{self.transit_agency.slug}.html" | ||
|
||
@property | ||
def in_person_eligibility_context(self): | ||
return in_person_context.eligibility_index[self.system_name].dict() | ||
|
||
def clean(self): | ||
template_errors = [] | ||
errors = [] | ||
|
||
if self.transit_agency: | ||
templates = [ | ||
|
@@ -276,10 +281,19 @@ def clean(self): | |
# so just create directly for a missing template | ||
for t in templates: | ||
if not template_path(t): | ||
template_errors.append(ValidationError(f"Template not found: {t}")) | ||
errors.append(ValidationError(f"Template not found: {t}")) | ||
|
||
if EnrollmentMethods.IN_PERSON in self.supported_enrollment_methods: | ||
try: | ||
in_person_eligibility_context = self.in_person_eligibility_context | ||
except KeyError: | ||
in_person_eligibility_context = None | ||
|
||
if not in_person_eligibility_context: | ||
errors.append(ValidationError(f"In-person eligibility context not found for: {self.system_name}")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Open to suggestions on this error message 😅 I guess it would help if it told the user some sort of action to take, like to contact a dev or to uncheck There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm... yeah, this is a technically accurate error message! But since we show it to end-users, I think we probably want something more friendly here 😅 I agree with your action-oriented take, maybe something like:
|
||
|
||
if template_errors: | ||
raise ValidationError(template_errors) | ||
if errors: | ||
raise ValidationError(errors) | ||
|
||
def eligibility_form_instance(self, *args, **kwargs): | ||
"""Return an instance of this flow's EligibilityForm, or None.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
from .eligibility import eligibility_index | ||
|
||
__all__ = ["eligibility_index"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
from dataclasses import dataclass, asdict | ||
|
||
from benefits.core.context import SystemName | ||
|
||
|
||
@dataclass | ||
class EligibilityIndex: | ||
policy_details: str | ||
|
||
def dict(self): | ||
return asdict(self) | ||
|
||
|
||
eligibility_index = { | ||
SystemName.OLDER_ADULT.value: EligibilityIndex( | ||
policy_details="I confirmed this rider’s identity using a government-issued ID and verified they are age 65 or older." | ||
), | ||
SystemName.MEDICARE.value: EligibilityIndex( | ||
policy_details="I confirmed this rider’s identity using a government-issued ID and verified they possess a valid " | ||
"Medicare card." | ||
), | ||
SystemName.COURTESY_CARD.value: EligibilityIndex( | ||
policy_details="I confirmed this rider’s identity using a government-issued ID and verified they possess an MST " | ||
"Courtesy Card." | ||
), | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
{% extends "core/includes/form.html" %} | ||
|
||
{% block extra-scripts %} | ||
<script nonce="{{ request.csp_nonce }}"> | ||
ready(function() { | ||
let showCheckbox = function(flow_id) { | ||
let checkbox_parent = document.querySelector("[class^='form-group']:has([id='id_verified_" + flow_id + "'])") | ||
checkbox_parent.classList.remove("d-none"); | ||
|
||
let flow_verified_checkbox = checkbox_parent.querySelector("[id='id_verified_" + flow_id + "']"); | ||
flow_verified_checkbox.classList.remove("d-none"); | ||
flow_verified_checkbox.required = true; | ||
|
||
let flow_verified_label = checkbox_parent.querySelector("[for='id_verified_" + flow_id + "']"); | ||
flow_verified_label.classList.remove("d-none"); | ||
}; | ||
|
||
let hideOtherCheckboxes = function(flow_id) { | ||
let other_groups = document.querySelectorAll("[class^='form-group']:has([id^='id_verified_']:not([id='id_verified_" + flow_id + "']))"); | ||
other_groups.forEach(group => { | ||
group.classList.add("d-none"); | ||
|
||
let checkbox = group.querySelector("[id^='id_verified']"); | ||
checkbox.classList.add("d-none"); | ||
checkbox.required = false; | ||
checkbox.checked = false; | ||
|
||
group.querySelector("[for^='id_verified']").classList.add("d-none"); | ||
|
||
}); | ||
}; | ||
|
||
/* Add listener to radio buttons. */ | ||
let flow_radio_buttons = document.querySelectorAll("[id*='id_flow_']"); | ||
flow_radio_buttons.forEach(input => { | ||
input.addEventListener("change", (event) => { | ||
let flow_id = event.currentTarget.value; | ||
showCheckbox(flow_id); | ||
hideOtherCheckboxes(flow_id); | ||
}); | ||
|
||
if (input.checked) { | ||
showCheckbox(input.value); | ||
} | ||
}); | ||
}); | ||
</script> | ||
{% endblock extra-scripts %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thekaveman - to follow up on #2698 (comment):
I rewrote this so that it is non-defensive and simply looks up the key. Consumers of this property are responsible for knowing what to do, whether that's to let the exception bubble up or to handle it.
Here's the diff: 21e0f69
That commit shows how
InPersonEligibilityForm
consumes the property.EnrollmentFlow.clean
consumes it in a similar way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see... yeah I don't think this is the specific use-case I was thinking about, and I didn't understand the difference in what we were talking about. I agree this change isn't as clean.
The use-case I was thinking about (related to #2698 (comment)): Why was it possible to load a fixture with a bad
system_name
? I expected this to have failed on import: #2698 (comment)But I think you described why this is: #2698 (comment). And then it was fixed anyway by adding
agency_card
to the enum...Sorry, I mixed up a few different things in that comment. But really I was wondering: why should we even be able to import bad data like that? And does that mean it is somehow possible to get a bad value in there from the Admin? I think both of those are probably answered by your explanation.