Refactor fares inheritance #905 (#906)

app/Http/Controllers/Api/FlightController.php

@@ -9,6 +9,7 @@ use App\Http\Resources\Navdata as NavdataResource;
 use App\Models\SimBrief;
 use App\Repositories\Criteria\WhereCriteria;
 use App\Repositories\FlightRepository;
+use App\Services\FareService;
 use App\Services\FlightService;
 use Exception;
 use Illuminate\Http\Request;
@@ -18,19 +19,26 @@ use Prettus\Repository\Exceptions\RepositoryException;
 
 class FlightController extends Controller
 {
+    /** @var \App\Services\FareService */
+    private $fareSvc;
+
+    /** @var \App\Repositories\FlightRepository */
     private $flightRepo;
+
+    /** @var \App\Services\FlightService */
     private $flightSvc;
 
     /**
-     * FlightController constructor.
+     * @param FareService      $fareSvc
-     *
      * @param FlightRepository $flightRepo
      * @param FlightService    $flightSvc
      */
     public function __construct(
+        FareService $fareSvc,
         FlightRepository $flightRepo,
         FlightService $flightSvc
     ) {
+        $this->fareSvc = $fareSvc;
         $this->flightRepo = $flightRepo;
         $this->flightSvc = $flightSvc;
     }
@@ -71,6 +79,7 @@ class FlightController extends Controller
         ])->find($id);
 
         $flight = $this->flightSvc->filterSubfleets($user, $flight);
+        $flight = $this->fareSvc->getReconciledFaresForFlight($flight);
 
         return new FlightResource($flight);
     }
@@ -130,6 +139,7 @@ class FlightController extends Controller
         // TODO: Remove any flights here that a user doesn't have permissions to
         foreach ($flights as $flight) {
             $this->flightSvc->filterSubfleets($user, $flight);
+            $this->fareSvc->getReconciledFaresForFlight($flight);
         }
 
         return FlightResource::collection($flights);

app/Http/Resources/Fare.php

@@ -3,7 +3,6 @@
 namespace App\Http\Resources;
 
 use App\Contracts\Resource;
-use App\Services\FareService;
 
 /**
  * @mixin \App\Models\Fare
@@ -12,17 +11,13 @@ class Fare extends Resource
 {
     public function toArray($request)
     {
-        /** @var FareService $fareSvc */
-        $fareSvc = app(FareService::class);
-        $fare = $fareSvc->getFares($this);
-
         return [
-            'id'       => $fare->id,
+            'id'       => $this->id,
-            'code'     => $fare->code,
+            'code'     => $this->code,
-            'name'     => $fare->name,
+            'name'     => $this->name,
-            'capacity' => $fare->capacity,
+            'capacity' => $this->capacity,
-            'cost'     => $fare->cost,
+            'cost'     => $this->cost,
-            'price'    => $fare->price,
+            'price'    => $this->price,
             'type'     => $this->type,
             'notes'    => $this->notes,
             'active'   => $this->active,

app/Http/Resources/Flight.php

@@ -58,7 +58,6 @@ class Flight extends Resource
 
         $res['airline'] = new Airline($this->airline);
         $res['subfleets'] = Subfleet::collection($this->whenLoaded('subfleets'));
-        $res['fares'] = Fare::collection($this->whenLoaded('fares'));
         $res['fields'] = $this->setFields();
 
         // Simbrief info

app/Models/Flight.php

@@ -25,6 +25,10 @@ use Illuminate\Support\Collection;
  * @property int        distance
  * @property int        flight_time
  * @property string     route
+ * @property string     dpt_time
+ * @property string     arr_time
+ * @property string     flight_type
+ * @property string     notes
  * @property int        level
  * @property float      load_factor
  * @property float      load_factor_variance

app/Services/BidService.php

@@ -13,11 +13,15 @@ use Illuminate\Support\Facades\Log;
 
 class BidService extends Service
 {
+    /** @var FareService */
+    private $fareSvc;
+
     /** @var FlightService */
     private $flightSvc;
 
-    public function __construct(FlightService $flightSvc)
+    public function __construct(FareService $fareSvc, FlightService $flightSvc)
     {
+        $this->fareSvc = $fareSvc;
         $this->flightSvc = $flightSvc;
     }
 
@@ -26,7 +30,7 @@ class BidService extends Service
      *
      * @param $bid_id
      *
-     * @return \App\Models\Bid|\Illuminate\Database\Eloquent\Model|object|null
+     * @return \App\Models\Bid|\Illuminate\Database\Eloquent\Model|tests/ImporterTest.php:521object|null
      */
     public function getBid($bid_id)
     {
@@ -55,6 +59,7 @@ class BidService extends Service
 
         foreach ($bids as $bid) {
             $bid->flight = $this->flightSvc->filterSubfleets($user, $bid->flight);
+            $bid->flight = $this->fareSvc->getReconciledFaresForFlight($bid->flight);
         }
 
         return $bids;

app/Services/FareService.php

@@ -10,18 +10,98 @@ use App\Models\PirepFare;
 use App\Models\Subfleet;
 use App\Support\Math;
 use function count;
+use Illuminate\Database\Eloquent\Relations\Pivot;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Log;
+use InvalidArgumentException;
 
 class FareService extends Service
 {
     /**
-     * Get the fares for a particular flight, with an optional subfleet
+     * @param Collection[Fare] $subfleet_fares The fare for a subfleet
-     * This will go through if there are any fares assigned to the flight,
+     * @param Collection[Fare] $flight_fares   The fares on a flight
-     * and then check the fares assigned on the subfleet, and give the
-     * final "authoritative" list of the fares for a flight.
      *
-     * If a subfleet is passed in,
+     * @return Collection[Fare] Collection of Fare
+     */
+    public function getFareWithOverrides($subfleet_fares, $flight_fares): Collection
+    {
+        /**
+         * Make sure we've got something in terms of fares on the subfleet or the flight
+         */
+        if (empty($subfleet_fares) && empty($flight_fares)) {
+            return collect();
+        }
+
+        /**
+         * Check to see if there are any subfleet fares. This might only have fares on the
+         * flight, no matter how rare that might be
+         */
+        if ($subfleet_fares === null || count($subfleet_fares) === 0) {
+            return $flight_fares->map(function ($fare, $_) {
+                return $this->getFareWithPivot($fare, $fare->pivot);
+            });
+        }
+
+        return $subfleet_fares->map(function ($sf_fare, $_) use ($flight_fares) {
+            /**
+             * Get the fare, using the subfleet's pivot values. This will return
+             * the fares with all the costs, etc, that are overridden for the given subfleet
+             */
+            $fare = $this->getFareWithPivot($sf_fare, $sf_fare->pivot);
+
+            /**
+             * Now, using the fares that have already been used from the subfleet
+             * now pass those fares in for the flight to override.
+             *
+             * First look to see that there actually is an override for that fare that's on
+             * the flight
+             */
+            $flight_fare = $flight_fares->whereStrict('id', $fare->id)->first();
+            if ($flight_fare === null) {
+                return $fare;
+            }
+
+            /**
+             * Found an override on the flight for the given fare. Check to see if we
+             * have values there that can be used to override or act as a pivot
+             */
+            $fare = $this->getFareWithPivot($fare, $flight_fare->pivot);
+
+            /**
+             * Finally return the fare that we have, it should have gone through the
+             * multiple levels of reconciliation that were required
+             */
+            return $fare;
+        });
+    }
+
+    /**
+     * This will return the flight but all of the subfleets will have the corrected fares with the
+     * right amounts based on the pivots, and with the correct "inheritence" for the flights
+     *
+     * @param Flight $flight
+     *
+     * @return \App\Models\Flight
+     */
+    public function getReconciledFaresForFlight(Flight $flight): Flight
+    {
+        $subfleets = $flight->subfleets;
+        $flight_fares = $flight->fares;
+
+        /**
+         * @var int      $key
+         * @var Subfleet $subfleet
+         */
+        foreach ($subfleets as $key => $subfleet) {
+            $subfleet->fares = $this->getFareWithOverrides($subfleet->fares, $flight_fares);
+        }
+
+        $flight->subfleets = $subfleets;
+        return $flight;
+    }
+
+    /**
+     * Get the fares for a particular flight, with the subfleet that is in use being passed in
      *
      * @param Flight|null   $flight
      * @param Subfleet|null $subfleet
@@ -33,30 +113,14 @@ class FareService extends Service
         if (!$flight) {
             $flight_fares = collect();
         } else {
-            $flight_fares = $this->getForFlight($flight);
+            $flight_fares = $flight->fares;
         }
 
         if (empty($subfleet)) {
-            return $flight_fares;
+            throw new InvalidArgumentException('Subfleet argument missing');
         }
 
-        $subfleet_fares = $this->getForSubfleet($subfleet);
+        return $this->getFareWithOverrides($subfleet->fares, $flight_fares);
-        if (empty($subfleet_fares) || $subfleet_fares->count() === 0) {
-            return $flight_fares;
-        }
-
-        // Go through all of the fares assigned by the subfleet
-        // See if any of the same fares are assigned to the flight
-        $fares = $subfleet_fares->map(function ($fare, $idx) use ($flight_fares) {
-            $flight_fare = $flight_fares->whereStrict('id', $fare->id)->first();
-            if (!$flight_fare) {
-                return $fare;
-            }
-
-            return $flight_fare;
-        });
-
-        return $fares;
     }
 
     /**
@@ -68,12 +132,24 @@ class FareService extends Service
      */
     public function getFares($fare)
     {
-        $pivot = $fare->pivot;
+        return $this->getFareWithPivot($fare, $fare->pivot);
+    }
+
+    /**
+     * Get the correct price of something supplied with the correct pivot
+     *
+     * @param Fare  $fare
+     * @param Pivot $pivot
+     *
+     * @return \App\Models\Fare
+     */
+    public function getFareWithPivot(Fare $fare, Pivot $pivot)
+    {
         if (filled($pivot->price)) {
-            if (strpos($fare->pivot->price, '%', -1) !== false) {
+            if (strpos($pivot->price, '%', -1) !== false) {
                 $fare->price = Math::addPercent($fare->price, $pivot->price);
             } else {
-                $fare->price = $fare->pivot->price;
+                $fare->price = $pivot->price;
             }
         }
 
@@ -81,7 +157,7 @@ class FareService extends Service
             if (strpos($pivot->cost, '%', -1) !== false) {
                 $fare->cost = Math::addPercent($fare->cost, $pivot->cost);
             } else {
-                $fare->cost = $fare->pivot->cost;
+                $fare->cost = $pivot->cost;
             }
         }
 
@@ -122,24 +198,6 @@ class FareService extends Service
         return $flight;
     }
 
-    /**
-     * return all the fares for a flight. check the pivot
-     * table to see if the price/cost/capacity has been overridden
-     * and return the correct amounts.
-     *
-     * @param Flight $flight
-     *
-     * @return Collection
-     */
-    public function getForFlight(Flight $flight)
-    {
-        $fares = $flight->fares->map(function ($fare) {
-            return $this->getFares($fare);
-        });
-
-        return $fares;
-    }
-
     /**
      * @param Flight $flight
      * @param Fare   $fare

tests/FinanceTest.php

@@ -165,13 +165,13 @@ class FinanceTest extends TestCase
         return [$user, $pirep, $fares];
     }
 
-    public function testFlightFaresNoOverride()
+    /*public function testFlightFaresNoOverride()
     {
         $flight = factory(Flight::class)->create();
         $fare = factory(Fare::class)->create();
 
         $this->fareSvc->setForFlight($flight, $fare);
-        $subfleet_fares = $this->fareSvc->getForFlight($flight);
+        $subfleet_fares = $this->fareSvc->get($flight);
 
         $this->assertCount(1, $subfleet_fares);
         $this->assertEquals($fare->price, $subfleet_fares->get(0)->price);
@@ -194,51 +194,14 @@ class FinanceTest extends TestCase
         // delete
         $this->fareSvc->delFareFromFlight($flight, $fare);
         $this->assertCount(0, $this->fareSvc->getForFlight($flight));
-    }
+    }*/
-
-    public function testFlightFaresSetToNull()
-    {
-        /** @var Flight $flight */
-        $flight = factory(Flight::class)->create();
-
-        /** @var Fare $fare */
-        $fare = factory(Fare::class)->create();
-
-        $this->fareSvc->setForFlight($flight, $fare);
-        $subfleet_fares = $this->fareSvc->getForFlight($flight);
-
-        $this->assertCount(1, $subfleet_fares);
-        $this->assertEquals($fare->price, $subfleet_fares->get(0)->price);
-        $this->assertEquals($fare->capacity, $subfleet_fares->get(0)->capacity);
-
-        //
-        // set an override now
-        //
-        $this->fareSvc->setForFlight($flight, $fare, [
-            'price' => 50, 'capacity' => 400,
-        ]);
-
-        // look for them again
-        $subfleet_fares = $this->fareSvc->getForFlight($flight);
-
-        $this->assertCount(1, $subfleet_fares);
-        $this->assertEquals(50, $subfleet_fares[0]->price);
-        $this->assertEquals(400, $subfleet_fares[0]->capacity);
-
-        // Set back to null
-        $this->fareSvc->setForFlight($flight, $fare, [
-            'price' => null, 'capacity' => null,
-        ]);
-
-        // Get the original and check that it's being used
-        $subfleet_fares = $this->fareSvc->getForFlight($flight);
-        $this->assertEquals($fare->price, $subfleet_fares->get(0)->price);
-        $this->assertEquals($fare->capacity, $subfleet_fares->get(0)->capacity);
-    }
 
     /**
      * Make sure that the API is returning the fares properly for a subfleet on a flight
      * https://github.com/nabeelio/phpvms/issues/899
+     *
+     * The fares, etc for a subfleet has to be adjusted to the fleet
+     * https://github.com/nabeelio/phpvms/issues/905
      */
     public function testFlightFaresOverAPI()
     {
@@ -254,29 +217,60 @@ class FinanceTest extends TestCase
         $subfleet = factory(Subfleet::class)->create();
         $this->fleetSvc->addSubfleetToFlight($subfleet, $flight);
 
+        /**
+         * Set a base fare
+         * Then override on multiple layers - subfleet modifies the cost, the flight modifies
+         * the price. This should then all be reflected as we go down the chain. This is
+         * mostly for the output side
+         */
         /** @var Fare $fare */
-        $fare = factory(Fare::class)->create();
+        $fare = factory(Fare::class)->create([
+            'price'    => 10,
+            'cost'     => 20,
+            'capacity' => 100,
+        ]);
 
-        $this->fareSvc->setForFlight($flight, $fare);
+        $this->fareSvc->setForSubfleet($subfleet, $fare, [
-        $flight_fares = $this->fareSvc->getForFlight($flight);
+            'capacity' => 200,
+        ]);
 
-        $this->assertCount(1, $flight_fares);
+        $this->fareSvc->setForFlight($flight, $fare, [
-        $this->assertEquals($fare->price, $flight_fares->get(0)->price);
+            'price' => 50,
-        $this->assertEquals($fare->capacity, $flight_fares->get(0)->capacity);
+        ]);
+
+        $flight = $this->fareSvc->getReconciledFaresForFlight($flight);
+
+        $this->assertEquals(50, $flight->subfleets[0]->fares[0]->price);
+        $this->assertEquals(200, $flight->subfleets[0]->fares[0]->capacity);
+        $this->assertEquals(20, $flight->subfleets[0]->fares[0]->cost);
 
         //
         // set an override now (but on the flight)
         //
-        $this->fareSvc->setForFlight($flight, $fare, ['price' => 50]);
 
         $req = $this->get('/api/flights/'.$flight->id);
         $req->assertStatus(200);
 
         $body = $req->json()['data'];
         $this->assertEquals($flight->id, $body['id']);
+
+        // Fares, etc, should be adjusted, per-subfleet
         $this->assertCount(1, $body['subfleets']);
-        $this->assertEquals(50, $body['fares'][0]['price']);
+        $this->assertEquals(50, $body['subfleets'][0]['fares'][0]['price']);
-        $this->assertEquals($fare->capacity, $body['fares'][0]['capacity']);
+        $this->assertEquals(200, $body['subfleets'][0]['fares'][0]['capacity']);
+        $this->assertEquals(20, $body['subfleets'][0]['fares'][0]['cost']);
+
+        $req = $this->get('/api/flights/search?flight_id='.$flight->id);
+        $req->assertStatus(200);
+
+        $body = $req->json()['data'][0];
+        $this->assertEquals($flight->id, $body['id']);
+
+        // Fares, etc, should be adjusted, per-subfleet
+        $this->assertCount(1, $body['subfleets']);
+        $this->assertEquals(50, $body['subfleets'][0]['fares'][0]['price']);
+        $this->assertEquals(200, $body['subfleets'][0]['fares'][0]['capacity']);
+        $this->assertEquals(20, $body['subfleets'][0]['fares'][0]['cost']);
     }
 
     public function testFlightFaresOverAPIOnUserBids()
@@ -300,11 +294,6 @@ class FinanceTest extends TestCase
         $fare = factory(Fare::class)->create();
 
         $this->fareSvc->setForFlight($flight, $fare);
-        $flight_fares = $this->fareSvc->getForFlight($flight);
-
-        $this->assertCount(1, $flight_fares);
-        $this->assertEquals($fare->price, $flight_fares->get(0)->price);
-        $this->assertEquals($fare->capacity, $flight_fares->get(0)->capacity);
 
         //
         // set an override now (but on the flight)
@@ -362,6 +351,10 @@ class FinanceTest extends TestCase
         /** @var \App\Models\Fare $fare */
         $fare = factory(Fare::class)->create();
 
+        // Subfleet needs to be attached to a flight
+        $subfleet = factory(Subfleet::class)->create();
+        $this->fleetSvc->addSubfleetToFlight($subfleet, $flight);
+
         $percent_incr = '20%';
         $percent_decr = '-20%';
         $percent_200 = '200%';
@@ -376,7 +369,8 @@ class FinanceTest extends TestCase
             'capacity' => $percent_200,
         ]);
 
-        $ac_fares = $this->fareSvc->getAllFares($flight, null);
+        // A subfleet is required to be passed in
+        $ac_fares = $this->fareSvc->getAllFares($flight, $subfleet);
 
         $this->assertCount(1, $ac_fares);
         $this->assertEquals($new_price, $ac_fares[0]->price);

tests/ImporterTest.php

@@ -476,6 +476,7 @@ class ImporterTest extends TestCase
         $this->assertCount(1, $status['errors']);
 
         // See if it imported
+        /** @var Flight $flight */
         $flight = Flight::where([
             'airline_id'    => $airline->id,
             'flight_number' => '1972',
@@ -514,7 +515,7 @@ class ImporterTest extends TestCase
         $this->assertEquals('C41', $dep_gate['value']);
 
         // Check the fare class
-        $fares = $this->fareSvc->getForFlight($flight);
+        $fares = $this->fareSvc->getFareWithOverrides(null, $flight->fares);
         $this->assertCount(3, $fares);
 
         $first = $fares->where('code', 'Y')->first();