Task 1: Code Review #1
Conversation
…-laravel-assignment into feature-001--relational-data-on-create
…-laravel-assignment into feature-001--relational-data-on-create
| 'writer_id' => 'nullable|exists:writers,id', | ||
| 'writer_name' => 'nullable|string|max:255', |
There was a problem hiding this comment.
With both writer_id and writer_name being nullable, the user could try creating a Book without a writer. Please add additional validation that at least one field needs to be present.
| @@ -34,7 +35,7 @@ public function create() | |||
| * @param \Illuminate\Http\Request $request | |||
| * @return \Illuminate\Http\RedirectResponse | |||
| */ | |||
| public function store(Request $request) | |||
| public function store(Request $request, BookRepository $repository) | |||
There was a problem hiding this comment.
Use Dependency Injection for BookRepository
| 'publisher_id' => 'nullable|exists:publishers,id', | ||
| 'publisher_name' => 'nullable|string|max:255', |
There was a problem hiding this comment.
Same for the publisher, users could try creating a Book without a publisher
| return Writer::create([ | ||
| 'name' => $requestData['writer_name'], | ||
| ]); |
There was a problem hiding this comment.
Assuming that the acceptance criteria states "if the writer doesn't exist, create a new one", with the current implementation users could enter the same author multiple times since they only enter the writer_name.
If the acceptance criteria states to enable adding a new Writer through the Create Book form, all the fields from the Create Writer form should be present and those fields should be used in checking if the Writer already exists in the database:
| return Publisher::create([ | ||
| 'name' => $requestData['publisher_name'], | ||
| 'location' => $requestData['publisher_location'], | ||
| ]); |
There was a problem hiding this comment.
Same for the Publisher, here all the fields are included but possibly the Publisher already exists in the database and is listed in the dropdown.
| @@ -0,0 +1,71 @@ | |||
| <?php | |||
|
|
|||
| namespace App; | |||
There was a problem hiding this comment.
Place this file inside App\Repositories folder
No description provided.