Refactor: choices for EnrollmentFlow.system_name

[angela-tran]

Needed by #2689 for #2632

This PR introduces a SystemName choices enum to be used as the possible values for EnrollmentFlow.system_name. This will be useful for defining flow-specific context dicts .

It's helpful to split this out from #2689 so that I can fix unit tests that are broken by the introduction of choices to EnrollmentFlow.system_name.

Benefits Admin user-facing change

The System name field on EnrollmentFlows now is a dropdown with these values:

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
benefits/core/context
__init__.py
flow.py
benefits/core/models
enrollment.py
Project Total

This report was generated by python-coverage-comment-action

[angela-tran]

Added a screenshot above of user-facing change. (Also, Kegan was correct that Django uses the enum name to generate the display value.)

[lalver1]

I don't think we need to do anything here, but in our local_fixtures we use agency_card as the system name for the Agency Cardholder EnrollmentFlow. This name isn't in the SystemName enum but this doesn't cause any trouble when you run bin/reset_db.sh, for example. The database still populates with agency_card as the system name even though the admin shows dashed lines:

I was thinking about when this gets deployed to prod but since there is no agency_card in prod this isn't an issue, it only affects the local dev environment.

[thekaveman]

@lalver1

I don't think we need to do anything here, in our local_fixtures we use agency_card as the system name

Wouldn't this cause an exception when that slug is looked up in the context dictionary?

[angela-tran]

... in our local_fixtures we use agency_card as the system name for the Agency Cardholder EnrollmentFlow.

Ahh, that's a good catch @lalver1

This name isn't in the SystemName enum but this doesn't cause any trouble when you run bin/reset_db.sh, for example. The database still populates with agency_card as the system name even though the admin shows dashed lines:

What you're pointing out here is that Django only enforces the choices when saving via a ModelForm (see django.forms.ChoiceField's validate). I think it would be best practice to create objects in the same way that ModelForms would, so that means our sample fixtures should abide by what the model's fields say. Same goes for the objects and fixtures we create in our pytests.

I suppose we could add logic to the model's clean method to mirror the validation that Django does with ModelForms, but not sure if that's actually worth our time and maintenance.

[angela-tran]

@thekaveman

Wouldn't this cause an exception when that slug is looked up in the context dictionary?

It would depend on how the look-up is written.

For example, in my WIP PR, the look-up of in_person_eligibility_context is defensive and will return an empty dictionary if the key was not found.

However, the look-up of agency_index assumes that every slug will have an entry, so that would throw an exception. I think that's desired because we would want to be alerted that we forgot to add copy for an agency index (I mean, hopefully we would've caught it earlier than that, but still).

Definitely open to a differing opinion on how this should work.

[angela-tran]

I'm going to add agency_card to our SystemName choice enum.

[thekaveman]

I think my initial take was, an exception should be thrown because we have a mismatch / invalid config in this case. E.g. a non-defensive lookup.

[lalver1]

Thanks @angela-tran and @thekaveman for elaborating on this.

I agree about adding agency_card to the SystemName enum, so that

... sample fixtures should abide by what the model's fields say

[angela-tran]

Ready for review again

[lalver1]

Looks good to me! 👍

[angela-tran]

I think my initial take was, an exception should be thrown because we have a mismatch / invalid config in this case. E.g. a non-defensive lookup.

Makes sense. I'll think about this more in #2689