Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🗃️ migrate trips table to use ID instead of IBNR #2422

Closed
wants to merge 8 commits into from

Conversation

MrKrisKrisu
Copy link
Member

see #2411

… a single modification. (╯°□°)╯︵ ┻━┻
@MrKrisKrisu
Copy link
Member Author

"SQLite doesn't support dropping foreign keys (you would need to re-create the table)."

(╯°□°)╯︵ ┻━┻

@MrKrisKrisu MrKrisKrisu marked this pull request as draft March 11, 2024 19:32
@MrKrisKrisu MrKrisKrisu changed the title 🗃️ migrate trips table to use ID instead of IBNR [MAJOR] 🗃️ migrate trips table to use ID instead of IBNR Mar 17, 2024
@MrKrisKrisu MrKrisKrisu force-pushed the dev-ks/2411/hafastrips branch 2 times, most recently from 4922288 to 631ad15 Compare March 17, 2024 11:57
@MrKrisKrisu MrKrisKrisu changed the title [MAJOR] 🗃️ migrate trips table to use ID instead of IBNR 🗃️ migrate trips table to use ID instead of IBNR Mar 17, 2024
@MrKrisKrisu MrKrisKrisu marked this pull request as ready for review March 17, 2024 12:07
Role::where('name', 'event-moderator')->delete();
Role::where('name', 'open-beta')->delete();
Role::where('name', 'closed-beta')->delete();
// empty now - replaced by PermissionSeeder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Not a fan. Why is this needed?
Also:

  • This file should be deleted
  • Moving this logic from a migration to a seeder should be a separate PR (imho)


public function down(): void {
Permission::findByName('view event history')->delete();
// empty now - replaced by PermissionSeeder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Not a fan. Why is this needed?
Also:

  • This file should be deleted
  • Moving this logic from a migration to a seeder should be a separate PR (imho)

$permissionCreate->delete();
$permissionUpdate->delete();
$permissionDelete->delete();
// empty now - replaced by PermissionSeeder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Not a fan. Why is this needed?
Also:

  • This file should be deleted
  • Moving this logic from a migration to a seeder should be a separate PR (imho)

$roleAdmin->revokePermissionTo($permission);

$permission->delete();
// empty now - replaced by PermissionSeeder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Not a fan. Why is this needed?
Also:

  • This file should be deleted
  • Moving this logic from a migration to a seeder should be a separate PR (imho)

Role::findByName('closed-beta')->revokePermissionTo($permission);

$permission->delete();
// empty now - replaced by PermissionSeeder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Not a fan. Why is this needed?
Also:

  • This file should be deleted
  • Moving this logic from a migration to a seeder should be a separate PR (imho)

app/Http/Controllers/HafasController.php Outdated Show resolved Hide resolved
app/Models/Trip.php Show resolved Hide resolved
app/Models/Trip.php Outdated Show resolved Hide resolved
app/Models/Trip.php Outdated Show resolved Hide resolved
@MrKrisKrisu
Copy link
Member Author

@HerrLevin As discussed: I'll split up this PR in multiple small PR's and do separate migrations for create and drop the data for security reasons (data loss).

@MrKrisKrisu MrKrisKrisu marked this pull request as draft March 22, 2024 10:54
@HerrLevin
Copy link
Member

@MrKrisKrisu Not necessarily multiple PRs but multiple migrations. :)

# Conflicts:
#	database/migrations/2023_11_21_000002_add_default_permissions.php
#	database/migrations/2023_12_17_000003_add_event_history_permission.php
#	database/migrations/2024_01_29_000000_add_station_permissions.php
#	database/migrations/2024_01_30_000000_add_activity_permissions.php
#	database/migrations/2024_02_01_000000_add_permissions_for_manual_trips_beta.php
#	database/seeders/Constants/PermissionSeeder.php
#	database/seeders/DatabaseSeeder.php
#	tests/TestCase.php
#	update.sh
# Conflicts:
#	app/Http/Controllers/API/v1/StationController.php
#	app/Http/Controllers/HafasController.php
#	app/Models/Trip.php
#	database/factories/TripFactory.php
#	database/schema/sqlite-schema.sql
@MrKrisKrisu MrKrisKrisu deleted the dev-ks/2411/hafastrips branch May 21, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants