From dc7efc981cb890165776e46795b98b3a49b779fd Mon Sep 17 00:00:00 2001 From: Nabeel S Date: Tue, 27 Oct 2020 18:46:15 -0400 Subject: [PATCH] Refactor fares inheritance #905 (#906) --- app/Http/Controllers/Api/FlightController.php | 14 +- app/Http/Resources/Fare.php | 17 +- app/Http/Resources/Flight.php | 1 - app/Models/Flight.php | 4 + app/Services/BidService.php | 9 +- app/Services/FareService.php | 150 ++++++++++++------ tests/FinanceTest.php | 110 ++++++------- tests/ImporterTest.php | 3 +- 8 files changed, 187 insertions(+), 121 deletions(-) diff --git a/app/Http/Controllers/Api/FlightController.php b/app/Http/Controllers/Api/FlightController.php index 5e1928d9..5ca351cb 100644 --- a/app/Http/Controllers/Api/FlightController.php +++ b/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); diff --git a/app/Http/Resources/Fare.php b/app/Http/Resources/Fare.php index 8189eaf6..e89a5ab9 100644 --- a/app/Http/Resources/Fare.php +++ b/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, - 'code' => $fare->code, - 'name' => $fare->name, - 'capacity' => $fare->capacity, - 'cost' => $fare->cost, - 'price' => $fare->price, + 'id' => $this->id, + 'code' => $this->code, + 'name' => $this->name, + 'capacity' => $this->capacity, + 'cost' => $this->cost, + 'price' => $this->price, 'type' => $this->type, 'notes' => $this->notes, 'active' => $this->active, diff --git a/app/Http/Resources/Flight.php b/app/Http/Resources/Flight.php index 9a9d5752..06d17c6e 100644 --- a/app/Http/Resources/Flight.php +++ b/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 diff --git a/app/Models/Flight.php b/app/Models/Flight.php index 76cdb4f2..94371226 100644 --- a/app/Models/Flight.php +++ b/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 diff --git a/app/Services/BidService.php b/app/Services/BidService.php index f30cd620..acd88e51 100644 --- a/app/Services/BidService.php +++ b/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; diff --git a/app/Services/FareService.php b/app/Services/FareService.php index 4189dcd6..8647c5c5 100644 --- a/app/Services/FareService.php +++ b/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 - * This will go through if there are any fares assigned to the flight, - * and then check the fares assigned on the subfleet, and give the - * final "authoritative" list of the fares for a flight. + * @param Collection[Fare] $subfleet_fares The fare for a subfleet + * @param Collection[Fare] $flight_fares The fares on 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); - 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; + return $this->getFareWithOverrides($subfleet->fares, $flight_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 diff --git a/tests/FinanceTest.php b/tests/FinanceTest.php index 5904e81f..65c278fd 100644 --- a/tests/FinanceTest.php +++ b/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); - $flight_fares = $this->fareSvc->getForFlight($flight); + $this->fareSvc->setForSubfleet($subfleet, $fare, [ + 'capacity' => 200, + ]); - $this->assertCount(1, $flight_fares); - $this->assertEquals($fare->price, $flight_fares->get(0)->price); - $this->assertEquals($fare->capacity, $flight_fares->get(0)->capacity); + $this->fareSvc->setForFlight($flight, $fare, [ + 'price' => 50, + ]); + + $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($fare->capacity, $body['fares'][0]['capacity']); + $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']); + + $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); diff --git a/tests/ImporterTest.php b/tests/ImporterTest.php index ffd3524f..eaa2a10f 100644 --- a/tests/ImporterTest.php +++ b/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();