-
Notifications
You must be signed in to change notification settings - Fork 35
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
B 19894 too queue moves int #13005
B 19894 too queue moves int #13005
Conversation
…show up for New Move filter choice
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger
Smaller No assets were smaller Unchanged
|
Flaky test failures, functionality can still be reviewed |
} | ||
} | ||
query.Where("moves.status in (?)", translatedStatuses) | ||
query.Where("moves.status IN (?)", statuses) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is so much cleaner than the way it was done before, nice job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit picky. Sorryyyyyy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, approved but I also like Daniels suggested changes 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran through and tried to break it - seems all in working order. LGTM!
Important
The large number of commits here are a result of bringing in the latest main.
See main branch for commits to pay attention to as part of 19894 work: main...B-19894-TOO-queue-moves-MAIN
Agility ticket
Summary
This work is to show only moves with the following statuses in the TOO queue:
New Move
Service Counseling Completed
Approvals Requested
Previously, the TOO queue had filter status dropdowns for:
New Move
Approvals Requested
Approved
To test:
New Move
Service Counseling Completed
Approvals Requested
Fix:
Before making any changes, I discovered that the New Move drop down status filter actually showed both New Move moves and Service Counseling Completed moves which was funky. I removed this interesting filtering in
pkg/services/order/order_fetcher.go
andpkg/services/move/move_searcher.go
.Changes:
I removed the status enum values from being defined in
swagger-def/ghc.yaml
. I found this to be unnecessary as we are now handling the status filter inpkg/handlers/ghcapi/queues.go
, which is also where we are handling the status filter for the SC and TIO roles as well.Had to make the tests happy as well - lots of tests use "Approved" as the status for TOO actions, and that status no longer appears in the TOO queue. A few NTS tests were fixed as well. The NTS tests in question used a "Submitted" status move initially in the test generation but then moved it to "Approved" so I modified those tests to search for the move as a TOO rather than expect it in the TOO queue.