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.

Sign up for GitHub

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

Jump to bottom

Refactor: PaymentProcessor to TransitProcessor #2262

Merged
merged 8 commits into from
Aug 2, 2024
Merged

Refactor: PaymentProcessor to TransitProcessor #2262

merged 8 commits into from
Aug 2, 2024

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Aug 1, 2024 •
edited
Loading

Closes #2235

The PR contains only the basic renames/refactors and no other changes to logic, tests, etc.

local_fixtures.json is also updated, you should be able to checkout and run bin/reset_db.sh and/or bin/init.sh to recreate the local database with these changes.

Things to check locally

  1. Checkout the main branch
  2. Edit your local_fixtures.json (or whatever fixture you are using) to add values to existing PaymentProcessor fields updated by this PR (client_id, client_secret_name, audience)
  3. Run bin/reset_db.sh (now your local DB mirrors the structure of dev)
  4. Checkout this PR's branch
  5. Run bin/init.sh (running this PR's migration on top of existing DB structure)
  • For fields that were moved to TransitAgency, confirm data is migrated
  • Confirm the Can X payment processor permissions are removed from the database
  • Confirm the Can X transit processor permissions are added to the database
  • Confirm the Cal-ITP group has Can [view|change] transit processor permissions

@thekaveman thekaveman self-assigned this Aug 1, 2024
@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Aug 1, 2024
@thekaveman thekaveman added this to the Admin tool: agency users milestone Aug 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 1, 2024 •
edited
Loading

Coverage report

Click to see where and how coverage changed

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

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

@thekaveman thekaveman changed the title Refactor: PaymentProcessor becomes TransitProcessor Refactor: PaymentProcessor to TransitProcessor Aug 1, 2024
@thekaveman thekaveman changed the title Refactor: PaymentProcessor to TransitProcessor Refactor: PaymentProcessor to TransitProcessor Aug 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 1, 2024

Preview url: https://benefits-2262--cal-itp-previews.netlify.app

@thekaveman thekaveman force-pushed the refactor/models-transitprocessor branch 2 times, most recently from d595159 to 66bda46 Compare August 1, 2024 01:38
@thekaveman
Copy link
Member Author

@angela-tran I don't want to step on your toes with #2256 since we both have migrations...

I think this PR is ready to open for review, but I can rebase it onto yours and deal with the migrations conflicts here if you'd rather.

@thekaveman thekaveman force-pushed the refactor/models-transitprocessor branch from 66bda46 to bff74dc Compare August 1, 2024 15:51
@angela-tran
Copy link
Member

@angela-tran I don't want to step on your toes with #2256 since we both have migrations...

I think this PR is ready to open for review, but I can rebase it onto yours and deal with the migrations conflicts here if you'd rather.

Thanks for checking! I'm still working on my PR, so I'm ok if your PR goes in first and I rebase onto main and resolve merge conflicts. I might also look over my migrations when I'm done making changes and see if I can combine them into one migration anyways.

In general, I think it's up to each PR to make sure it's working with the latest migrations in main.

@thekaveman thekaveman marked this pull request as ready for review August 1, 2024 16:11
@thekaveman thekaveman requested a review from a team as a code owner August 1, 2024 16:11
@thekaveman thekaveman marked this pull request as draft August 1, 2024 16:14
@thekaveman thekaveman marked this pull request as ready for review August 1, 2024 16:20
@angela-tran
Copy link
Member

Things to check locally

  • For fields that were moved to TransitAgency, confirm data is migrated
  • Confirm the Can X payment processor permissions are removed from the database
  • Confirm the Can X transit processor permissions are added to the database

  • Confirm the Cal-ITP group has Can [add|change] transit processor permissions

@thekaveman Did you mean Can [view|change] transit processor?

  • Confirm the Cal-ITP group has Can [view|change] transit processor permissions

angela-tran
angela-tran requested changes Aug 1, 2024
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Code changes look good 👍

Noticed a small typo

benefits/core/migrations/0016_refactor_paymentprocessor_transitprocessor.py Outdated Show resolved Hide resolved
benefits/core/models.py Outdated Show resolved Hide resolved
@thekaveman thekaveman force-pushed the refactor/models-transitprocessor branch from fb35314 to 8f0f2a7 Compare August 2, 2024 00:41
@lalver1
Copy link
Member

lalver1 commented Aug 2, 2024

Looks good to me too!

  • For fields that were moved to TransitAgency, confirm data is migrated
  • Confirm the Can X payment processor permissions are removed from the database
  • Confirm the Can X transit processor permissions are added to the database
  • Confirm the Cal-ITP group has Can [view|change] transit processor permissions

@thekaveman thekaveman force-pushed the refactor/models-transitprocessor branch from 8f0f2a7 to 3941634 Compare August 2, 2024 16:08
@thekaveman thekaveman merged commit 58abd80 into main Aug 2, 2024
17 checks passed
@thekaveman thekaveman deleted the refactor/models-transitprocessor branch August 2, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angela-tran angela-tran angela-tran approved these changes

Assignees

@thekaveman thekaveman

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
Milestone
Admin tool: agency users
Development

Successfully merging this pull request may close these issues.

Model refactor: PaymentProcessor
3 participants
@thekaveman @angela-tran @lalver1