-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Support for Creating Contacts with Optional Company Information #61
Changes from all commits
997d1e0
c3dfb21
df329be
3d6bb7a
9272342
e86fa4c
d7e905a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,33 +26,31 @@ def index | |
end | ||
end | ||
|
||
def show | ||
company = @current_user.companies.find_by(id: params[:id]) | ||
authorize company | ||
|
||
if company | ||
contacts = company.contacts | ||
render json: { company: CompanySerializer.new(company), contacts: ContactSerializer.new(contacts) } | ||
def create | ||
authorize Contact | ||
if params[:company_id] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the company controller, this could potentially be refactored into the model. While not necessary for this specific PR, something to make note of, especially if there is any more logic added to the action at a later time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, definitely. There is a ticket up for that, #58 in backlog. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ope! I'm sorry I forgot that you mentioned that in the stand up. |
||
company = @current_user.companies.find_by(id: params[:company_id]) | ||
if company | ||
contact = company.contacts.new(contact_params.merge(user_id: @current_user.id)) | ||
else | ||
return render json: { error: "Company not found" }, status: :not_found | ||
end | ||
else | ||
render json: { error: "Company not found or unauthorized access" }, status: :not_found | ||
contact = @current_user.contacts.new(contact_params) | ||
end | ||
|
||
if contact.save | ||
render json: ContactsSerializer.new(contact), status: :created | ||
else | ||
render json: { error: contact.errors.full_messages.to_sentence }, status: :unprocessable_entity | ||
end | ||
end | ||
|
||
def create | ||
authorize Contact | ||
contact = @current_user.contacts.new(contact_params) | ||
if contact.save | ||
render json: ContactsSerializer.new(contact), status: :created | ||
else | ||
render json: { error: contact.errors.full_messages.to_sentence }, status: :unprocessable_entity | ||
end | ||
end | ||
private | ||
|
||
private | ||
|
||
def contact_params | ||
params.require(:contact).permit(:first_name, :last_name, :company_id, :email, :phone_number, :notes) | ||
end | ||
def contact_params | ||
params.require(:contact).permit(:first_name, :last_name, :company_id, :email, :phone_number, :notes) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,22 @@ | ||
class ContactsSerializer | ||
include JSONAPI::Serializer | ||
attributes :first_name, :last_name, :company_id, :email, :phone_number, :notes, :user_id | ||
|
||
attribute :company do |object| | ||
company = object.company | ||
if company | ||
{ | ||
id: company.id, | ||
name: company.name, | ||
website: company.website, | ||
street_address: company.street_address, | ||
city: company.city, | ||
state: company.state, | ||
zip_code: company.zip_code, | ||
notes: company.notes | ||
} | ||
else | ||
nil | ||
end | ||
end | ||
end |
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.
Perhaps adding some clarification on which of the fields are absolutely required. I saw in the tests you were using a minimal params when creating. So while it is very helpful to have the full JSON as an example, having some information on which fields will trigger the 404 error if not included can clear that up. For example, first and last name are required while email is an optional field.
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.
Thank you for pointing this out. Due to how the DB is currently set up only the first name, last name, and company ID id needed, depending on which URL being used. The Read Me currently states that unique first and last names are required, but does not specify that the new route needs a company ID. I can update that to make it more apparent.
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 Read Me has been updated.