4
\$\begingroup\$

I am working on a Laravel application (Github repo) that requires user registration and login.

Alter registration, the users can change their registration details (except password, for which there is the default password recovery functionality) and add more info.

enter image description here

They also have the possibility to replace the default avatar image with a picture of their choice

In routes\web.php I have:

use Illuminate\Support\Facades\Route;

Route::get('/', [App\Http\Controllers\Frontend\HomepageController::class, 'index'])->name('homepage');

Auth::routes();

Route::get('/dashboard', [App\Http\Controllers\Dashboard\DashboardController::class, 'index'])->name('dashboard');

Route::get('/dashboard/profile', [App\Http\Controllers\Dashboard\UserProfileController::class, 'index'])->name('profile');

Route::post('/dashboard/profile/update', [App\Http\Controllers\Dashboard\UserProfileController::class, 'update'])->name('profile.update');

Route::post('/dashboard/profile/deleteavatar/{id}', [App\Http\Controllers\Dashboard\UserProfileController::class, 'deleteavatar'])->name('profile.deleteavatar'); 

In Controllers\Dashboard\UserProfileController.php I have:

namespace App\Http\Controllers\Dashboard;
use App\Http\Controllers\Controller;
use Illuminate\Http\Request;
use Auth;
use App\Models\UserProfile;

class UserProfileController extends Controller
{

  // Guard this route  
    public function __construct() {
            $this->middleware('auth');
    }

    public function index(UserProfile $user)
    {
        return view('dashboard.userprofile',
            array('current_user' => Auth::user()) 
        );
    }

    public function update(Request $request)
    {
            $current_user = Auth::user();
            
            $request->validate([
                    'first_name' => ['required', 'string', 'max:255'],
                    'last_name' => ['required', 'string', 'max:255'],
                    'email' => ['required', 'email', 'max:100', 'unique:users,email,'. $current_user->id],
                    'avatar' => ['mimes:jpeg, jpg, png, gif', 'max:2048'],
            ]);
        
            $current_user->first_name = $request->get('first_name');
            $current_user->last_name = $request->get('last_name');
            $current_user->email = $request->get('email');
            $current_user->bio = $request->get('bio');

            // Upload avatar
            if (isset($request->avatar)) {
                $imageName = md5(time()) . '.' . $request->avatar->extension();
                $request->avatar->move(public_path('images/avatars'), $imageName);
                $current_user->avatar = $imageName;
            }
        
            // Update user
            $current_user->update();
            return redirect('dashboard/profile')
                ->with('success', 'User data updated successfully');
        }

        // Delete avatar
        public function deleteavatar($id) {
            $current_user = Auth::user();
            $current_user->avatar = "default.png";
            $current_user->save();
        }
}

The update profile form:

<form action="{{ route('profile.update') }}" enctype='multipart/form-data' method="post" novalidate>
  {{csrf_field()}}
  <div class="form-group">
    <input type="text" id="first_name" name="first_name" placeholder="First name" class="form-control" value="{{old('first_name', $current_user->first_name)}}">
    @if ($errors->has('first_name'))
    <span class="errormsg text-danger">{{ $errors->first('first_name') }}</span>
    @endif
  </div>
  <div class="form-group">
    <input type="text" id="last_name" name="last_name" placeholder="Last name" class="form-control" value="{{old('last_name', $current_user->last_name)}}">
    @if ($errors->has('first_name'))
    <span class="errormsg text-danger">{{ $errors->first('last_name') }}</span>
    @endif
  </div>
  <div class="form-group">
    <input type="text" id="email" name="email" placeholder="E-mail address" class="form-control" value="{{old('email', $current_user->email)}}">
    @if ($errors->has('email'))
    <span class="errormsg text-danger">{{ $errors->first('email') }}</span>
    @endif
  </div>
  <div class="form-group">
    <textarea name="bio" id="bio" class="form-control" cols="30" rows="6">{{old('bio', $current_user->bio)}}</textarea>
    @if ($errors->has('bio'))
    <span class="errormsg text-danger">{{ $errors->first('bio') }}</span>
    @endif
  </div>
  <label for="avatar" class="text-muted">Upload avatar</label>
  <div class="form-group d-flex">
    <div class="w-75 pr-1">
      <input type='file' name='avatar' id="avatar" class="form-control border-0 py-0 pl-0 file-upload-btn" value="{{$current_user->avatar}}">
      @if ($errors->has('avatar'))
      <span class="errormsg text-danger">{{ $errors->first('avatar') }}</span>
      @endif
    </div>
    <div class="w-25 position-relative" id="avatar-container">
      <img class="rounded-circle img-thumbnail avatar-preview" src="{{asset('images/avatars')}}/{{$current_user->avatar}}" alt="{{$current_user->first_name}} {{$current_user->first_name}}">
      <span class="avatar-trash">
      @if($current_user->avatar !== 'default.png')
      <a href="#" class="icon text-light" id="delete-avatar" data-uid="{{$current_user->id}}"><i class="fa fa-trash"></i></a>
      @endif
      </span>
    </div>
  </div>
  <div class="form-group d-flex mb-0">
    <div class="w-50 pr-1">
      <input type="submit" name="submit" value="Save" class="btn btn-block btn-primary">
    </div>
    <div class="w-50 pl-1">
      <a href="{{route('profile')}}" class="btn btn-block btn-primary">Cancel</a>
    </div>
  </div>
</form>

The deleting of the user's picture (reverting to the default avatar, in other words, is done via AJAX):

(function() {
    //Delete Avatar
    $('#delete-avatar').on('click', function(evt) {
        evt.preventDefault();
        var $avatar = $('#avatar-container').find('img');
        var $topAvatar = $('#top_avatar');
        var $trashIcon = $(this);
        var defaultAvatar = APP_URL + '/images/avatars/default.png';

        //Get user's ID
        var id = $(this).data('uid');

        if (confirm('Delete the avatar?')) {
            var CSRF_TOKEN = $('meta[name="csrf-token"]').attr('content');
            $.ajax({
                url: APP_URL + '/dashboard/profile/deleteavatar/' + id,
                method: 'POST',
                data: {
                    id: id,
                    _token: CSRF_TOKEN,
                },
                success: function() {
                    $avatar.attr('src', defaultAvatar);
                    $topAvatar.attr('src', defaultAvatar);
                    $trashIcon.remove();
                }
            });
        }
    });
})();

Questions:

  1. Could the code be significantly "shortened"?
  2. Are there better alternatives to the means I have chosen to do the various "actions" (inserting/updating the avatar and bio, etc)?
  3. How can this be improved (in any way)?
\$\endgroup\$
1
+50
\$\begingroup\$

The suggestions below should allow the code to be shortened and improved.

Update method

The UserProfileController::update() method is somewhat long. The sections below should allow it to be simplified.

Pass fields to update to model method update()

The UserProfileController::update() method is somewhat long. Presuming that the model UserProfile is a sub-class of Illuminate\Database\Eloquent\Model then the update() method can be passed an array of attributes to update. Instead of these lines:

$current_user->first_name = $request->get('first_name');
$current_user->last_name = $request->get('last_name');
$current_user->email = $request->get('email');
$current_user->bio = $request->get('bio');

Get an array of fields to update from $request->all(), then set the avatar on that array if the avatar needs to be updated.

Make a form request class for handling the validation

The validation rules could be moved out to a FormRequest subclass.

namespace App\Http\Requests;

use Auth;
use Illuminate\Foundation\Http\FormRequest;

class UserUpdateRequest extends FormRequest
{
    public function rules()
    {
        return [
            'first_name' => ['required', 'string', 'max:255'],
            'last_name' => ['required', 'string', 'max:255'],
            'email' => ['required', 'email', 'max:100', 'unique:users,email,'. Auth::user()->id],
            'avatar' => ['mimes:jpeg, jpg, png, gif', 'max:2048'],
        ];
    }
}

If the validation fails and the request was an XHR request, an HTTP response with a 422 status code will be returned to the user including a JSON representation of the validation errors..

Then that subclass can be injected instead of Illuminate\Http\Request in the update method arguments and use $request->all() to get the fields to pass to $current_user->update().

  public function update(UserUpdateRequest $request)
  {
      $current_user = Auth::user();
      $fieldsToUpdate = $request->all()
      // Upload avatar
      if (isset($request->avatar)) {
          $imageName = md5(time()) . '.' . $request->avatar->extension();
          $request->avatar->move(public_path('images/avatars'), $imageName);
          $fieldsToUpdate['avatar'] = $imageName;
      }
    
      // Update user
      $current_user->update($fieldsToUpdate);
      return redirect('dashboard/profile')
          ->with('success', 'User data updated successfully');
  }

Middleware

Instead of setting the middleware in the controller, a Middleware Group could be added to routes\web.php - e.g.

Route::group(['middleware' => ['auth']], function() {
    Route::get('/dashboard', [App\Http\Controllers\Dashboard\DashboardController::class, 'index'])->name('dashboard');

    Route::get('/dashboard/profile', [App\Http\Controllers\Dashboard\UserProfileController::class, 'index'])->name('profile');

    Route::post('/dashboard/profile/update', [App\Http\Controllers\Dashboard\UserProfileController::class, 'update'])->name('profile.update');

    Route::post('/dashboard/profile/deleteavatar/{id}', [App\Http\Controllers\Dashboard\UserProfileController::class, 'deleteavatar'])->name('profile.deleteavatar'); 
});

Also, for the sake of readability (e.g. see section 2.3 of PSR-12) it would be wise to alias the profile controller using a use statement.

use App\Http\Controllers\Dashboard\UserProfileController;

Then each reference can simply be UserProfileController Instead of the fully qualified name.

Resource Controller

While it may not save many lines and would likely require updating the route paths, consider using a Resource controller. The Update route would instead be /dashboard/profile with the verb PUT or PATCH.

Testing

Laravel offers great support for writing feature tests to ensure the routes output what you expect. It appears there is already a factory for the user and a migration for the user table so those could be used with the RefreshDatabase trait in tests.

The tests could use the SQLite database engine for testing- simply by uncommenting the lines 24 and 25 of phpunit.xml.

Feature tests can make great use of the HTTP test functions available- e.g. requesting routes acting as a user (see the example in that section about using a model factory to generate and authenticate a user) and ensuring the status is okay or redirected to a certain route with assertRedirect().

If using a formRequest subclass as suggested above an assertion could be made that the response code is 422 for invalid input (e.g. missing required field, wrong type of field, etc).

JavaScript

The call to $.ajax() can be replaced with a call to $.post(). Then there is no need to specify the method, and the keys can be removed from the options:

        $.post(
            APP_URL + '/dashboard/profile/deleteavatar/' + id,
            {
                id: id,
                _token: CSRF_TOKEN,
            },
            function() {
                $avatar.attr('src', defaultAvatar);
                $topAvatar.attr('src', defaultAvatar);
                $trashIcon.remove();
            }
        });
\$\endgroup\$
2
  • \$\begingroup\$ Great advice. However, when I go to /dashboard/profile/update I am not redirected to the login page... \$\endgroup\$ Jun 4 at 9:28
  • \$\begingroup\$ I checked out the code suggestion and found that $current_user was undefined in that example for UserUpdateRequest. Updating it works in my sample test and I have updated the example above. \$\endgroup\$ Jun 6 at 4:25
2
\$\begingroup\$

I think I'll give it a shot. :)

1. Could the code be significantly "shortened"?

I think controller wise, it's okay. It's what I've learnt on Laracasts as well, almost the same thing.

Anw, here are my 2 cents :)

userprofile.blade.php

@if ($errors->has('first_name'))
    <span class="errormsg text-danger">{{ $errors->first('first_name') }}</span>
@endif

can be done with:

@error ('first_name')
        <span class="errormsg text-danger">{{ $message }}</span>
@enderror

Also, the {{csrf_field()}} can be replaced with @csrf


2. Are there better alternatives to do the various "actions"?

From what I've learnt and experience, I do recommend checking out a few starter packages:

These little starter kits come with authentication pre-installed. And by looking into it, I'm pretty sure you can make the most use out of these kits. (With smarts like you, I'm pretty sure avatar implementation should be no issue.) :)


3. How can this be improved (in any way)?

May I suggest Vue? I personally think it's beautiful because everything is in a component. Which includes event listeners, html.

It's can also easily be injected into your blade file.

Major plus point: Paired with Inertia, your pages don't have to make a request every time you visit a page. Inertia automatically intercepts a request and makes a partial page reload, which makes your whole blog significantly faster.

This can be included in your blog's navbar, perhaps? Maybe for loading tags etc. Here's a link to their docs

Just a suggestion ¯\_(ツ)_/¯ Let's say, at any point you want to make single page applications (SPA) in your blog, you can do so with Vue.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.