Projekt Laravel

0

Witajcie, proszę o spojrzenie na ten projekt: https://tinyurl.com/y8wcvck8

  1. Zależy mi zwłaszcza na ocenie komponentu do eksportu pliku excelowego (app/Modules/Companies/Exports).
  2. Ponieważ projekt służył nauce nie tylko języka i frameworka, ale także np. samej koncepcji uprawnień i ról, to założenia z tym związane są osiągnięte w najprostszy możliwy sposób. Czy na projekt do szukania pierwszej pracy to wystarcza?

Za każdą uwagę bardzo dziękuję.

1

Commit który nazywa się small improvements? Small view fix, Small changes, Improvements, Moved to modules? Nie musiałem zaglądać w kod żeby Cię ocenić,

Zamiast Small improvements mogłeś wpisać Composer update, Add logs on Db::listen() and Refactor UserSeeder.
Zamiast Small view fix mogłeś napisać Correct html markup
Zamiast Small changes mogłeś napisać Use config() global method instead of Config::get() facade
Zamiast Improvements mogłeś wpisać Update EmployeesController.store() validator, Pass "firstName" to editemployee.blade, Update CompaniesTableSeeder <- przy czym to powinno być rozbite na kilka mniejszych commitów.

1

Rzeczywiście słabe nazwy komitów, ale to nie jest takie ważne jak dobry kod i dobre praktyki związane z kodem.

Pusty routing. Widzę, że to jakiś generator kontrolerów. Sądzę, że takie rozwiązanie nie jest zbyt dobre.
Jak chcesz operować na dowolnym crudzie, lepiej działać na bazie danych , i tworzyć tabele i rekordy w kontrolerze, a nie kontrolery w locie.

https://bitbucket.org/alksndr2/simplecrm/src/automate-modules/app/Modules/Companies/Controllers/EmployeesController.php

   public function store(Request $request, Company $company){

        $validated=$this->validate($request,[
            'firstName'=>'required',
            'lastName'=>'required',
            'email'=>'required|email',
            'phone'=>'required|integer'
        ]);

        $employee= new Employee();
        $employee->firstName=$validated['firstName'];
        $employee->lastName=$validated['lastName'];
        $employee->email=$validated['email'];
        $employee->phone=$validated['phone'];
        $company->employees()->save($employee);
        return back()->with('message','Record added');;

    }

Nie używasz Form Request - https://laravel.com/docs/5.7/validation#form-request-validation.
Nowego rekordu nie dodaje się przez słówko new tylko przez fasadę i metodę create, czyli:

Employee::create([
'firsr_name' => request->first_name,
'last_name' => $request->last_name,
])

chyba, że jest ku temu rzeczywisty powód.

Wartości przesyłanych pól wyciąga się z requesta, a nie z walidatora.
Do wyciągania wartości z modeli i określania kluczy kolumn tabeli używa się snake case, czyli xxx_xxx, camel case używa się do nazw zmiennych $xxxXXX i nazw funkcji.

https://bitbucket.org/alksndr2/simplecrm/src/automate-modules/app/Modules/Companies/Models/Employee.php

Masz namespace App w tym pliku, zamiast App\Modules\Companies\Models\Employee;

Modele są puste w środku, nie używasz mutatorów, ani scopów.

https://laravel.com/docs/5.7/eloquent#local-scopes
https://laravel.com/docs/5.7/eloquent-mutators

Nie wiem czemu wpakowałeś wszystko do Modules, od routingu masz folder routes, od kontrolerów masz folder Http/Controllers. Do widoków jest resources/views.

Route::post('edit-company/update-company/{company}', 'HomeController@update')->name('update');"

Do aktualizacji rekordu stosuje się metodę patch:

Route::patch('company/{company}', 'HomeController@update')->name('company.update');

Kontrolery mają być crudowe, więc do operacji związanych z company ma być kontroler CompaniesController, a nie HomeController.

Route::get('company/{company}/employees/{employee}/delete', 'EmployeesController@delete');

Nie usuwa się rekordów metodą get.
Jest to niebezpieczne, bo użytkownik może wpisując w adres url usuwać zasoby.
Do tego celu stosuje się metodę POST, a w Laravelu dokładniej DELETE.
Route::delete('company/{company}/employees/{employee}', 'EmployeesController@delete'
Jest to metoda post z dodatkowym polem formularza _method="DELETE",

$employee=Employee::find($employee); // powinny być spacje oddzielające operatory,
$employee = Employee::find($employee);

 public function edit($company, $employee){
        $employee=Employee::find($employee);
        return view('Companies::editemployee', compact('employee'));
    }

jeżeli używasz nazy zmiennej $employee to możesz zbindować model - https://laravel.com/docs/5.7/routing#route-model-binding
i wtedy nie musisz używać find:

public function edit($company, Employee $employee){

        return view('Companies::editemployee', compact('employee'));
    }

a jak nie chcesz bindować to nazywaj zmienną tym czym jest, czyli $employeeId

 public function edit($company, $employeeId){
        $employee=Employee::find($employeeId);
        return view('Companies::editemployee', compact('employee'));
    }

<?php

namespace App\Modules\Companies\Controllers;

use Illuminate\Http\Request;
use App\Http\Controllers\Controller;
use App\Modules\Companies\Models\Company;
use App\Modules\Companies\Models\Employee;
use App\Modules\Companies\Exports\CompaniesExport;
use Maatwebsite\Excel\Facades\Excel;
use Carbon\Carbon;

class ExportController extends Controller
{
    public function __construct()
    {
        $this->middleware('auth');
        $this->middleware(['role:admin']);
    }
    public function exportInfo()
    {
        $date = Carbon::now()->format('Y-m-d');
        return (new CompaniesExport)->download('companies'.$date.'.xlsx');
    }

}

CompaniesExport powinno zostać wstrzyknięte w konstruktorze:


class ExportController extends Controller
{
   protected $companiesExport;

    public function __construct(CompaniesExport $companiesExport)
    {
        $this->middleware('auth');
        $this->middleware(['role:admin']);
       $this->companiesExport = companiesExport;
    }
    public function exportInfo()
    {
        $date = Carbon::now()->format('Y-m-d');
        return ($this->companiesExport)->download('companies'.$date.'.xlsx');
    }

}

1 użytkowników online, w tym zalogowanych: 0, gości: 1