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

Refactor: enrollment with Backoffice API #1905

Merged
merged 12 commits into from
Mar 8, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Feb 23, 2024

This PR refactors the enrollment phase to use the cal-itp/littlepay library rather than having its own implementation against an older API.

From the user's perspective, everything should work exactly the same.

For testing this, you'll need to configure the new fields on PaymentProcessor: client_id, audience, and client_secret_name. The values for the first two can be found in LastPass, and for the secret, you'll need to set the name of the secret and then set the actual value in your .env file.

Earlier notes

Pushing this up for visibility, but please be aware that this branch will definitely be rebased since several things are WIP:

  • need to use SecretNameField for client_secret field
  • need to figure out how to properly assert that oauth.ensure_active_token is being called - having a hard time figuring out how to set up the mocked Client to work how I'd expect
  • need to delete old enrollment.api module and tests
  • need to update view tests for new implementation of enrollment
  • need to add back any valid test cases from deleted enrollment.api tests to be covered by view tests
  • need to generate migration for changes to PaymentProcessor model

@angela-tran angela-tran self-assigned this Feb 23, 2024
@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. labels Feb 23, 2024
@angela-tran angela-tran linked an issue Feb 23, 2024 that may be closed by this pull request
2 tasks
@github-actions github-actions bot added the migrations [auto] Review for potential model changes/needed data migrations updates label Feb 23, 2024
Copy link

github-actions bot commented Feb 28, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/core
  models.py
  benefits/enrollment
  views.py
Project Total  

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

@angela-tran angela-tran force-pushed the refactor/enrollment-backoffice branch 3 times, most recently from 31e6ef1 to c47e390 Compare February 28, 2024 23:19
@angela-tran angela-tran marked this pull request as ready for review February 28, 2024 23:22
@angela-tran angela-tran requested a review from a team as a code owner February 28, 2024 23:22
@angela-tran
Copy link
Member Author

@thekaveman This is ready for review. I rebased and cleaned up the commits as much as I could. I think technically I could combine the two migration files into one, but that would require combining some changes that are currently in separate commits into one commit that might be a bit monstrous.

@thekaveman
Copy link
Member

I think technically I could combine the two migration files into one, but that would require combining some changes that are currently in separate commits into one commit that might be a bit monstrous.

I think how you have done it as a separate migration makes sense! We're now in the world of needing to migrate model changes, so I like it 👍

@thekaveman
Copy link
Member

@angela-tran this is so exciting! Thank you for your hard work on this refactor!! 👏

I think we should wait to merge this so that it is not going out to prod at the same time as all our secrets / Admin changes scheduled for Friday 3/1 (that seems like a lot to release all at once / harder to diagnose any issues that come up).

Let's plan for another release in the first week or two of March that includes this refactor.

@angela-tran angela-tran force-pushed the refactor/enrollment-backoffice branch 2 times, most recently from 41b0783 to 304f645 Compare March 6, 2024 15:06
@angela-tran
Copy link
Member Author

I rebased one more time so that I could rename client_secret to client_secret_name 😄

@thekaveman
Copy link
Member

Looking at this now

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.

Overall this is looking really good 👍

I think the majority of the view changes and their tests are great.

Just a few more changes needed in the model / construction of the littlepay.api.Client and some minor cleanup of migrations and fixtures.

benefits/core/migrations/local_fixtures.json Show resolved Hide resolved
benefits/core/models.py Show resolved Hide resolved
benefits/enrollment/views.py Outdated Show resolved Hide resolved
benefits/enrollment/views.py Outdated Show resolved Hide resolved
updates unit test for view function. some fields were needed
on PaymentProcessor to represent Backoffice API config
change test so that getting API token is entirely mocked - I was not
able to figure out how to mock the oauth object on Client
add handling for known case that should be treated as a success
the enrollment view tests have not been updated yet so there are some
expected failures.

they also need to be updated to add back coverage of cases that the
deleted tests used to cover, e.g. HTTP errors from API requests.

some cases from the old tests, e.g. invalid responses, do not apply with
the new code.
@angela-tran angela-tran force-pushed the refactor/enrollment-backoffice branch from 304f645 to ed26642 Compare March 7, 2024 21:29
@angela-tran angela-tran marked this pull request as draft March 7, 2024 21:34
@angela-tran
Copy link
Member Author

I was able to make the requested changes and also tested the enrollment phase with those new code changes. Thanks for all the feedback @thekaveman. Let me know if you see anything else

image

@angela-tran angela-tran marked this pull request as ready for review March 7, 2024 21:53
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.

Sorry, one more requested change as I'm trying to get this running locally...

Can you please:

  • remove existing sample payment processor secrets in .env.sample for the old API e.g. mst_payment_processor_client_cert
  • add new sample payment processor secrets in .env.sample for the new API e.g. mst_payment_processor_client_secret
  • update the sample fixture to reference these new sample secrets

@angela-tran
Copy link
Member Author

Sorry, one more requested change as I'm trying to get this running locally...

For sure. I forgot about that .env.sample file. Done in 3bfb9e8

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.

Fantastic!! 🎉

I got this to work locally and tested the following flows:

  • ☑️ New enrollment in a group -- received the expected success response
  • ☑️ Duplicate enrollment in a group -- received the expected 409 response, got the success page

We'll handle error flows in #1936

@thekaveman
Copy link
Member

thekaveman commented Mar 8, 2024

Post-merge in dev

  • Add the new client_secret secrets in KeyVault
  • Update the PaymentProcessor instances in the Admin with new secret names and non-secret config
  • Delete the old cert-based secrets in KeyVault
  • Delete the old PemData instances in the Admin

We'll wait to delete the old secrets and data in test and prod until after we've deployed to those environments 😅

See #1937

@thekaveman
Copy link
Member

IMHO the best part of this PR 😁 😁 👍

image

@angela-tran
Copy link
Member Author

Awesome! I'm working on merging / post-merge actions now

@angela-tran angela-tran merged commit 88be9f3 into dev Mar 8, 2024
11 checks passed
@angela-tran angela-tran deleted the refactor/enrollment-backoffice branch March 8, 2024 20:37
@angela-tran
Copy link
Member Author

I finished the post-merge steps for dev. I would like to leave configuring the test and prod secrets for when we are doing the deployments for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Enrollment to use Back Office API
2 participants