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

Feat: command for group migration #49

Merged
merged 4 commits into from
Apr 9, 2024
Merged

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Apr 4, 2024

Closes #47

Built off #48

This PR adds a subcommand to the groups command called migrate. It calls the client.migrate_concession_group method that was implemented in #48 for all groups that are returned by any filters given. (If no filters are given, all groups are returned, so all groups would be migrated.)

See my comment below on an error I'm seeing when I tried to use migrate in the QA environment.

@angela-tran angela-tran self-assigned this Apr 4, 2024
Copy link

github-actions bot commented Apr 4, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  littlepay
  main.py
  littlepay/commands
  groups.py
Project Total  

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

@angela-tran
Copy link
Member Author

angela-tran commented Apr 4, 2024

I tried running migrate and got back a 403. I'm able to run other commands (e.g. littlepay groups -f Courtesy) just fine, so I don't think it's an auth token problem.

Here's the output, with group ID redacted:

calitp@bd7236ab9397:~/app$ littlepay groups -f Courtesy migrate
Migrating group: qa, mst [<group id>]
❌ Error: 403 Client Error: Forbidden for url: <url>/api/v1/concession_groups/<group id>/migrate
👥 Matching groups (0): qa, mst

@angela-tran angela-tran marked this pull request as ready for review April 4, 2024 20:18
@angela-tran angela-tran requested a review from a team as a code owner April 4, 2024 20:18
@thekaveman thekaveman marked this pull request as draft April 4, 2024 21:12
@thekaveman thekaveman marked this pull request as ready for review April 4, 2024 21:12
@thekaveman
Copy link
Member

Sorry didn't mean to mark this one as Draft!

littlepay/main.py Outdated Show resolved Hide resolved
tests/commands/test_groups.py Outdated Show resolved Hide resolved
@angela-tran angela-tran marked this pull request as draft April 5, 2024 17:40
@angela-tran
Copy link
Member Author

😲 The command worked today

calitp@bd7236ab9397:~/app$ littlepay groups -f Courtesy migrate
Migrating group: qa, mst [<group id redacted>]
❔ Are you sure? (yes/no): y
Migrating group...
✅ Migrated
👥 Matching groups (0): qa, mst

@angela-tran angela-tran marked this pull request as ready for review April 5, 2024 18:34
@thekaveman
Copy link
Member

thekaveman commented Apr 5, 2024

@angela-tran Great news! Do you know why it's printing the last line?

👥 Matching groups (0): qa, mst

@angela-tran
Copy link
Member Author

@angela-tran Great news! Do you know why it's printing the last line?

It's from this line in the main logic for the groups command. I think that line is more for CSV output rather than being relevant for any of the subcommands.

@angela-tran
Copy link
Member Author

Hmm, that line was there even before the CSV output additions, so I'm not sure what its intended purpose was

@angela-tran
Copy link
Member Author

It was introduced when there were no subcommands, so maybe it just needs to be wrapped in a conditional?

@thekaveman
Copy link
Member

It was introduced when there were no subcommands, so maybe it just needs to be wrapped in a conditional?

Yeah would you mind? It looks a little strange coming after the migrate messages.

the command operates on a potentially large number of groups and
modifies configuration, so this adds a layer of protection against
mistakes.

note that this commit doesn't change the implementation
@thekaveman thekaveman added the cli New command line interface feature implementation or refactor label Apr 9, 2024
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.

Nice 👍

@angela-tran angela-tran merged commit 6a9e5e7 into main Apr 9, 2024
3 checks passed
@angela-tran angela-tran deleted the feat/migrate-groups-command branch April 9, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli New command line interface feature implementation or refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command to migrate a concession group
2 participants