Refactor: merge fields from EligibilityApiConfig into EligibilityApiVerificationRequest by lalver1 · Pull Request #3441 · cal-itp/benefits

lalver1 · 2026-01-20T21:04:27Z

This PR moves fields that are not specific to a transit agency to EligibilityApiVerificationRequest and deletes EligibilityApiConfig since it is no longer needed.

Follow-ups

Update Littlepay and Switchio local development fixtures in LastPass

Reviewing

Working from main, set your dev environment to use either the Littlepay or Switchio dev fixtures
Reset your database (bin/reset_db.sh) to simulate a running Benefits instance
Switch to the branch of this PR
Run bin/init.sh to simulate what a running Benefits instance will do when the code in this PR is deployed
Confirm that in the new database, api_private_key and api_public_key were moved to EligibilityApiVerificationRequest and that they have the correct data, i.e., api_client_private_key points to the Benefits client private key PemData instance and api_client_public_key points to the Benefits client public key PemData instance
Also confirm that the eligibility_api_config field was removed from TransitAgency and that the EligibilityApiConfig instances were deleted from the database
Successfully run through a full CST Agency Cardholder flow (digital and in-person)
In the admin, ensure that the EligibilityApiConfig model was removed and that the fields in steps 5 and 6 show up in the correct model
Go to the http://localhost/cst/publickey URL and ensure that you can still see Benefits' public key

To review the dev container

Switch to the branch of this PR
Set your environment to use the local_fixtures.json fixtures
Reset your database (bin/reset_db.sh) and ensure the database is successfully recreated
Repeat steps 5-8 from above (without enrolling a card since local_fixtures.json are not set up for enrollment)

github-actions · 2026-01-20T21:05:42Z

Coverage report

Click to see where and how coverage changed

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

_{This report was generated by python-coverage-comment-action}

lalver1 · 2026-01-20T21:51:06Z

    api_auth_key_secret_name = SecretNameField(
        help_text="The name of a secret containing the value of the auth header to send in Eligibility API requests.",
    )
+    api_client_private_key = models.ForeignKey(


Regarding the naming, I tried to keep the 3 key-related fields sort of consistent.

Take it or leave it, but these two fields could be simplified to just client_*_key (e.g. dropping the api_ prefix).

Might help differentiate from the fields that are more about the server (API) config vs. Benefits (client) config.

Ah thanks for this suggestion! I like this simplification and I made the change in 4504964 instead of going back to the original commit to hopefully make reviewing this change easier.

lalver1 · 2026-01-20T22:07:45Z

-        agency = kwargs.get("agency")
-        return HttpResponse(agency.eligibility_api_public_key_data, content_type="text/plain")
+        eligibility_api_public_key_data = EligibilityApiVerificationRequest.objects.first().api_client_public_key_data
+        return HttpResponse(eligibility_api_public_key_data, content_type="text/plain")


It's interesting how the change in this PR affects this view/URL.

First, as seen from this code, eligibility_api_public_key_data is not agency-related, so the URL <agency:agency>/publickey does not really need the agency argument anymore. I didn't remove it (yet) because I wanted to double check if there are any other systems (like eligibility server GH Actions, for example) that would still expect to include the agency argument. If there are none, the URL can be simplified.

The second thing, I think that this indirectly also shows that the api_client_public_key (and api_client_private_key) fields can even be removed from EligibilityApiVerificationRequest because they don't depend on the EV server instance Benefits communicates with, these 2 fields are used by the whole Benefits application, so maybe we can just use the PemData model directly? If it makes sense, it could be a followup refactor.

These are both great points and I think we can simplify this significantly. I'd argue a follow-up issue/PR probably makes the most sense but open to doing it here as well.

We can see in the config files on the MST and SBMTD EV servers this is a simple path:

# In MST's case CLIENT_KEY_PATH = "https://benefits.calitp.org/mst/publickey" # In SBMTD's case CLIENT_KEY_PATH = "https://benefits.calitp.org/sbmtd/publickey"

And going to a simpler top-level approach seems pretty easy to do, and also reduces code

CLIENT_KEY_PATH = "https://benefits.calitp.org/publickey"

Since we don't have an "app configuration" model (something we have kicked around in the past, but not totally necessary at least for this) we'll just want to be careful with how the query for PemData works, to ensure we always get exactly just the public key version.

If this feels more related to the merging of those 2 models into EligibilityApiVerificationRequest it definitely feels in scope to me to do it as part of this PR. FWIW.

Thanks for talking through these points with me @thekaveman. We agreed that addressing the two observations from the comment above (#3441 (comment)) wouldn't do much good at this point, we can keep the organization as-is.

On observation 1, it's not wrong to keep the agency argument in the URL. Conceptually this is still correct, it's just that the client public key is shared among all agencies, so it's ok to request a key to a particular agency, even if the view associated with the URL does not end up using the agency argument. A small clarification was added as an in-line comment on this point.

On observation 2, an "app configuration" model is not necessary at this point since it would only hold 2 fields, and it seems to add more complication and not much benefit. Without this extra model, we couldn't identify a way to select the correct PemData instance corresponding to the encryption key we need to fetch. Because of this, we should keep the client API encryption keys in EligibilityApiVerificationRequest along with the EV server's public key. They are neatly organized under the same model.

thekaveman · 2026-01-20T23:14:41Z

I note the autofix run from pre-commit.ci: 2bfd2c3

I wouldn't have expected that, was your devcontainer fully rebuilt on the latest main, that includes the changes to add isort? Or is there more in-fighting between black and isort and pre-commit and the devcontainer?

lalver1 · 2026-01-22T00:26:00Z

I note the autofix run from pre-commit.ci: 2bfd2c3

I wouldn't have expected that, was your devcontainer fully rebuilt on the latest main, that includes the changes to add isort? Or is there more in-fighting between black and isort and pre-commit and the devcontainer?

Thanks for pointing this out! For some reason my dev container is not applying isort when saving a file, so I applied it manually and committed the changes as the last commit and this worked. I'll see what's going on with my dev container, but for this PR it looks like there are no more autofixes 🎉. This should be ready for a re-review 🙏 .

thekaveman

Some comments, looking good overall.

thekaveman · 2026-01-22T01:27:25Z

    api_auth_key_secret_name = SecretNameField(
        help_text="The name of a secret containing the value of the auth header to send in Eligibility API requests.",
    )
+    api_client_private_key = models.ForeignKey(


Take it or leave it, but these two fields could be simplified to just client_*_key (e.g. dropping the api_ prefix).

Might help differentiate from the fields that are more about the server (API) config vs. Benefits (client) config.

move api_private_key and api_public_key fields from EligibilityApiConfig to EligibilityApiVerificationRequest

move api_private_key and api_public_key data getters from TransitAgency to EligibilityApiVerificationRequest. Also update usage of the moved fields.

use TransitAgency.slug instead

remove the field since it is not used anymore

since the DeleteModel step already deletes the table from the database, this step is redundant.

drop the "api_" prefix from EligibilityApiVerificationRequest fields to help differentiate fields that are more about the server (API) config vs. Benefits (client) config.

since EligibilityApiConfig will be deleted, these steps are redundant and can be removed to clean up the migration.

thekaveman

Nice work @lalver1! I went through all the steps and everything worked as expected (except for In-Person, since I couldn't configure agency_card for that and didn't want to mess around with why).

lalver1 self-assigned this Jan 20, 2026

lalver1 force-pushed the refactor/merge-fields-eligibility branch from c68c4f1 to 9a273fb Compare January 20, 2026 21:14

lalver1 commented Jan 20, 2026

View reviewed changes

lalver1 marked this pull request as ready for review January 20, 2026 22:08

lalver1 requested a review from a team as a code owner January 20, 2026 22:08

lalver1 force-pushed the refactor/merge-fields-eligibility branch from 2bfd2c3 to f19735e Compare January 22, 2026 00:17

thekaveman reviewed Jan 22, 2026

View reviewed changes

thekaveman reviewed Jan 26, 2026

View reviewed changes

Comment thread benefits/core/migrations/0072_move_fields_from_eligapiconfig_to_eligapiverificationrequest.py Outdated

lalver1 added 10 commits January 27, 2026 17:35

refactor(models): move EV server encryption key fields

05b6bde

move api_private_key and api_public_key fields from EligibilityApiConfig to EligibilityApiVerificationRequest

refactor(models): move EV server encryption key data getters

8c52561

move api_private_key and api_public_key data getters from TransitAgency to EligibilityApiVerificationRequest. Also update usage of the moved fields.

refactor(models): remove api_id field from EligibilityApiConfig

245b617

use TransitAgency.slug instead

refactor(models): remove eligibility_api_config from TransitAgency

4c3809d

remove the field since it is not used anymore

refactor(models): remove EligibilityApiConfig models/instances

4804317

chore: clean up fixtures and rename migration

e30b073

chore(isort): apply isort formatting

974a217

chore(models): remove unnecessary deletion of instances

db33935

since the DeleteModel step already deletes the table from the database, this step is redundant.

chore(models): simplify field names for clarity

69746f7

drop the "api_" prefix from EligibilityApiVerificationRequest fields to help differentiate fields that are more about the server (API) config vs. Benefits (client) config.

chore(models): remove unnecessary deletion of fields

d832b65

since EligibilityApiConfig will be deleted, these steps are redundant and can be removed to clean up the migration.

lalver1 force-pushed the refactor/merge-fields-eligibility branch from 4504964 to d832b65 Compare January 27, 2026 17:58

thekaveman approved these changes Jan 27, 2026

View reviewed changes

lalver1 merged commit d8c6f32 into main Jan 27, 2026
14 checks passed

lalver1 deleted the refactor/merge-fields-eligibility branch January 27, 2026 22:22

Uh oh!

Conversation

lalver1 commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Follow-ups

Reviewing

Uh oh!

github-actions Bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

lalver1 Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

thekaveman Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

lalver1 Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

lalver1 Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

thekaveman Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thekaveman Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

lalver1 Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

thekaveman commented Jan 20, 2026

Uh oh!

lalver1 commented Jan 22, 2026

Uh oh!

thekaveman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thekaveman Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thekaveman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lalver1 commented Jan 20, 2026 •

edited

Loading

github-actions Bot commented Jan 20, 2026 •

edited

Loading

thekaveman Jan 20, 2026 •

edited

Loading