From 59900317b0a97675ef3ef9f63d311056c8d1e40f Mon Sep 17 00:00:00 2001 From: Kris Date: Thu, 23 Nov 2023 20:34:47 +0100 Subject: [PATCH] :sparkles: create api endpoint for creating trips manually [closed-beta] (#2145) --- .../Controllers/API/v1/TripController.php | 68 +++++++++++++++ .../Backend/Transport/ManualTripCreator.php | 79 +++++++++++++++++ .../Frontend/Admin/TripController.php | 69 +++++---------- app/Models/HafasOperator.php | 3 + app/Models/HafasTrip.php | 36 ++++++-- app/Models/Status.php | 11 ++- app/Models/TrainCheckin.php | 4 + app/Models/TrainStation.php | 1 + app/Models/TrainStopover.php | 6 +- app/Models/User.php | 6 ++ ...fix_departure_column_on_train_checkins.php | 23 +++++ ...1_23_000002_add_user_id_to_hafas_trips.php | 31 +++++++ routes/api.php | 6 +- routes/web/admin.php | 2 +- .../Transport/ManualTripCreatorTest.php | 87 +++++++++++++++++++ 15 files changed, 370 insertions(+), 62 deletions(-) create mode 100644 app/Http/Controllers/API/v1/TripController.php create mode 100644 app/Http/Controllers/Backend/Transport/ManualTripCreator.php create mode 100644 database/migrations/2023_11_23_000001_fix_departure_column_on_train_checkins.php create mode 100644 database/migrations/2023_11_23_000002_add_user_id_to_hafas_trips.php create mode 100644 tests/Feature/Transport/ManualTripCreatorTest.php diff --git a/app/Http/Controllers/API/v1/TripController.php b/app/Http/Controllers/API/v1/TripController.php new file mode 100644 index 000000000..118b50e4f --- /dev/null +++ b/app/Http/Controllers/API/v1/TripController.php @@ -0,0 +1,68 @@ + later solve the problem for non-existing stations + */ + public function createTrip(Request $request): HafasTripResource { + if (!auth()->user()?->hasRole('closed-beta')) { + abort(403, 'this endpoint is currently only available for beta users'); + } + + $validated = $request->validate([ + 'category' => ['required', new Enum(HafasTravelType::class)], + 'lineName' => ['required'], + 'journey_number' => ['nullable', 'numeric', 'min:1'], + 'operator_id' => ['nullable', 'numeric', 'exists:hafas_operators,id'], + 'originId' => ['required', 'exists:train_stations,ibnr'], + 'originDeparturePlanned' => ['required', 'date'], + 'destinationId' => ['required', 'exists:train_stations,ibnr'], + 'destinationArrivalPlanned' => ['required', 'date'], + ]); + + DB::beginTransaction(); + + $creator = new ManualTripCreator(); + + $creator->category = HafasTravelType::from($validated['category']); + $creator->lineName = $validated['lineName']; + $creator->journeyNumber = $validated['journey_number']; + $creator->operator = HafasOperator::find($validated['operator_id']); + $creator->origin = TrainStation::where('ibnr', $validated['originId'])->firstOrFail(); + $creator->originDeparturePlanned = Carbon::parse($validated['originDeparturePlanned']); + $creator->destination = TrainStation::where('ibnr', $validated['destinationId'])->firstOrFail(); + $creator->destinationArrivalPlanned = Carbon::parse($validated['destinationArrivalPlanned']); + + $trip = $creator->createTrip(); + $creator->createOriginStopover(); + $creator->createDestinationStopover(); + + DB::commit(); + + return new HafasTripResource($trip); + } +} diff --git a/app/Http/Controllers/Backend/Transport/ManualTripCreator.php b/app/Http/Controllers/Backend/Transport/ManualTripCreator.php new file mode 100644 index 000000000..e7fcb00d1 --- /dev/null +++ b/app/Http/Controllers/Backend/Transport/ManualTripCreator.php @@ -0,0 +1,79 @@ +trip = HafasTrip::create([ + 'trip_id' => TripBackend::generateUniqueTripId(), + 'category' => $this->category, + 'number' => $this->lineName, + 'linename' => $this->lineName, + 'journey_number' => $this->journeyNumber, + 'operator_id' => $this->operator->id ?? null, + 'origin' => $this->origin->ibnr, + 'destination' => $this->destination->ibnr, + 'departure' => $this->originDeparturePlanned, + 'arrival' => $this->destinationArrivalPlanned, + 'source' => TripSource::USER, + 'user_id' => auth()->user()?->id ?? null, + ]); + return $this->trip; + } + + public function createOriginStopover(): TrainStopover { + if ($this->trip === null) { + throw new \InvalidArgumentException('Cannot create stopover without trip'); + } + return TrainStopover::create([ + 'trip_id' => $this->trip->trip_id, + 'train_station_id' => $this->origin->id, + 'arrival_planned' => $this->originDeparturePlanned, + 'departure_planned' => $this->originDeparturePlanned, + ]); + } + + public function createDestinationStopover(): TrainStopover { + if ($this->trip === null) { + throw new \InvalidArgumentException('Cannot create stopover without trip'); + } + return TrainStopover::create([ + 'trip_id' => $this->trip->trip_id, + 'train_station_id' => $this->destination->id, + 'arrival_planned' => $this->destinationArrivalPlanned, + 'departure_planned' => $this->destinationArrivalPlanned, + ]); + } + + public static function generateUniqueTripId(): string { + $tripId = Str::uuid(); + while (HafasTrip::where('trip_id', $tripId)->exists()) { + return self::generateUniqueTripId(); + } + return $tripId; + } +} diff --git a/app/Http/Controllers/Frontend/Admin/TripController.php b/app/Http/Controllers/Frontend/Admin/TripController.php index 070ec15b6..9978df404 100644 --- a/app/Http/Controllers/Frontend/Admin/TripController.php +++ b/app/Http/Controllers/Frontend/Admin/TripController.php @@ -3,9 +3,13 @@ namespace App\Http\Controllers\Frontend\Admin; use App\Enum\HafasTravelType; +use App\Http\Controllers\Backend\Transport\ManualTripCreator; +use App\Http\Controllers\Backend\Transport\ManualTripCreator as TripBackend; +use App\Models\HafasOperator; use App\Models\HafasTrip; use App\Models\TrainStation; use App\Models\TrainStopover; +use Carbon\Carbon; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Validation\Rules\Enum; @@ -23,19 +27,6 @@ public function renderTrip(string $id): View { ]); } - /** - * This form is currently for testing purposes only. - * Admins can create a trip with manually entered data. - * Users can check in to this trip. - * It should be tested if the trip is created correctly and all data required for the trip is present, - * so no (500) errors occur. - * - * @return View - */ - public function renderForm(): View { - return view('admin.trip.create'); - } - public function createTrip(Request $request): RedirectResponse { $validated = $request->validate([ 'origin' => ['required', 'numeric', 'exists:train_stations,ibnr'], @@ -49,49 +40,29 @@ public function createTrip(Request $request): RedirectResponse { 'journey_number' => ['required', 'numeric'], ]); - $departure = str_contains($validated['departure'], '+') && str_contains($validated['departure'], '-') - ? $validated['departure'] : $validated['departure'] . '+00:00'; - $arrival = str_contains($validated['arrival'], '+') && str_contains($validated['arrival'], '-') - ? $validated['arrival'] : $validated['arrival'] . '+00:00'; + $creator = new ManualTripCreator(); - $originStation = TrainStation::where('ibnr', $validated['origin'])->firstOrFail(); - $destinationStation = TrainStation::where('ibnr', $validated['destination'])->firstOrFail(); + $creator->category = HafasTravelType::from($validated['category']); + $creator->lineName = $validated['linename']; + $creator->journeyNumber = $validated['journey_number']; + $creator->operator = HafasOperator::find($validated['operator_id']); + $creator->origin = TrainStation::where('ibnr', $validated['origin'])->firstOrFail(); + $creator->originDeparturePlanned = Carbon::parse(str_contains($validated['departure'], '+') && str_contains($validated['departure'], '-') + ? $validated['departure'] : $validated['departure'] . '+00:00'); + $creator->destination = TrainStation::where('ibnr', $validated['destination'])->firstOrFail(); + $creator->destinationArrivalPlanned = Carbon::parse(str_contains($validated['arrival'], '+') && str_contains($validated['arrival'], '-') + ? $validated['arrival'] : $validated['arrival'] . '+00:00'); - $trip = HafasTrip::create([ - 'trip_id' => strtr('manual-userId-uuid', [ - 'userId' => auth()->id(), - 'uuid' => uniqid('', true), - ]), - 'category' => $validated['category'], - 'number' => $validated['number'], - 'linename' => $validated['linename'], - 'journey_number' => $validated['journey_number'], - 'operator_id' => $validated['operator_id'], - 'origin' => $validated['origin'], - 'destination' => $validated['destination'], - 'departure' => $departure, - 'arrival' => $arrival, - ]); - //Origin stopover - TrainStopover::create([ - 'trip_id' => $trip->trip_id, - 'train_station_id' => $originStation->id, - 'arrival_planned' => $departure, - 'departure_planned' => $departure, - ]); - //Destination stopover - TrainStopover::create([ - 'trip_id' => $trip->trip_id, - 'train_station_id' => $destinationStation->id, - 'arrival_planned' => $arrival, - 'departure_planned' => $arrival, - ]); + $trip = $creator->createTrip(); + $creator->createOriginStopover(); + $creator->createDestinationStopover(); + $trip->refresh(); return redirect()->route('trains.trip', [ 'tripID' => $trip->trip_id, 'lineName' => $trip->linename, 'start' => $trip->origin, - 'departure' => $departure, + 'departure' => $trip->departure->toIso8601String(), ]); } } diff --git a/app/Models/HafasOperator.php b/app/Models/HafasOperator.php index 992efea2f..92c587bd9 100644 --- a/app/Models/HafasOperator.php +++ b/app/Models/HafasOperator.php @@ -6,6 +6,9 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\HasMany; +/** + * @todo rename table only to "Operator" (or "TransportOperator", ..., but not HAFAS) + */ class HafasOperator extends Model { use HasFactory; diff --git a/app/Models/HafasTrip.php b/app/Models/HafasTrip.php index 45d2f71f6..f1d0a164b 100644 --- a/app/Models/HafasTrip.php +++ b/app/Models/HafasTrip.php @@ -12,10 +12,28 @@ use Illuminate\Database\Eloquent\Relations\HasOne; /** - * @property $stopovers - * @property PolyLine $polyLine - * @property PolyLine $polyline - * @property $linename + * @property int $id + * @property string $trip_id + * @property HafasTravelType $category + * @property string $number + * @property string $linename + * @property string $journey_number + * @property int $operator_id + * @property int $origin + * @property int $destination + * @property int $polyline_id + * @property UTCDateTime $departure + * @property UTCDateTime $arrival + * @property UTCDateTime $last_refreshed + * @property TripSource $source + * @property int $user_id + * @property $stopovers + * @property PolyLine $polyLine + * + * @todo rename table only to "Trip" (without Hafas) + * @todo rename "linename" to "line_name" (or something else, but not "linename") + * @todo migrate origin & destination to use "id" instead of "ibnr" and rename to "origin_id" & "destination_id" + * @todo is "delay" still needed? We save planned and real in the stopovers. check. */ class HafasTrip extends Model { @@ -24,7 +42,7 @@ class HafasTrip extends Model protected $fillable = [ 'trip_id', 'category', 'number', 'linename', 'journey_number', 'operator_id', 'origin', 'destination', - 'polyline_id', 'departure', 'arrival', 'delay', 'source', 'last_refreshed', + 'polyline_id', 'departure', 'arrival', 'delay', 'source', 'user_id', 'last_refreshed', ]; protected $hidden = ['created_at', 'updated_at']; protected $casts = [ @@ -40,6 +58,7 @@ class HafasTrip extends Model 'arrival' => UTCDateTime::class, 'last_refreshed' => 'datetime', 'source' => TripSource::class, + 'user_id' => 'integer', ]; public function polyline(): HasOne { @@ -67,4 +86,11 @@ public function stopovers(): HasMany { public function checkins(): HasMany { return $this->hasMany(TrainCheckin::class, 'trip_id', 'trip_id'); } + + /** + * If this trip was created by a user, this model belongs to the user, so they can edit and delete it. + */ + public function user(): BelongsTo { + return $this->belongsTo(User::class, 'user_id', 'id'); + } } diff --git a/app/Models/Status.php b/app/Models/Status.php index ec6d22088..9b2ee2a07 100644 --- a/app/Models/Status.php +++ b/app/Models/Status.php @@ -19,16 +19,19 @@ * @property int event_id * @property StatusVisibility visibility * @property TrainCheckin $trainCheckin + * + * @todo merge model with "TrainCheckin" (later only "Checkin") because the difference between trip sources (HAFAS, + * User, and future sources) should be handled in the Trip model. */ class Status extends Model { use HasFactory; - protected $fillable = ['user_id', 'body', 'business', 'visibility', 'event_id', 'tweet_id', 'mastodon_post_id']; - protected $hidden = ['user_id', 'business']; - protected $appends = ['favorited', 'socialText', 'statusInvisibleToMe', 'description']; - protected $casts = [ + protected $fillable = ['user_id', 'body', 'business', 'visibility', 'event_id', 'tweet_id', 'mastodon_post_id']; + protected $hidden = ['user_id', 'business']; + protected $appends = ['favorited', 'socialText', 'statusInvisibleToMe', 'description']; + protected $casts = [ 'id' => 'integer', 'user_id' => 'integer', 'business' => Business::class, diff --git a/app/Models/TrainCheckin.php b/app/Models/TrainCheckin.php index 6e6422d9c..0d66dedc7 100644 --- a/app/Models/TrainCheckin.php +++ b/app/Models/TrainCheckin.php @@ -22,6 +22,10 @@ * @property TrainStopover $destination_stopover * @property TrainStation $originStation * @property TrainStation $destinationStation + * + * @todo rename table to "Checkin" (without Train - we have more than just trains) + * @todo merge model with "Status" because the difference between trip sources (HAFAS, + * User, and future sources) should be handled in the Trip model. */ class TrainCheckin extends Model { diff --git a/app/Models/TrainStation.php b/app/Models/TrainStation.php index 219a7feec..13bb2b674 100644 --- a/app/Models/TrainStation.php +++ b/app/Models/TrainStation.php @@ -19,6 +19,7 @@ * @property Event[] $events * @property Carbon $created_at * @property Carbon $updated_at + * @todo rename table to "Station" (without Train - we have more than just trains) */ class TrainStation extends Model { diff --git a/app/Models/TrainStopover.php b/app/Models/TrainStopover.php index 1cb07a5ab..ebdf2f424 100644 --- a/app/Models/TrainStopover.php +++ b/app/Models/TrainStopover.php @@ -7,8 +7,12 @@ use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; -use Illuminate\Database\Eloquent\Relations\HasMany; +/** + * @todo rename table to "Stopover" (without Train - we have more than just trains) + * @todo rename "train_station_id" to "station_id" - we have more than just trains. + * @todo rename "cancelled" to "is_cancelled" - or split into "is_arrival_cancelled" and "is_departure_cancelled"? need to think about this. + */ class TrainStopover extends Model { use HasFactory; diff --git a/app/Models/User.php b/app/Models/User.php index 5adc1e7d4..335edd928 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -42,6 +42,12 @@ * @property string language * @property Carbon last_login * @property Status[] $statuses + * + * @todo replace "role" with an explicit permission system - e.g. spatie/laravel-permission + * @todo replace "experimental" also with an explicit permission system - user can add self to "experimental" group + * @todo rename home_id to home_station_id + * @todo rename mapprovider to map_provider + * @todo remove "twitterUrl" (Twitter isn't used by traewelling anymore) */ class User extends Authenticatable implements MustVerifyEmail { diff --git a/database/migrations/2023_11_23_000001_fix_departure_column_on_train_checkins.php b/database/migrations/2023_11_23_000001_fix_departure_column_on_train_checkins.php new file mode 100644 index 000000000..7b16fe1b0 --- /dev/null +++ b/database/migrations/2023_11_23_000001_fix_departure_column_on_train_checkins.php @@ -0,0 +1,23 @@ +dateTime('departure')->nullable()->default(null)->change(); + $table->dateTime('arrival')->nullable()->default(null)->change(); + }); + } + + public function down(): void { + + Schema::table('train_checkins', static function(Blueprint $table) { + $table->dateTime('departure')->change(); + $table->dateTime('arrival')->change(); + }); + } +}; diff --git a/database/migrations/2023_11_23_000002_add_user_id_to_hafas_trips.php b/database/migrations/2023_11_23_000002_add_user_id_to_hafas_trips.php new file mode 100644 index 000000000..df1495a42 --- /dev/null +++ b/database/migrations/2023_11_23_000002_add_user_id_to_hafas_trips.php @@ -0,0 +1,31 @@ +unsignedBigInteger('user_id') + ->nullable() + ->default(null) + ->comment('if not null, this trip belongs to the user (e.g. manually created trips)') + ->after('source'); + + $table->foreign('user_id') + ->references('id') + ->on('users') + ->onDelete('set null'); + //only set to null, because the trip can also be used by other users. + //if the user is deleted and no other checkins are using this trip, the trip will be deleted automatically by our nightly cleanup job + }); + } + + public function down(): void { + Schema::table('hafas_trips', function(Blueprint $table) { + $table->dropColumn('user_id'); + }); + } +}; diff --git a/routes/api.php b/routes/api.php index 3e2372e67..35ed2665c 100644 --- a/routes/api.php +++ b/routes/api.php @@ -26,6 +26,7 @@ use App\Http\Controllers\API\v1\SupportController; use App\Http\Controllers\API\v1\TokenController; use App\Http\Controllers\API\v1\TransportController; +use App\Http\Controllers\API\v1\TripController; use App\Http\Controllers\API\v1\UserController; use App\Http\Controllers\API\v1\WebhookController; use Illuminate\Support\Facades\Route; @@ -74,8 +75,9 @@ Route::put('unread/{id}', [NotificationsController::class, 'markAsUnread']); }); }); - Route::group(['prefix' => 'trains', 'middleware' => ['scope:write-statuses']], static function() { - Route::get('trip/', [TransportController::class, 'getTrip']); + Route::group(['prefix' => 'trains', 'middleware' => ['scope:write-statuses']], static function() { //TODO: rename from "trains" -> we have more then trains... + Route::get('trip', [TransportController::class, 'getTrip']); + Route::post('trip', [TripController::class, 'createTrip']); Route::post('checkin', [TransportController::class, 'create']); Route::group(['prefix' => 'station'], static function() { Route::get('{name}/departures', [TransportController::class, 'departures']); diff --git a/routes/web/admin.php b/routes/web/admin.php index aa2df7950..0fe7f26d1 100644 --- a/routes/web/admin.php +++ b/routes/web/admin.php @@ -45,7 +45,7 @@ }); Route::prefix('trip')->group(function() { - Route::get('/create', [TripController::class, 'renderForm']) + Route::view('/create', 'admin.trip.create') ->name('admin.trip.create'); Route::post('/create', [TripController::class, 'createTrip']); diff --git a/tests/Feature/Transport/ManualTripCreatorTest.php b/tests/Feature/Transport/ManualTripCreatorTest.php new file mode 100644 index 000000000..abeae1583 --- /dev/null +++ b/tests/Feature/Transport/ManualTripCreatorTest.php @@ -0,0 +1,87 @@ +create(); + $destinationStation = TrainStation::factory()->create(); + $departure = Carbon::now()->addMinutes(5)->setSecond(0)->setMicrosecond(0); + $arrival = Carbon::now()->addMinutes(15)->setSecond(0)->setMicrosecond(0); + + $creator = new ManualTripCreator(); + + $creator->category = HafasTravelType::REGIONAL; + $creator->lineName = 'S1'; + $creator->journeyNumber = 85001; + $creator->operator = HafasOperator::factory()->create(); + $creator->origin = $originStation; + $creator->originDeparturePlanned = $departure; + $creator->destination = $destinationStation; + $creator->destinationArrivalPlanned = $arrival; + + $trip = $creator->createTrip(); + $creator->createOriginStopover(); + $creator->createDestinationStopover(); + $trip->refresh(); + + $this->assertEquals($trip->source, TripSource::USER); + + $this->assertDatabaseHas('hafas_trips', [ + 'trip_id' => $trip->trip_id, + 'category' => $trip->category, + 'number' => $trip->number, + 'linename' => $trip->linename, + 'journey_number' => $trip->journey_number, + 'operator_id' => $trip->operator_id, + 'origin' => $trip->origin, + 'destination' => $trip->destination, + 'departure' => $trip->departure, + 'arrival' => $trip->arrival, + 'source' => $trip->source, + ]); + $this->assertDatabaseHas('train_stopovers', [ + 'trip_id' => $trip->trip_id, + 'train_station_id' => $originStation->id, + 'arrival_planned' => $departure, + 'departure_planned' => $departure, + ]); + $this->assertDatabaseHas('train_stopovers', [ + 'trip_id' => $trip->trip_id, + 'train_station_id' => $destinationStation->id, + 'arrival_planned' => $arrival, + 'departure_planned' => $arrival, + ]); + + /**** Checkin ****/ + + $checkin = TrainCheckinController::checkin( + user: User::factory()->create(), + hafasTrip: $trip, + origin: $originStation, + departure: $departure, + destination: $destinationStation, + arrival: $arrival + ); + + $this->assertDatabaseHas('train_checkins', [ + 'trip_id' => $trip->trip_id, + 'user_id' => $checkin['status']->trainCheckin->user_id, + ]); + } +}