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

[Debt] Remove applicant filters query #8975

Merged
merged 12 commits into from
Jan 11, 2024

Conversation

yonikid15
Copy link
Contributor

🤖 Resolves #7654

👋 Introduction

  • Removes applicantFilter query
  • Updates tests

🧪 Testing

Assist reviewers with steps they can take to test that the PR does what it says it does.

  1. Ensure site works correctly and all tests pass.

@@ -98,73 +100,6 @@ protected function filterToCreateInput(ApplicantFilter $filter)
return $input;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth repointing these tests at poolCandidateSearchRequest.applicantFilter instead of removing them? Or am I misunderstanding how this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the test testFilterCanBeStoredAndRetrievedWithoutChangingResults() was querying an applicant filter so I thought why not move the test there and save some space. However, I think your right looking back at it now since it's querying for all the fields which makes for a stronger test. I'll make the changes now 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow, that was a bit more work than just repointing the tests. 😬 Thanks for pushing through! 🥇

@yonikid15 yonikid15 changed the title 7654 remove applicant filters query [Debt] Remove applicant filters query Jan 10, 2024
Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

Looks great!

@yonikid15 yonikid15 added this pull request to the merge queue Jan 11, 2024
Merged via the queue into main with commit e1716b8 Jan 11, 2024
7 checks passed
@yonikid15 yonikid15 deleted the 7654-remove-applicantFilters-query branch January 11, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

♻️ Remove deprecated applicantFilters query
2 participants