Skip to content

Refactor: add enrollment_littlepay token view #2934

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

Merged
merged 4 commits into from
May 27, 2025

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented May 19, 2025

Part of #2907

This PR moves the entire token view from benefits.enrollment.views into benefits.enrollment_littlepay.views. It also refactors the view into a class-based view.

The URL for the view was moved under enrollment_littlepay with a namespace of littlepay so that the URL is something like <base URL>/littlepay/token. The route was updated as well so that all consumers of the route use the new URL.

Reviewing

Enrollment with Littlepay should still work.

Screenshot

image

@angela-tran angela-tran self-assigned this May 19, 2025
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev tests Related to automated testing (unit, UI, integration, etc.) back-end Django views, sessions, middleware, models, migrations etc. and removed front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels May 19, 2025
Copy link

github-actions bot commented May 19, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits
  routes.py
  benefits/enrollment
  views.py
  benefits/enrollment_littlepay
  urls.py
  views.py
Project Total  

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

@angela-tran
Copy link
Member Author

angela-tran commented May 19, 2025

This PR will need to coordinate with the one for the index view. I can wait to rebase on top of that one

I'm gonna go ahead and mark this as ready to review. I can help with resolving conflicts or rebasing the refactor/enrollment-index-view branch (from #2930) on top of these changes if/when they're merged into main.

@angela-tran angela-tran marked this pull request as ready for review May 20, 2025 20:14
@angela-tran angela-tran requested a review from a team as a code owner May 20, 2025 20:14
@thekaveman thekaveman marked this pull request as draft May 22, 2025 15:22
@thekaveman
Copy link
Member

Putting this back in Draft while (someone) resolves the conflicts.

@thekaveman thekaveman force-pushed the refactor/enrollment_littlepay-token-view branch from 6f8dc9c to 0ab6925 Compare May 23, 2025 17:32
@thekaveman thekaveman marked this pull request as ready for review May 23, 2025 17:33
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.

OK I rebased on the latest main (I hope I didn't break anything 😅)

  • Tests are all passing ✅
  • Was able to run through a full enrollment locally ✅

I did make one tiny change during the rebase: it hasn't been our pattern to use unittest.mock directly in tests, we typically use the mocker fixture provided by pytest-mock. I adjusted one of the tests to follow our typical pattern for consistency.

@pytest.mark.django_db
@pytest.mark.usefixtures("mocked_session_agency", "mocked_session_eligible")
def test_token_valid(mocker, client):
mocker.patch.object(Session, "access_token", "enrollment_token")
Copy link
Member

Choose a reason for hiding this comment

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

This was the adjustment:

Previously this test used the @patch() decorator from unittest.mock to override the Session.access_token attribute with a PropertyMock (also from unittest.mock).

The small change was to use mocker.patch.object on Session to achieve the same result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! Thanks for the adjustment

Copy link
Member

@lalver1 lalver1 left a comment

Choose a reason for hiding this comment

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

Looks good! I also made sure that I could still enroll with Littlepay and everything worked.

@angela-tran angela-tran merged commit c969a34 into main May 27, 2025
14 checks passed
@angela-tran angela-tran deleted the refactor/enrollment_littlepay-token-view branch May 27, 2025 20:47
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. front-end HTML/CSS/JavaScript and Django templates tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants