Finished project #2
Conversation
| * @param \Illuminate\Http\ClientStoreRequest $request | ||
| * @return \Illuminate\Http\Response | ||
| */ | ||
| public function store(\App\Http\Requests\ClientStoreRequest $request) |
There was a problem hiding this comment.
@faxunil why don't you add use App\Http\Requests\ClientStoreRequest on top instead, and then you wouldn't need to use full path here
| */ | ||
| public function create() | ||
| { | ||
| abort_if(!Auth::user()->hasRole('admin'), 401, __('Not Allowed action')); |
There was a problem hiding this comment.
@faxunil technically, 401 code means "unauthenticated", which means not logged in. In this case, it should be 403, meaning "unauthorized" or "logged in but has no permission".
| */ | ||
| public function store(ClientStoreRequest $request) | ||
| { | ||
| if (!$client=Client::create($request->validated())) { |
There was a problem hiding this comment.
@faxunil I don't think this check and redirect back is necessary. What wrong could potentially happen in the Client::create statement? I've never had such case in my experience, all the checks should happen on validation level.
| if (!$client=Client::create($request->validated())) { | ||
| return redirect()->back()->with(['error' => __('Create error')]); | ||
| } | ||
| return redirect()->to(url('/clients'))->with(['message' => $client->name . ' ' . __('Successfully created')]); |
There was a problem hiding this comment.
@faxunil not sure what error you're having with route names, but please debug it, or give more details, it should be return redirect()->route(some_route_name)
| public $clients=[]; | ||
| public function __construct() | ||
| { | ||
| $users=User::with('roles')->orderBy('name')->get(); |
There was a problem hiding this comment.
@faxunil please format the code according to PSR standards, there should be space for = sign, like $users = []; and not $users=[];, your code is pretty hard to read.
|
|
||
| public function __construct() | ||
| { | ||
| $this->projects = Project::orderBy('title')->get()->pluck('title', 'id'); |
There was a problem hiding this comment.
@faxunil you can use pluck without get(), please search my channel for video about pluck.
|
|
||
| public function projects() | ||
| { | ||
| return $this->hasMany(Project::class,'user_id'); |
There was a problem hiding this comment.
@faxunil I think the user_id is unnecessary parameter here.
| * @param \App\Models\Client $client | ||
| * @return void | ||
| */ | ||
| public function saving(Client $client) |
There was a problem hiding this comment.
@faxunil there are separate methods like creating() and updating() and deleting() from what I remember.
| $table->string('title')->index(); | ||
| $table->text('description')->nullable(); | ||
| $table->dateTime('due_date')->index(); | ||
| $table->foreignId('user_id')->constrained('users'); |
There was a problem hiding this comment.
@faxunil if the name is with convention, like xxxxx_id then you can just do constrained() without table name
| @endforeach | ||
| </ul> | ||
| @endif | ||
| <form action="{{url('/clients')}}" method="POST" enctype="multipart/form-data"> |
There was a problem hiding this comment.
@faxunil should be route() as well, not sure why it didn't work for you
| <form action="{{url('/clients/'.$client->id)}}" method="POST"> | ||
| @csrf | ||
| @method('PUT') | ||
| <input name="id" value="{{old('id',$client->id)}}" type="hidden"/> |
There was a problem hiding this comment.
@faxunil why you add those hidden fields? The id is already part of the URL, and created_by shouldn't be passed from the form, it should be automatic.
| </thead> | ||
| <tbody class="bg-white divide-y divide-gray-200"> | ||
| @if(count($client->projects)>0) | ||
| @foreach($client->projects as $project) |
There was a problem hiding this comment.
@faxunil look up the documentation for @forelse structure
|
|
||
| <a href="{{url('/clients/'.$client->id.'/edit')}}" | ||
| class="text-indigo-600 hover:text-indigo-900 inline-flex">{{__('Edit')}}</a> | ||
| @if($client->projects->count()==0) |
There was a problem hiding this comment.
@faxunil I think this will cause N+1 query. Haven't tested, but it should be probably $clients->projects()->count()
My video: https://www.youtube.com/watch?v=MbksBczShcA
|
@faxunil ok reviewed your code and added comments where I noticed something missing or wrong. But route naming is really important, try something like |
Dear Mr. Povilas Korop!
I would appreciate your opinion and comments.
I also have 2 question also
GET|HEAD | api/clients | App\Http\Controllers\Api\ClientController@index
GET|HEAD | clients | App\Http\Controllers\ClientController@index
Thanks for Your effort.
Br.
Gergely