Skip to content

Commit

Permalink
🗃️ prepare checkins table to use relation instead of duplicate data (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
MrKrisKrisu authored Dec 20, 2023
1 parent 3a9f12d commit e31071e
Show file tree
Hide file tree
Showing 6 changed files with 203 additions and 62 deletions.
41 changes: 41 additions & 0 deletions app/Console/Commands/MigrateStopovers.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

namespace App\Console\Commands;

use App\Models\TrainCheckin;
use Illuminate\Console\Command;

/**
* @deprecated Just created and marked as deprecated, because it is only needed for migrating old checkins.
* Can be deleted after migration.
*/
class MigrateStopovers extends Command
{

protected $signature = 'app:migrate-stopovers';
protected $description = 'Calculate missing stopover relations for train checkins. Currently only needed for migrating old checkins.';

public function handle(): int {
while (TrainCheckin::whereNull('origin_stopover_id')->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;
}
}
30 changes: 16 additions & 14 deletions app/Http/Controllers/Backend/Transport/TrainCheckinController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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();

Expand Down
131 changes: 95 additions & 36 deletions app/Models/TrainCheckin.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,43 +15,68 @@
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
{

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 {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -115,15 +173,16 @@ public function getDestinationStopoverAttribute(): TrainStopover {
);
$this->HafasTrip->load('stopovers');
}
return $stopOver;
$this->update(['destination_stopover_id' => $stopover->id]);
return $stopover;
}

/**
* Takes the planned and optionally real and manual times and returns an array to use while displaying the status.
* 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;
Expand All @@ -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);
}

Expand Down
24 changes: 13 additions & 11 deletions database/factories/TrainCheckinFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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),
];
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

/**
* Currently we have origin, destination, departure and arrival in the train_checkins table.
* This is not well-designed, because we have to store the same information multiple times.
* This migration adds two new columns for a foreign key to the stopovers table, so we can remove the duplicate
* information in the future.
*
* This is intentionally kept so small to make migration easier.
* The double columns should be gradually changed in the code.
*/
return new class extends Migration
{
public function up(): void {
Schema::table('train_checkins', static function(Blueprint $table) {
$table->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');
});
}
};
2 changes: 1 addition & 1 deletion tests/Feature/Webhooks/WebhookStatusTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit e31071e

Please sign in to comment.