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

feat: upload job offer 💰 #77

Merged
merged 17 commits into from
Apr 3, 2024

Conversation

hebronmekuria
Copy link
Contributor

@hebronmekuria hebronmekuria commented Apr 2, 2024

Description ✏️

Uploads a student's job offer to the job_offers table in the database.

Closes #4.

  • Implements a function in upload-job-offers.ts file that receives a job offer and adds it to the job_offers table in the db.
  • Adds a jobOffer Zod object in employment.types.ts.

Type of Change 🐞

  • Feature - A non-breaking change which adds functionality.
  • Fix - A non-breaking change which fixes an issue.
  • Refactor - A change that neither fixes a bug nor adds a feature.
  • Documentation - A change only to in-code or markdown documentation.
  • Tests - A change that adds missing unit/integration tests.
  • Chore - A change that is likely none of the above.

Checklist ✅

  • I have done a self-review of my code.
  • I have manually tested my code (if applicable).
  • I have added/updated any relevant documentation (if applicable).

@hebronmekuria hebronmekuria requested a review from ramiAbdou as a code owner April 2, 2024 18:49
@hebronmekuria hebronmekuria changed the title chore: upload job offer function and tests chore: upload job offer function and tests 💰 Apr 2, 2024
Copy link
Member

@ramiAbdou ramiAbdou left a comment

Choose a reason for hiding this comment

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

Hey @hebronmekuria - I was able to take a look at the build errors and it was actually quite difficult to track down because Kysely's error messages weren't the best. But the root of the issue was that student_id is non-nullable in the database, but in our Zod object it was nullish. The camelCase/snake_case conversion didn't seem to be the issue.

Also, in running things locally, I realized that I need to fix the test setup for the database - it's not properly working now that we have multiple test files that use the database (I think I know why).

I pushed up a fix right now for the type error, and I also removed the test file temporarily. I want to get your PR in, and then we can circle back to add the appropriate tests after I get a fix in. Be sure to keep a copy of your test file somewhere so we can follow back up on that!

If you can just merge my changes into your branch, and update the PR title so that it shows a feat instead of a chore (this is proper backend feature work 🙂), we should be good to merge this in!

@ramiAbdou
Copy link
Member

Also, in the future if the PR has some errors or anything like that, feel free to leave it out of the PR description and instead you can add comments directly in your code in the "Files Changed" tab so that everything is colocated. That way we can keep the PR description clean and only describing the feature itself!

@hebronmekuria hebronmekuria changed the title chore: upload job offer function and tests 💰 feat: upload job offer function and tests 💰 Apr 2, 2024
@hebronmekuria
Copy link
Contributor Author

Okay, great! Also, can company id be null? I also have that as nullist in the Zod object

@hebronmekuria
Copy link
Contributor Author

Never mind, comapny id is nullable in the database! So to get this straight, I should do a PR without the test file, even though the tests still don't pass right?

@ramiAbdou
Copy link
Member

Yup, companyId can be null. The reason being is because not all companies are in our database, so we allow free-text in the case that it's not in our database (and we don't create a new company record if the user inputs free-text).

@ramiAbdou
Copy link
Member

So to get this straight, I should do a PR without the test file, even though the tests still don't pass right?

Yup, just remove the test file and that will actually make the tests pass!

@hebronmekuria
Copy link
Contributor Author

Nice, all the tests passed....but the tests that are running pretain to a different file right, or are we testing and confirming if uploadjoboffer works?

@ramiAbdou
Copy link
Member

Yup the tests are from elsewhere in the codebase!

@hebronmekuria
Copy link
Contributor Author

Okay, nice, just pushed the changes. so are we al good for uploadjoboffers, and should I start on listjoboffers?

@hebronmekuria
Copy link
Contributor Author

I cleaned up the PR too. Let me know if I should be more description Omar's recent PR came in clutch 💯

@hebronmekuria hebronmekuria changed the title feat: upload job offer function and tests 💰 feat: uploads job offer 💰 Apr 2, 2024
@ramiAbdou ramiAbdou changed the title feat: uploads job offer 💰 feat: upload job offer 💰 Apr 2, 2024
Copy link
Member

@ramiAbdou ramiAbdou left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@ramiAbdou
Copy link
Member

Yes, you're all good to go on listJobOffers! I can create another issue for you with some better acceptance criteria!

@ramiAbdou ramiAbdou merged commit c09c1d9 into colorstackorg:main Apr 3, 2024
1 check passed
@hebronmekuria hebronmekuria deleted the uploadJobOffer branch June 14, 2024 16:24
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.

Kickoff: Compensation Data 💰
2 participants