Feat/job application show #35
Conversation
db/seeds.rb
Outdated
There was a problem hiding this comment.
I like that you added a seed of the job application. That way you can use it as like default values for the front end and do some testing in the development and testing stages. And you didn't create a bunch of seeds either which would populate even the production database with a bunch of dummy data.
There was a problem hiding this comment.
Thank you, I think once we have company seeds this will be very useful for viewing responses with tools like postman.
| user = User.find(params[:user_id]) | ||
| job_application = JobApplication.find_by(id: params[:id]) | ||
|
|
||
| if job_application.nil? || job_application.user_id != user.id |
There was a problem hiding this comment.
Would we want a different error if the user doesn't match?
There was a problem hiding this comment.
Kinda torn on this, Im not sure if the effort is worth the result, but i'm willing to hear out justifications. Feel free to post some in this thread if anyone is interested.
There was a problem hiding this comment.
Not 100% one way or the other, but clearer messages to the front is helpful and since a lack of job application is a slightly different error than an incorrect user id. They both result in the job application not working as intended, so I also see why one error for each does the job.
There was a problem hiding this comment.
Another potential edge case would be a lack of parameters being sent.
There was a problem hiding this comment.
This is worth doing. I will incorporate this edge case in the testing, if it requires reworking the method, I will incorporate that as well. Thank you :)
There was a problem hiding this comment.
Edge case has been addressed and read me has been updated accordingly, If the change is suitable, feel free to resolve conversation and/or resolve conflict to merge :) Reach out if there Is anything I can help clarify
|
Excellent job Kyle! I pulled your branch down to my local to run the tests and everything seems to be running smoothly. I also noticed that you have 100% test coverage of your controller and model with SimpleCov. |
|
|
||
| it "returns error serializer if no params are passed in URL for job application id" do | ||
| get "/api/v1/users/#{@user.id}/job_applications/" | ||
|
|
||
| expect(response).to_not be_successful | ||
| expect(response.status).to eq(400) | ||
|
|
||
| json = JSON.parse(response.body, symbolize_names: true) | ||
|
|
||
| expect(json[:message]).to eq("Job application ID is missing") | ||
| expect(json[:status]).to eq(400) | ||
| end |
There was a problem hiding this comment.
Thank you for adding this test and being very through with your work. I like your error message here, as I feel it is very informative and user-friendly.
| if params[:id].blank? | ||
| render json: ErrorSerializer.format_error(ErrorMessage.new("Job application ID is missing", 400)), status: :bad_request | ||
| return | ||
| end |
There was a problem hiding this comment.
This is a nice addition to the show action. I noticed that you are grabbing the ID from the params, and not from the authentication token. I'm curious about this decision, as both the companies and contacts are collecting it from the token.
There was a problem hiding this comment.
I noticed that the JobApplications controller is not utilizing the authentication tokens or the authenticate_user method that has been defined in the ApplicationController. Other controllers in the application are using these for authentication, so I wanted to flag this for consistency and security purposes.
Could you clarify if this was intentional or an oversight? If it's an oversight, it might be worth adding the necessary authentication logic to ensure this controller aligns with the others. I'm curious to hear your thoughts!
| if params[:id].blank? | ||
| render json: ErrorSerializer.format_error(ErrorMessage.new("Job application ID is missing", 400)), status: :bad_request | ||
| return | ||
| end |
There was a problem hiding this comment.
I noticed that the JobApplications controller is not utilizing the authentication tokens or the authenticate_user method that has been defined in the ApplicationController. Other controllers in the application are using these for authentication, so I wanted to flag this for consistency and security purposes.
Could you clarify if this was intentional or an oversight? If it's an oversight, it might be worth adding the necessary authentication logic to ensure this controller aligns with the others. I'm curious to hear your thoughts!
Type of Change
Description
Readme.mdMotivation and Context
Related Tickets
Screenshots (if appropriate):
Added Test?
Yes 🫡
job_application_show_specAll previous tests still pass 🥳
Checklist: