trjhtr

I'm looking for a code review for the following system that I created. It allows a landlord, to add a tenant to start a tenancy. It has added, accept, reject, and prohibits a tenant from adding themselves. Tenants can't add landlords.

First thing up is my controller

The form is what is rendered when the landlord clicks add. It prepopulates with landlord name and id. The tenant's name, and id, and also the landlord's properties.

 //Renders Form
 public function create($id)
 $user = User::where('id', $id)->first();
 $properties = PropertyAdvert::where('user_id', Auth::id())->get();

 return view('/pages/account/tenancy/create', compact('user', 'properties'));
 

 //Stores data
 public function store(Request $request, User $user)
 $properties = PropertyAdvert::where('user_id', Auth::id())->get();
 $Tenancy = Tenancy::create([
 'tenant_id' => $request->tenant_id,
 'tenant_name' => $request->tenant_name,
 'landlord_id' => $request->landlord_id,
 'landlord_name' => $request->landlord_name,
 'property_address' => $request->property_address,
 ]);

 //Redirct somewhere
 

 public function accept(Request $request)
 Tenancy::where('accepted', 0)->where('request_sent', 1)
 ->update(
 [
 'accepted' => 1,
 'request_sent' => 0,
 ]

 );
 return back();
 

 public function reject(Request $request)
 Tenancy::where('accepted', 0)->where('request_sent', 1)
 ->update(
 [
 'accepted' => 0,
 'request_sent' => 0,
 ]
 );
 return back();
 

 public function end(Request $request)
 Tenancy::where('accepted', 1)->update(
 [
 'accepted' => 0,
 ]
 );
 return back();
 

This is the view
This is the tenants page. Only the landlord can add, but only the tenant can accept/reject and end the tenancy. Different buttons appear based on the bolleans in the db

 <div class="container">

 <!-- ============== Tenant Profile section ============== -->

 @if($user->userType != "Landlord")
 <div class="row">
 <!-- 
 If the request hasn't been sent, and hasn't been excepted.
 Provie button to send a tenancy request
 -->

 <!-- 
 If tennancy = null
 if tennacny not accepted
 if request hasn't been sent
 if currently signed in user != user/tenant id
 -->
 @if(Auth::user()->id == $user->id)
 <h1>You cannot add yourself</h1>
 @elseif($Tenancy == null || $Tenancy->accepted == 0 && $Tenancy->request_sent != 1)
 <a href="/account/tenancy/$user->id/create" class="btn btn-primary">Start Tenancy</a>
 @endif

 <!-- 
 If the user signed in, isn't the owner of this profile.
 Do not show these buttons that control accept/reject/end
 -->

 @if(Auth::user()->id == $user->id)
 <!-- 
 If the request has been sent, but hasn't been accepted.
 Give option to accept and reject.
 This updates the values in DB.
 -->
 @if($Tenancy != null && $Tenancy->accepted == 0 && $Tenancy->request_sent == 1)
 <form method="POST" action="/account/tenancy/$user->id/accept">
 csrf_field() 
 <input type="submit" class="btn btn-primary" value="Accept Request">
 </form>
 <form method="POST" action="/account/tenancy/$user->id/reject">
 csrf_field() 
 <input type="submit" class="btn btn-warning" value="Reject Request">
 </form>
 <!-- 
 If the request has been accepted.
 Show button to end the tenancy,
 and property details
 -->
 @elseif($Tenancy != null && $Tenancy->accepted == 1 && $Tenancy->request_sent == 0)
 <form method="POST" action="/account/tenancy/$user->id/end">
 csrf_field() 
 <input type="submit" class="btn btn-primary" value="End Tenancy">
 </form>
 <h5>Currently in Tenancy with $Tenancy->landlord_name</h5>
 <h5>Your property is $Tenancy->property_address</h5>
 @endif <!-- End of current user vs this user-->
 @endif <!-- Initial If-->
 </div>

 <div class="row mt-4">
 <div class="col-md-9">
 <span class="text-lead text-center">Your watched properties</span>
 <hr>
 <div class="row py-2">
 @foreach ($user->WatchedProperties as $WatchedProperties)
 <div class="col-md-4 mb-4">
 <a href="/property/$WatchedProperties->image_info">
 <img class="list-image img-fluid" src="$WatchedProperties->image_url">
 </a>
 <p class="mt-2">$WatchedProperties->address .', '. $WatchedProperties->town .', '. $WatchedProperties->county</p>
 </div>
 @endforeach
 </div>
 </div>
 <div class="col-md-3 spacing">
 <table class="table">
 <tbody>
 <!--
 Looping thorugh all watchlists.
 Watchlist controller Index
 -->
 <a href="#" class="link-sub-title">Property Preferences</a>
 <p class="text-sub-title">Your Watchlists</p>
 @foreach ($Watchlists as $Watchlist)
 <tr>
 <td>
 <a href="/watchlist/$Watchlist->id">
 $Watchlist->title
 </a>
 </td>
 </tr>
 @endforeach
 </tbody>
 </table>
 </div>
 </div>

Magic strings

You are using the magic string "landlord". Consider using a static variable, for example on your user model instead. Using a static variable has some advantages over using this magic string:

• IDE's will allow you to inspect where your variable is used


• You have all the possible options in one place, preventing you from having to guess it


• Changing the string is now an easy task, instead of a very tedious one where you have to search for it through your entire codebase

Validate your requests

I don't think you are validating anything right now. You prevent the forms from showing for certain users, but do not prevent the endpoints from accepting requests.

Add casts in your models

You can use $casts in your models to use booleans instead of ints for your accepted and request_sent variables on your Tenancy.

Use blade comments

You are currently using html comments, which will be rendered in the source of your page. Instead use blade comments (ala -- My comment --) which will be translated to php comments and thus be hidden.

Some side notes

Consider using @csrf instead of csrf_field() . The change is purely cosmetic though.

I am assuming that you are using the VerifyCsrfToken and the auth middleware to prevent to verify the csrf token you added and make sure a user is actually logged in.