Add Support for Creating Contacts with Optional Company Information #61
Conversation
There was a problem hiding this comment.
The changes made to the controllers looks great. I appreciate the checks for companies to make those connections while the contact is being made.
I asked for some more information to be provided in the README, I don't see anything in the code specifically that needs to be addressed before approval.
|
|
||
| Authorization: Bearer Token - put in token for user | ||
|
|
||
| raw json body with all fields: |
There was a problem hiding this comment.
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.
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.
The Read Me has been updated.
| 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.
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.
Yes, definitely. There is a ticket up for that, #58 in backlog.
There was a problem hiding this comment.
Ope! I'm sorry I forgot that you mentioned that in the stand up.
config/routes.rb
Outdated
There was a problem hiding this comment.
Can you let me know why we need a contacts :create under both contacts and /companies/contacts?
There was a problem hiding this comment.
Great question! In Create a Contact, we need that companies/create to create a contact with a company. If we create a contact without a company, then it goes to the contacts/create route. It was the last piece needed for that page.
There was a problem hiding this comment.
Extremely thorough testing!
Type of Change
Description
• Refactored ContactSerializer to include company information if a contact is created with a company.
• Added tests for the ContactController to validate logic for handling nested contact routes and companies.
• Implemented a create action for the nested contacts route.
Motivation and Context
These changes allow a contact to be created with a company.name/company_id or without a company, ensuring flexibility and maintaining a consistent user experience. The additional tests ensure 100% code coverage in SimpleCov for the Contact Controller's new logic.
closes #60
closes #3
Related Tickets
• Refactoring of the ContactsController has been moved to ticket #58.
Added Test?
Request/Controller Specs?
Checklist: