Update tou_version and tou_acceptances migration #1028
Conversation
outdooracorn
left a comment
outdooracorn
left a comment
There was a problem hiding this comment.
- The title is incorrect. This isn't a refactor, as it makes changes to the behaviour.
code refactoring is the process of restructuring existing source code—changing the factoring—without changing its external behavior
- https://en.wikipedia.org/wiki/Code_refactoring - Please add a PR description detailing the changes being made and the rationale for them. This can be used as the git commit message when this PR is squashed and merged. Treating the PR description as the squashed and merged git commit message means it can be reviewed at the same time as the code. Once a commit is in the
mainbranch, it shouldn't be modified.
There was a problem hiding this comment.
This looks like it is renaming and modifying an existing database migration. Each change to the database schema requires its own migration. If you delete or modify an existing migration, you will likely break Laravel's migration tracking.
In this case, I expect that:
- Trying to roll back the
2025_10_14_091126_tou_versions.phpmigration will fail, as that file no longer exists - When trying to apply the
2026_01_06_131233_tou_version.phpmigration, it would fail due to thetou_versionstable already existing. Did this succeed when you applied this migration locally?
This PR should contain a new migration file that makes the required changes to the existing tou_versions and tou_acceptances tables. See https://laravel.com/docs/10.x/migrations#updating-tables. All previous migration files should remain untouched.
I also recommend reading up on how Laravel's database migration process works. Laravel's docs on Database Migrations is probably a good place to start.
There was a problem hiding this comment.
Same as above, each time the database schema is changed, a new migration is required.
| $table->id(); | ||
| $table->unsignedInteger('user_id'); | ||
| $table->string('tou_version', 10); | ||
| $table->string('tou_version'); |
There was a problem hiding this comment.
How come you've removed the $length parameter from the string() method? This will now use the default length of 255 ([1] [2]) when we know the length will always be 10 characters.
As hinted at in #977 (review), I think we should use char(10) to create a fixed width CHAR column. We don't need the flexibility that a VARCHAR column provides, so the slight performance improvement and data efficiency would be good to benefit from.
You'll probably have to do something like this to update the column's data type:
Schema::table('tou_acceptances', function (Blueprint $table) {
....
$table->char('tou_version', 10)->change();
....
}
| protected $visible = self::FIELDS; | ||
|
|
||
| protected $casts = [ | ||
| 'version' => 'string', |
There was a problem hiding this comment.
Is this cast needed now that we have $primaryKey = 'version' and $keyType = 'string'?
| public function up(): void { | ||
| Schema::create('tou_versions', function (Blueprint $table) { | ||
| $table->id(); | ||
| $table->string('version')->unique(); |
There was a problem hiding this comment.
Same here, make the column a CHAR instead of VARCHAR:
Schema::table('tou_versions', function (Blueprint $table) {
....
$table->char('version', 10)->change();
....
}
| public function up(): void { | ||
| Schema::create('tou_versions', function (Blueprint $table) { | ||
| $table->id(); | ||
| $table->string('version')->unique(); |
There was a problem hiding this comment.
Remember to make the version column the new primary key!
dati18
changed the title
|
Closing pull request due to creating a new PR at #1030 |
Bug: T407722