From 0ce86e9e4464f5c291753966918e132302662388 Mon Sep 17 00:00:00 2001 From: Nabeel Shahzad Date: Tue, 24 Jul 2018 14:50:50 -0500 Subject: [PATCH] Rewrite add/remove bids code w additional tests --- app/Http/Controllers/Api/UserController.php | 2 +- app/Interfaces/Controller.php | 22 ----- app/Models/Bid.php | 4 + app/Models/Flight.php | 3 +- app/Models/User.php | 21 ++--- app/Services/FlightService.php | 91 ++++++++++++--------- tests/FlightTest.php | 27 ++++-- 7 files changed, 92 insertions(+), 78 deletions(-) diff --git a/app/Http/Controllers/Api/UserController.php b/app/Http/Controllers/Api/UserController.php index 9c39dac7..5ba6bd2a 100644 --- a/app/Http/Controllers/Api/UserController.php +++ b/app/Http/Controllers/Api/UserController.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers\Api; +use App\Exceptions\BidExists; use App\Http\Resources\Bid as BidResource; use App\Http\Resources\Pirep as PirepResource; use App\Http\Resources\Subfleet as SubfleetResource; @@ -98,7 +99,6 @@ class UserController extends Controller * @return mixed * @throws \Illuminate\Database\Eloquent\ModelNotFoundException * @throws \App\Exceptions\BidExists - * @throws \App\Services\Exception */ public function bids(Request $request) { diff --git a/app/Interfaces/Controller.php b/app/Interfaces/Controller.php index 5967c4c7..76d66942 100755 --- a/app/Interfaces/Controller.php +++ b/app/Interfaces/Controller.php @@ -71,28 +71,6 @@ abstract class Controller extends \Illuminate\Routing\Controller return $fields; } - /** - * Run a validation - * @param $request - * @param $rules - * @return bool - * @throws \Symfony\Component\HttpKernel\Exception\BadRequestHttpException - */ - /*public function validate($request, $rules) - { - if ($request instanceof Request) { - $validator = Validator::make($request->all(), $rules); - } else { - $validator = Validator::make($request, $rules); - } - - if (!$validator->passes()) { - throw new BadRequestHttpException($validator->errors(), null, 400); - } - - return true; - }*/ - /** * Simple normalized method for forming the JSON responses * @param $message diff --git a/app/Models/Bid.php b/app/Models/Bid.php index 65b79980..543f7568 100644 --- a/app/Models/Bid.php +++ b/app/Models/Bid.php @@ -16,6 +16,10 @@ class Bid extends Model 'flight_id', ]; + protected $casts = [ + 'user_id' => 'integer', + ]; + /** * Relationships */ diff --git a/app/Models/Flight.php b/app/Models/Flight.php index 46bcaa3b..29a7b1a4 100644 --- a/app/Models/Flight.php +++ b/app/Models/Flight.php @@ -17,7 +17,8 @@ use PhpUnitsOfMeasure\Exception\NonStringUnitName; * @property integer airline_id * @property mixed flight_number * @property mixed route_code - * @property int route_leg + * @property int route_leg + * @property bool has_bid * @property Collection field_values * @property Collection fares * @property Collection subfleets diff --git a/app/Models/User.php b/app/Models/User.php index 6e4484f5..ea91ad72 100755 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -10,18 +10,19 @@ use Illuminate\Notifications\Notifiable; use Laratrust\Traits\LaratrustUserTrait; /** - * @property integer $id - * @property string $name - * @property string $email - * @property string $password - * @property string $api_key + * @property integer id + * @property string name + * @property string email + * @property string password + * @property string api_key + * @property mixed ident * @property string curr_airport_id * @property string home_airport_id - * @property Flight[] $flights - * @property string $flight_time - * @property string $remember_token - * @property \Carbon\Carbon $created_at - * @property \Carbon\Carbon $updated_at + * @property Flight[] flights + * @property string flight_time + * @property string remember_token + * @property \Carbon\Carbon created_at + * @property \Carbon\Carbon updated_at * @property Rank rank * @property Journal journal * @property string pilot_id diff --git a/app/Services/FlightService.php b/app/Services/FlightService.php index 9dbcfbef..247e394e 100644 --- a/app/Services/FlightService.php +++ b/app/Services/FlightService.php @@ -19,9 +19,9 @@ use Log; class FlightService extends Service { private $fareSvc, - $flightRepo, - $navDataRepo, - $userSvc; + $flightRepo, + $navDataRepo, + $userSvc; /** * FlightService constructor. @@ -35,7 +35,8 @@ class FlightService extends Service FlightRepository $flightRepo, NavdataRepository $navdataRepo, UserService $userSvc - ) { + ) + { $this->fareSvc = $fareSvc; $this->flightRepo = $flightRepo; $this->navDataRepo = $navdataRepo; @@ -114,15 +115,15 @@ class FlightService extends Service ]; $found_flights = $this->flightRepo->findWhere($where); - if($found_flights->count() === 0) { + if ($found_flights->count() === 0) { return false; } // Find within all the flights with the same flight number // Return any flights that have the same route code and leg // If this list is > 0, then this has a duplicate - $found_flights = $found_flights->filter(function($value, $key) use ($flight) { - if($flight->route_code === $value->route_code + $found_flights = $found_flights->filter(function ($value, $key) use ($flight) { + if ($flight->route_code === $value->route_code && $flight->route_leg === $value->route_leg) { return true; } @@ -130,7 +131,7 @@ class FlightService extends Service return false; }); - if($found_flights->count() === 0) { + if ($found_flights->count() === 0) { return false; } @@ -160,10 +161,10 @@ class FlightService extends Service FlightFieldValue::updateOrCreate( [ 'flight_id' => $flight->id, - 'name' => $fv['name'], + 'name' => $fv['name'], ], [ - 'value' => $fv['value'] + 'value' => $fv['value'] ] ); } @@ -199,43 +200,53 @@ class FlightService extends Service * Allow a user to bid on a flight. Check settings and all that good stuff * @param Flight $flight * @param User $user - * @return Bid|null + * @return mixed * @throws \App\Exceptions\BidExists */ public function addBid(Flight $flight, User $user) { - # If it's already been bid on, then it can't be bid on again - if ($flight->has_bid && setting('bids.disable_flight_on_bid')) { - Log::info($flight->id.' already has a bid, skipping'); - throw new BidExists(); + # Get all of the bids for this user. See if they're allowed to have multiple + # bids + $bids = Bid::where('user_id', $user->id)->get(); + if ($bids->count() > 0 && setting('bids.allow_multiple_bids') === false) { + throw new BidExists('User "'.$user->id.'" already has bids, skipping'); } - # See if we're allowed to have multiple bids or not - if (!setting('bids.allow_multiple_bids')) { - $user_bids = Bid::where(['user_id' => $user->id])->first(); - if ($user_bids) { - Log::info('User "'.$user->id.'" already has bids, skipping'); - throw new BidExists(); + # Get all of the bids for this flight + $bids = Bid::where('flight_id', $flight->id)->get(); + if ($bids->count() > 0) { + # Does the flight have a bid set? + if ($flight->has_bid === false) { + $flight->has_bid = true; + $flight->save(); + } + + # Check all the bids for one of this user + foreach ($bids as $bid) { + if ($bid->user_id === $user->id) { + Log::info('Bid exists, user='.$user->ident.', flight='.$flight->id); + return $bid; + } + } + + if (setting('bids.allow_multiple_bids') === false) { + throw new BidExists('A bid already exists for this flight'); + } + } else { + if ($flight->has_bid === true) { + Log::info('Bid exists, flight='.$flight->id.'; no entry in bids table, cleaning up'); } } - # See if this user has this flight bid on already - $bid_data = [ + $bid = Bid::firstOrCreate([ 'user_id' => $user->id, - 'flight_id' => $flight->id - ]; - - $user_bid = Bid::where($bid_data)->first(); - if ($user_bid) { - return $user_bid; - } - - $user_bid = Bid::create($bid_data); + 'flight_id' => $flight->id, + ]); $flight->has_bid = true; $flight->save(); - return $user_bid; + return $bid; } /** @@ -245,16 +256,18 @@ class FlightService extends Service */ public function removeBid(Flight $flight, User $user) { - $user_bid = Bid::where([ - 'flight_id' => $flight->id, 'user_id' => $user->id - ])->first(); + $bids = Bid::where([ + 'flight_id' => $flight->id, + 'user_id' => $user->id + ])->get(); - if ($user_bid) { - $user_bid->forceDelete(); + foreach ($bids as $bid) { + $bid->forceDelete(); } # Only flip the flag if there are no bids left for this flight - if (!Bid::where('flight_id', $flight->id)->exists()) { + $bids = Bid::where('flight_id', $flight->id)->get(); + if ($bids->count() === 0) { $flight->has_bid = false; $flight->save(); } diff --git a/tests/FlightTest.php b/tests/FlightTest.php index 392574bf..1c53b90d 100644 --- a/tests/FlightTest.php +++ b/tests/FlightTest.php @@ -373,7 +373,11 @@ class FlightTest extends TestCase */ public function testBids() { + $this->settingsRepo->store('bids.allow_multiple_bids', true); + $this->settingsRepo->store('bids.disable_flight_on_bid', false); + $user = factory(User::class)->create(); + $user2 = factory(User::class)->create(); $headers = $this->headers($user); $flight = $this->addFlight($user); @@ -387,12 +391,13 @@ class FlightTest extends TestCase $flight = Flight::find($flight->id); $this->assertTrue($flight->has_bid); - # Check the table and make sure thee entry is there - $this->expectException(\App\Exceptions\BidExists::class); - $this->flightSvc->addBid($flight, $user); + # Check the table and make sure the entry is there + $bid_retrieved = $this->flightSvc->addBid($flight, $user); + $this->assertEquals($bid->id, $bid_retrieved->id); $user->refresh(); - $this->assertEquals(1, $user->bids->count()); + $bids = $user->bids; + $this->assertEquals(1, $bids->count()); # Query the API and see that the user has the bids # And pull the flight details for the user/bids @@ -407,13 +412,25 @@ class FlightTest extends TestCase $body = $req->json()['data']; $req->assertStatus(200); - $this->assertEquals($flight->id, $body[0]['id']); + $this->assertEquals($flight->id, $body[0]['flight_id']); + + # have a second user bid on it + $bid_user2 = $this->flightSvc->addBid($flight, $user2); + $this->assertNotNull($bid_user2); + $this->assertNotEquals($bid_retrieved->id, $bid_user2->id); # Now remove the flight and check API $this->flightSvc->removeBid($flight, $user); $flight = Flight::find($flight->id); + + # user2 still has a bid on it + $this->assertTrue($flight->has_bid); + + # Remove it from 2nd user + $this->flightSvc->removeBid($flight, $user2); + $flight->refresh(); $this->assertFalse($flight->has_bid); $user->refresh();