-
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
[Feature] New work experience fields #11947
base: main
Are you sure you want to change the base?
Conversation
// 'workExperience.employmentCategory' => [ | ||
// 'required', | ||
// ], |
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.
Some validation can't be used until the frontend is updated. Validation will need clean-up once the frontend is usable
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.
A/C in #11864
Makes mention of handling this
$employmentCategory = $this->arg('workExperience.employmentCategory') ?? $workExperienceModel->employment_category; | ||
$extSizeOfOrganization = $this->arg('workExperience.extSizeOfOrganization') ?? $workExperienceModel->ext_size_of_organization; | ||
$extRoleSeniority = $this->arg('workExperience.extRoleSeniority') ?? $workExperienceModel->ext_role_seniority; | ||
$govEmploymentType = $this->arg('workExperience.govEmploymentType') ?? $workExperienceModel->gov_employment_type; | ||
$govPositionType = $this->arg('workExperience.govPositionType') ?? $workExperienceModel->gov_position_type; | ||
$govContractStartDate = $this->arg('workExperience.govContractStartDate') ?? $workExperienceModel->gov_contract_start_date; |
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.
Will delete some of these unused ones after some reviewing is done
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 dunno, this doesn't smell quite right to me. Laravel validation (and Lighthouse) is all about validation input but really you want to validate the final state of the model. This seems very clunky though. I think I'll ask on Slack if anyone has ideas.
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 uh, what do we want?
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.
Decision in scrum: don't query existing model, just expect that all the fields to check exist in the input.
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.
Aye aye, reusing the validator then in 688a7a9
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.
Wow, a big chonky one! I just did a code read-through before I start trying things out.
$employmentCategory = $this->arg('workExperience.employmentCategory') ?? $workExperienceModel->employment_category; | ||
$extSizeOfOrganization = $this->arg('workExperience.extSizeOfOrganization') ?? $workExperienceModel->ext_size_of_organization; | ||
$extRoleSeniority = $this->arg('workExperience.extRoleSeniority') ?? $workExperienceModel->ext_role_seniority; | ||
$govEmploymentType = $this->arg('workExperience.govEmploymentType') ?? $workExperienceModel->gov_employment_type; | ||
$govPositionType = $this->arg('workExperience.govPositionType') ?? $workExperienceModel->gov_position_type; | ||
$govContractStartDate = $this->arg('workExperience.govContractStartDate') ?? $workExperienceModel->gov_contract_start_date; |
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 dunno, this doesn't smell quite right to me. Laravel validation (and Lighthouse) is all about validation input but really you want to validate the final state of the model. This seems very clunky though. I think I'll ask on Slack if anyone has ideas.
api/graphql/schema.graphql
Outdated
classification: UUID @rename(attribute: "classification_id") | ||
department: UUID @rename(attribute: "department_id") |
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.
Maybe these two should be something like DepartmentBelongsTo?
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.
Hm, I think I should have "id" in the name of that, changed in 371b58a
Not sure about sticking "BelongsTo" in there
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'm thinking of two reasons why this might be worth doing (even though it works as is). 1) Consistency is good - it will match the other relationship inputs in the schema. 2) If we do eventually change back to a full work_experiences
table we won't be forced to change the schema to make it work.
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.
The reason I am hesitant about trying to keep it consistent is because it isn't the same at all. We're storing values in JSON and I had to install something to mimic normal Eloquent relations when querying the experience 😕
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.
Given that you did install something to mimic normal eloquent relations... would DepartmentBelongsTo work? Maybe try it.
🤖 Resolves #11868
👋 Introduction
Add new fields to work experiences. This included various tasks like factory, testing, schema, snapshotting, and enum changes.
Figma for reference, needed for proofreading
https://www.figma.com/design/piRkRtWAHheoEX5Jq8d9u7/Create%2FEdit-an-experience-(Applicants)?node-id=472-70951
🕵️ Details
Was stumped for a bit on handling classifications and departments through JSON, but our friend Staudenmeir of
EloquentHasManyDeep
fame had an answer,HasJsonRelationships
🧪 Testing
Refresh API first!
Useful operations
Add or subtract fields as needed
📸 Screenshot
Creating
Validator
Querying
🚚 Deployment