From e31071e9b55c6aa2ed0b486c071a830223fec108 Mon Sep 17 00:00:00 2001 From: Kris Date: Wed, 20 Dec 2023 19:25:55 +0100 Subject: [PATCH] :card_file_box: prepare checkins table to use relation instead of duplicate data (#2222) --- app/Console/Commands/MigrateStopovers.php | 41 ++++++ .../Transport/TrainCheckinController.php | 30 ++-- app/Models/TrainCheckin.php | 131 +++++++++++++----- database/factories/TrainCheckinFactory.php | 24 ++-- ...ion_stopover_foreign_to_train_checkins.php | 37 +++++ tests/Feature/Webhooks/WebhookStatusTest.php | 2 +- 6 files changed, 203 insertions(+), 62 deletions(-) create mode 100644 app/Console/Commands/MigrateStopovers.php create mode 100644 database/migrations/2023_12_18_000000_add_origin_and_destination_stopover_foreign_to_train_checkins.php diff --git a/app/Console/Commands/MigrateStopovers.php b/app/Console/Commands/MigrateStopovers.php new file mode 100644 index 000000000..f40754556 --- /dev/null +++ b/app/Console/Commands/MigrateStopovers.php @@ -0,0 +1,41 @@ +orWhereNull('destination_stopover_id')->count() > 0) { + TrainCheckin::with(['HafasTrip.stopovers', 'originStation', 'destinationStation']) + ->whereNull('origin_stopover_id') + ->orWhereNull('destination_stopover_id') + ->limit(100) + ->each(function(TrainCheckin $checkin) { + $originStopover = $checkin->HafasTrip->stopovers->where('train_station_id', $checkin->originStation->id) + ->where('departure_planned', $checkin->departure) + ->first(); + $destinationStopover = $checkin->HafasTrip->stopovers->where('train_station_id', $checkin->destinationStation->id) + ->where('arrival_planned', $checkin->arrival) + ->first(); + $checkin->update([ + 'origin_stopover_id' => $originStopover->id, + 'destination_stopover_id' => $destinationStopover->id, + ]); + + $this->info("Migrated stopover ids for checkin {$checkin->id}"); + }); + } + return Command::SUCCESS; + } +} diff --git a/app/Http/Controllers/Backend/Transport/TrainCheckinController.php b/app/Http/Controllers/Backend/Transport/TrainCheckinController.php index 934fccc3f..04b7988e2 100644 --- a/app/Http/Controllers/Backend/Transport/TrainCheckinController.php +++ b/app/Http/Controllers/Backend/Transport/TrainCheckinController.php @@ -32,7 +32,6 @@ use Exception; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Http\Resources\Json\AnonymousResourceCollection; -use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; use InvalidArgumentException; use JetBrains\PhpStorm\ArrayShape; @@ -190,15 +189,17 @@ private static function createTrainCheckin( ); try { $trainCheckin = TrainCheckin::create([ - 'status_id' => $status->id, - 'user_id' => $status->user_id, - 'trip_id' => $trip->trip_id, - 'origin' => $firstStop->trainStation->ibnr, - 'destination' => $lastStop->trainStation->ibnr, - 'distance' => $distance, - 'points' => $pointCalculation->points, - 'departure' => $firstStop->departure_planned, - 'arrival' => $lastStop->arrival_planned + 'status_id' => $status->id, + 'user_id' => $status->user_id, + 'trip_id' => $trip->trip_id, + 'origin' => $firstStop->trainStation->ibnr, //@todo: deprecated - use origin_stopover_id instead + 'origin_stopover_id' => $firstStop->id, + 'destination' => $lastStop->trainStation->ibnr, //@todo: deprecated - use destination_stopover_id instead + 'destination_stopover_id' => $lastStop->id, + 'distance' => $distance, + 'points' => $pointCalculation->points, + 'departure' => $firstStop->departure_planned, //@todo: deprecated - use origin_stopover_id instead + 'arrival' => $lastStop->arrival_planned //@todo: deprecated - use destination_stopover_id instead ]); $alsoOnThisConnection = $trainCheckin->alsoOnThisConnection; @@ -244,10 +245,11 @@ public static function changeDestination( ); $checkin->update([ - 'arrival' => $newDestinationStopover->arrival_planned, - 'destination' => $newDestinationStopover->trainStation->ibnr, - 'distance' => $newDistance, - 'points' => $pointsResource->points, + 'arrival' => $newDestinationStopover->arrival_planned, + 'destination' => $newDestinationStopover->trainStation->ibnr, + 'destination_stopover_id' => $newDestinationStopover->id, + 'distance' => $newDistance, + 'points' => $pointsResource->points, ]); $checkin->refresh(); diff --git a/app/Models/TrainCheckin.php b/app/Models/TrainCheckin.php index 0d66dedc7..45a2864ea 100644 --- a/app/Models/TrainCheckin.php +++ b/app/Models/TrainCheckin.php @@ -15,17 +15,40 @@ use Illuminate\Support\Facades\Log; /** + * //properties * @property int $id * @property int $status_id + * @property int $user_id + * @property string $trip_id + * @property int $origin @deprecated -> use origin_stopover instead + * @property int $origin_stopover_id + * @property int $destination @deprecated -> use destination_stopover instead + * @property int $destination_stopover_id + * @property int $distance + * @property int $duration + * @property UTCDateTime $departure @deprecated -> use origin_stopover instead + * @property UTCDateTime $manual_departure + * @property UTCDateTime $arrival @deprecated -> use destination_stopover instead + * @property UTCDateTime $manual_arrival + * @property int $points + * @property bool $forced + * + * //relations * @property HafasTrip $HafasTrip * @property TrainStopover $origin_stopover * @property TrainStopover $destination_stopover + * @property Status $status + * @property User $user * @property TrainStation $originStation + * @property TrainStopover $originStopoverRelation * @property TrainStation $destinationStation + * @property TrainStopover $destinationStopoverRelation * * @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. + * @todo use the `id` from trips, instead of the hafas trip id - this is duplicated data + * @todo drop the `origin`, `destination`, `departure` and `arrival` columns and use the stopover instead */ class TrainCheckin extends Model { @@ -33,25 +56,27 @@ class TrainCheckin extends Model use HasFactory; protected $fillable = [ - 'status_id', 'user_id', 'trip_id', 'origin', 'destination', + 'status_id', 'user_id', 'trip_id', 'origin', 'origin_stopover_id', 'destination', 'destination_stopover_id', 'distance', 'duration', 'departure', 'manual_departure', 'arrival', 'manual_arrival', 'points', 'forced', ]; protected $hidden = ['created_at', 'updated_at']; protected $appends = ['origin_stopover', 'destination_stopover', 'speed']; protected $casts = [ - 'id' => 'integer', - 'status_id' => 'integer', - 'user_id' => 'integer', - 'origin' => 'integer', - 'destination' => 'integer', - 'distance' => 'integer', - 'duration' => 'integer', - 'departure' => UTCDateTime::class, - 'manual_departure' => UTCDateTime::class, - 'arrival' => UTCDateTime::class, - 'manual_arrival' => UTCDateTime::class, - 'points' => 'integer', - 'forced' => 'boolean', + 'id' => 'integer', + 'status_id' => 'integer', + 'user_id' => 'integer', + 'origin' => 'integer', //@deprecated -> use origin_stopover_id instead + 'origin_stopover_id' => 'integer', + 'destination' => 'integer', //@deprecated -> use destination_stopover_id instead + 'destination_stopover_id' => 'integer', + 'distance' => 'integer', + 'duration' => 'integer', + 'departure' => UTCDateTime::class, //@deprecated -> use origin_stopover_id instead + 'manual_departure' => UTCDateTime::class, + 'arrival' => UTCDateTime::class, //@deprecated -> use destination_stopover_id instead + 'manual_arrival' => UTCDateTime::class, + 'points' => 'integer', + 'forced' => 'boolean', ]; public function status(): BelongsTo { @@ -74,14 +99,30 @@ public function HafasTrip(): HasOne { return $this->hasOne(HafasTrip::class, 'trip_id', 'trip_id'); } + /** + * @return BelongsTo + * @todo rename to `originStopover` when `getOriginStopoverAttribute` is removed + */ + public function originStopoverRelation(): BelongsTo { + return $this->belongsTo(TrainStopover::class, 'origin_stopover_id', 'id'); + } + + /** + * @return TrainStopover + * @todo make this a relation (hasOne) when the origin and destination columns are removed + */ public function getOriginStopoverAttribute(): TrainStopover { - $stopOver = $this->HafasTrip->stopovers->where('train_station_id', $this->originStation->id) + if ($this->origin_stopover_id !== null) { + return $this->originStopoverRelation; + } + + $stopover = $this->HafasTrip->stopovers->where('train_station_id', $this->originStation->id) ->where('departure_planned', $this->departure) ->first(); - if ($stopOver == null) { + if ($stopover === null) { //To support legacy data, where we don't save the stopovers in the stopovers table, yet. Log::info('TrainCheckin #' . $this->id . ': Origin stopover not found. Created a new one.'); - $stopOver = TrainStopover::updateOrCreate( + $stopover = TrainStopover::updateOrCreate( [ "trip_id" => $this->trip_id, "train_station_id" => $this->originStation->id @@ -93,17 +134,34 @@ public function getOriginStopoverAttribute(): TrainStopover { ); $this->HafasTrip->load('stopovers'); } - return $stopOver; + $this->update(['origin_stopover_id' => $stopover->id]); + return $stopover; } + /** + * @return BelongsTo + * @todo rename to `destinationStopover` when `getDestinationStopoverAttribute` is removed + */ + public function destinationStopoverRelation(): BelongsTo { + return $this->belongsTo(TrainStopover::class, 'destination_stopover_id', 'id'); + } + + /** + * @return TrainStopover + * @todo make this a relation (hasOne) when the origin and destination columns are removed + */ public function getDestinationStopoverAttribute(): TrainStopover { - $stopOver = $this->HafasTrip->stopovers->where('train_station_id', $this->destinationStation->id) + if ($this->destination_stopover_id !== null) { + return $this->destinationStopoverRelation; + } + + $stopover = $this->HafasTrip->stopovers->where('train_station_id', $this->destinationStation->id) ->where('arrival_planned', $this->arrival) ->first(); - if ($stopOver == null) { + if ($stopover === null) { //To support legacy data, where we don't save the stopovers in the stopovers table, yet. Log::info('TrainCheckin #' . $this->id . ': Destination stopover not found. Created a new one.'); - $stopOver = TrainStopover::updateOrCreate( + $stopover = TrainStopover::updateOrCreate( [ "trip_id" => $this->trip_id, "train_station_id" => $this->destinationStation->id @@ -115,7 +173,8 @@ public function getDestinationStopoverAttribute(): TrainStopover { ); $this->HafasTrip->load('stopovers'); } - return $stopOver; + $this->update(['destination_stopover_id' => $stopover->id]); + return $stopover; } /** @@ -123,7 +182,7 @@ public function getDestinationStopoverAttribute(): TrainStopover { * Precedence: Manual > Real > Planned. * Only returns the 'original' planned time if the updated time differs from the planned time. */ - private static function prepareDisplayTime($planned, $real = NULL, $manual = NULL): array { + private static function prepareDisplayTime($planned, $real = null, $manual = null): array { if (isset($manual)) { $time = $manual; $type = TimeType::MANUAL; @@ -134,28 +193,28 @@ private static function prepareDisplayTime($planned, $real = NULL, $manual = NUL $time = $planned; $type = TimeType::PLANNED; } - return array( - 'time' => $time, - 'original' => ($planned->toString() !== $time->toString()) ? $planned : NULL, - 'type' => $type - ); + return [ + 'time' => $time, + 'original' => ($planned->toString() !== $time->toString()) ? $planned : null, + 'type' => $type + ]; } public function getDisplayDepartureAttribute(): \stdClass { $planned = $this->origin_stopover?->departure_planned - ?? $this->origin_stopover?->departure - ?? $this->departure; - $real = $this->origin_stopover?->departure_real; - $manual = $this->manual_departure; + ?? $this->origin_stopover?->departure + ?? $this->departure; + $real = $this->origin_stopover?->departure_real; + $manual = $this->manual_departure; return (object) self::prepareDisplayTime($planned, $real, $manual); } public function getDisplayArrivalAttribute(): \stdClass { $planned = $this->destination_stopover?->arrival_planned - ?? $this->destination_stopover?->arrival - ?? $this->arrival; - $real = $this->destination_stopover?->arrival_real; - $manual = $this->manual_arrival; + ?? $this->destination_stopover?->arrival + ?? $this->arrival; + $real = $this->destination_stopover?->arrival_real; + $manual = $this->manual_arrival; return (object) self::prepareDisplayTime($planned, $real, $manual); } diff --git a/database/factories/TrainCheckinFactory.php b/database/factories/TrainCheckinFactory.php index 55f3a7aa1..a277160ea 100644 --- a/database/factories/TrainCheckinFactory.php +++ b/database/factories/TrainCheckinFactory.php @@ -13,17 +13,19 @@ class TrainCheckinFactory extends Factory public function definition(): array { $trip = HafasTrip::factory()->create(); return [ - 'status_id' => Status::factory(), - 'user_id' => User::factory(), - 'trip_id' => $trip->trip_id, - 'origin' => $trip->originStation->ibnr, - 'destination' => $trip->destinationStation->ibnr, - 'distance' => $this->faker->randomFloat(2, 0, 100), - 'departure' => $trip->departure, - 'manual_departure' => null, - 'arrival' => $trip->arrival, - 'manual_arrival' => null, - 'points' => $this->faker->numberBetween(0, 100), + 'status_id' => Status::factory(), + 'user_id' => User::factory(), + 'trip_id' => $trip->trip_id, + 'origin' => $trip->originStation->ibnr, //TODO: @deprecated - use origin_stopover_id in future instead + 'origin_stopover_id' => $trip->stopovers->where('train_station_id', $trip->originStation->id)->first()->id, + 'destination' => $trip->destinationStation->ibnr, //TODO: @deprecated - use destination_stopover_id in future instead + 'destination_stopover_id' => $trip->stopovers->where('train_station_id', $trip->destinationStation->id)->first()->id, + 'distance' => $this->faker->randomFloat(2, 0, 100), + 'departure' => $trip->departure, //TODO: @deprecated - use origin_stopover_id in future instead + 'manual_departure' => null, + 'arrival' => $trip->arrival, //TODO: @deprecated - use destination_stopover_id in future instead + 'manual_arrival' => null, + 'points' => $this->faker->numberBetween(0, 100), ]; } diff --git a/database/migrations/2023_12_18_000000_add_origin_and_destination_stopover_foreign_to_train_checkins.php b/database/migrations/2023_12_18_000000_add_origin_and_destination_stopover_foreign_to_train_checkins.php new file mode 100644 index 000000000..a41c266cf --- /dev/null +++ b/database/migrations/2023_12_18_000000_add_origin_and_destination_stopover_foreign_to_train_checkins.php @@ -0,0 +1,37 @@ +unsignedBigInteger('origin_stopover_id')->nullable()->after('origin'); + $table->unsignedBigInteger('destination_stopover_id')->nullable()->after('destination'); + + $table->foreign('origin_stopover_id')->references('id')->on('train_stopovers'); + $table->foreign('destination_stopover_id')->references('id')->on('train_stopovers'); + }); + } + + public function down(): void { + Schema::table('train_checkins', static function(Blueprint $table) { + $table->dropForeign(['origin_stopover_id']); + $table->dropForeign(['destination_stopover_id']); + + $table->dropColumn('origin_stopover_id'); + $table->dropColumn('destination_stopover_id'); + }); + } +}; diff --git a/tests/Feature/Webhooks/WebhookStatusTest.php b/tests/Feature/Webhooks/WebhookStatusTest.php index 9d5f751e8..ab46da177 100644 --- a/tests/Feature/Webhooks/WebhookStatusTest.php +++ b/tests/Feature/Webhooks/WebhookStatusTest.php @@ -108,7 +108,7 @@ public function testWebhookSendingOnDestinationChange() { $job->payload['event'] ); assertEquals($status->id, $job->payload['status']->id); - // This is really hacky, but i didn't got it working otherwise. + // This is really hacky, but I didn't get it working otherwise. $parsedStatus = json_decode($job->payload['status']->toJson()); assertEquals(self::AACHEN_HBF['id'], $parsedStatus->train->destination->evaIdentifier); return true;