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

[Bug] handle null state for SkillLevel on screening decision dialog #9773

Merged
merged 9 commits into from
Mar 20, 2024

Conversation

brindasasi
Copy link
Contributor

@brindasasi brindasasi commented Mar 18, 2024

πŸ€– Resolves #9757

πŸ‘‹ Introduction

This PR omits Not Found key word when there is no required level for the skill on the screening decision dialog title and inside the description section

πŸ•΅οΈ Details

Add any additional details that could assist with reviewing or testing this PR.

πŸ§ͺ Testing

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

  1. Open any existing process without required level on the skills ( make sure candidates exist)
  2. Go to screening and assessment tab
  3. Click on To assess for any candidate to open up the screening decision dialog
  4. Verify Not Found doesn't appear in the dialog

Regression Test

  1. Open any existing process WITH required level on the skills ( make sure candidates exist)
  2. Go to screening and assessment tab
  3. Click on To assess for any candidate to open up the screening decision dialog
  4. Verify Skill Levels are appearing properly for the Title and the description of the dialog.

πŸ“Έ Screenshot

When there is no required level
Screenshot 2024-03-18 at 10 22 41 AM
When there is required level
Screenshot 2024-03-18 at 2 01 25 PM

mnigh
mnigh previously requested changes Mar 18, 2024
apps/web/src/lang/fr.json Outdated Show resolved Hide resolved
apps/web/src/lang/fr.json Outdated Show resolved Hide resolved
@brindasasi brindasasi marked this pull request as ready for review March 18, 2024 18:09
@brindasasi brindasasi requested a review from mnigh March 18, 2024 19:53
Copy link
Member

@esizer esizer left a comment

Choose a reason for hiding this comment

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

Tried reviewing this but actually got a 500 from the API 😒

screenshot_19-Mar-2024_08-28-1710851337

@brindasasi
Copy link
Contributor Author

Tried reviewing this but actually got a 500 from the API 😒

screenshot_19-Mar-2024_08-28-1710851337

how did you get this error ? πŸ€” It works fine for me

@esizer
Copy link
Member

esizer commented Mar 19, 2024

how did you get this error ? πŸ€” It works fine for me

I just updated a pool to not have required_level like the first step suggested πŸ˜…

Open any existing process without required level on the skills

@brindasasi
Copy link
Contributor Author

brindasasi commented Mar 19, 2024

how did you get this error ? πŸ€” It works fine for me

I just updated a pool to not have required_level like the first step suggested πŸ˜…

Open any existing process without required level on the skills

I'm able to reproduce the error when I wipe out the existing required level and replace it with empty string.
The reason is that PoolCandidateSnapshot query couldn't parse the empty string into enum.
I don't think anyone would be able to add empty string for the Required Level via UI and not a valid use case IMO.

Currently in prod all existing processes have NULL in their requiredLevel column.
Similarly when I wipe out the required level column in my local and leave it as NULL, the fix works fine.

Let me know what do you think on that.

@vd1992
Copy link
Contributor

vd1992 commented Mar 19, 2024

Works... Partially

image

Comment on lines +79 to +81
let title = "";
const skillLevel = getSkillLevelMessage(poolSkill, intl);
if (!isEmpty(skillLevel)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

getSkillLevelMessage() will never fail the check as it returns a truthy string guaranteed, it either returns a skill level or the fallback notFound

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops ..looks like I have put back the old code during merge conflict.

Copy link
Contributor

@vd1992 vd1992 left a comment

Choose a reason for hiding this comment

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

Looks good!

@brindasasi brindasasi requested a review from esizer March 20, 2024 13:52
Copy link
Member

@esizer esizer left a comment

Choose a reason for hiding this comment

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

LGTM! πŸ‘

@brindasasi brindasasi added this pull request to the merge queue Mar 20, 2024
Merged via the queue into main with commit 08543b7 Mar 20, 2024
9 checks passed
@brindasasi brindasasi deleted the 9757-handle-null-state-skill-level branch March 20, 2024 16:49
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.

πŸ› Null state for pool skill levels on the assessment screening dialog is not very user friendly
4 participants