Skip to content
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

Chore: Clean up localfixtures, env, Cypress tests to only have CST #2214

Merged
merged 8 commits into from
Jul 15, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Jul 11, 2024

closes #2210
closes #2211

This work is related to #2209 #2205 and other tickets a part of removing specific real agencies from non-production apps.

What this PR does

  • Remove MST, SacRT, SBMTD objects from fixtures file and .env.sample file
  • Replace MST object with CST, rename Courtesy Card to Agency Card

Testing

Post-merge tasks

  • env.sample, env can be simplified

@machikoyasuda machikoyasuda requested a review from a team as a code owner July 11, 2024 16:03
@github-actions github-actions bot added migrations [auto] Review for potential model changes/needed data migrations updates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Jul 11, 2024
Copy link

github-actions bot commented Jul 11, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

@machikoyasuda machikoyasuda self-assigned this Jul 11, 2024
@machikoyasuda
Copy link
Member Author

@angela-tran Are you able to go through the entire Agency Card flow locally with this fixture file? I'm not currently but it might be because my Elig Server image is messed up.

@machikoyasuda machikoyasuda marked this pull request as draft July 11, 2024 16:20
@angela-tran
Copy link
Member

angela-tran commented Jul 11, 2024

@angela-tran Are you able to go through the entire Agency Card flow locally with this fixture file? I'm not currently but it might be because my Elig Server image is messed up.

I can get through some of it. Here's where I'm at:

  • ./bin/reset_db.sh

Tried to verify Agency Card eligibility, saw an error about NoneType with getting a secret.

  • add a value for agency_card_verifier_api_auth_key in my .env file, similar to courtesy_card_verifier_api_auth_key

Tried to verify Agency Card eligibility again, was able to see and submit the form, but got the "Your card information may not have been entered correctly." screen, and saw this in server logs:

[2024-07-11 16:49:44,352] DEBUG eligibility_server.verify:33 Verify initialized without hashing
2024-07-11 11:49:44 [2024-07-11 16:49:44,447] DEBUG eligibility_server.verify:123 User's types do not contain any of the requested types: ['agency_card']

Options to fix:

  • add agency_card to the data file we're using
  • create separate data file in .devcontainer/server for Benefits to use instead of looking at eligibility-server's sample data file, and add agency_card to it

@machikoyasuda
Copy link
Member Author

Yes! I was just about to comment that we'll have to also make a ticket to clean up MST/SBMTD-specific data from Elig Server test files too.

@machikoyasuda
Copy link
Member Author

@machikoyasuda machikoyasuda force-pushed the fix/2210-localfixtures-cst branch 2 times, most recently from e038bd6 to abe0eb5 Compare July 11, 2024 22:29
@machikoyasuda machikoyasuda changed the title Chore: Clean up localfixtures file to be only CST Chore: Clean up localfixtures, env, Cypress tests to only have CST Jul 11, 2024
@machikoyasuda machikoyasuda marked this pull request as ready for review July 11, 2024 23:13
@machikoyasuda
Copy link
Member Author

machikoyasuda commented Jul 11, 2024

@angela-tran @thekaveman @lalver1

I decided to do both #2210 #2211 in one PR, because the latter requires the former for it to work, and the former requires the latter for it to pass GitHub tests. I believe once the changes to the server.csv file and associated specs here cal-itp/eligibility-server#484 are merged and deployed, then this PR should then pass.

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Looking really good so far. One small request.

tests/pytest/eligibility/test_forms.py Outdated Show resolved Hide resolved
@machikoyasuda
Copy link
Member Author

@thekaveman Ready for re-review. Tests are now passing!

"pk": 7,
"fields": {
"name": "(MST) CalFresh oauth claims via Login.gov",
"name": "(CST) CalFresh oauth claims via Login.gov",
Copy link
Member

Choose a reason for hiding this comment

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

(Not for this PR): eventually we should have a new core.authprovider instance for this verifier:

{
  "model": "core.authprovider",
  "pk": 3,
  "fields": {
    "sign_out_button_template": "core/includes/button--sign-out--login-gov.html",
    "sign_out_link_template": "core/includes/link--sign-out--login-gov.html",
    "client_name": "calfresh-benefits-oauth-client-name",
    "client_id_secret_name": "auth-provider-client-id",
    "authority": "https://example.com",
    "scope": "verify:calfresh",
    "claim": "calfresh",
    "scheme": "dev-cal-itp_benefits"
  }
},

Then update the field here on the eligibilityverifier:

- "auth_provider": 1
+ "auth_provider": 3

@machikoyasuda machikoyasuda merged commit 6883589 into main Jul 15, 2024
15 checks passed
@machikoyasuda machikoyasuda deleted the fix/2210-localfixtures-cst branch July 15, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
3 participants