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/job application show #35

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,11 @@ Unsuccessful Response(job application does not exist OR belongs to another user)
{:message=>"Job application not found", :status=>404}
```

Unsuccessful Response(missing job application ID param):
```
{:message=>"Job application ID is missing", :status=>400}
```

### Companies

Get login credentials
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/api/v1/job_applications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ def create
end

def show
if params[:id].blank?
render json: ErrorSerializer.format_error(ErrorMessage.new("Job application ID is missing", 400)), status: :bad_request
return
end
Comment on lines +15 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

user = User.find(params[:user_id])
job_application = JobApplication.find_by(id: params[:id])

Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
namespace :v1 do
resources :users, only: [:create, :index, :show, :update] do
resources :job_applications, only: [:create, :show]
get '/job_applications(/:id)', to: 'job_applications#show'
end
resources :companies, only: [:create, :index]

Expand Down
12 changes: 12 additions & 0 deletions spec/requests/api/v1/job_applications/job_application_show_spec.rb
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another potential edge case would be a lack of parameters being sent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,18 @@
expect(json[:message]).to eq("Job application not found")
expect(json[:status]).to eq(404)
end

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
Comment on lines +112 to +123
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

end
end
end