-
Notifications
You must be signed in to change notification settings - Fork 9
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
[Debt] Refactors tests to remove Pool Operator, Request Responder, and Community Manager roles #12530
Conversation
@@ -1016,6 +989,7 @@ public function testUpdatePoolCandidateClaimVerificationValidation(): void | |||
*/ | |||
public function testManualStatusUpdatesTimestamps($status, $timestamp) | |||
{ | |||
$this->markTestSkipped('Only a pool operator can update applicationStatus.'); |
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.
I marked this test as skipped because pool operator is being removed. Should the test be removed altogether?
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.
currently ProcessOperators are updating candidate status upto Qualified or Disqualified.
But they can't place the candidates.
Is it possible to update the test to verify that? If this has been handled already leave it. If not, I would suggest verifying
- verify ProcessOperator can update status manually to ScreenedIn
- verify ProcessOperator can't update status manually to any of the placed 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.
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.
So @brindasasi is correct that ProcessOperators can now use the manual update, for certain statuses. So can Community Admins and Community Recruiters. This is new functionality, since #12494.
I don't think we need a second test for placing. #12494 already added a test, PoolCandidateUpdateTest.testManualStatusUpdatePolicy
, to ensure Qualifying and Screening are treated differently. The test here is meant to test the timestamp update, not test permissions.
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.
@mnigh if we have the policy test already we don't need that testManualStatusPlacedUpdatesTimestamps
test. Could you remove that ? With that it is good to be merged in.
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.
@brindasasi an update:
@tristan-orourke had another look and requested to: replace processOperatorUser
with communityRecruiterUser
; Re-add Placed 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.
I took a look and it seems fine. I think @tristan-orourke (or someone else who is confident) should take a look at the comments you made about skipped tests before any approval.
…alStatusPlacedUpdatesTimestamps
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.
LGTM!
🤖 Resolves #12300.
👋 Introduction
This PR refactors tests to remove Pool Operator, Request Responder, and Community Manager roles.
🧪 Testing