Skip to content

Conversation

@betsyecastro
Copy link
Contributor

Adds seeder to replace current slug values for the student table with a ULID.
Generates a ULID for new student records.

@betsyecastro betsyecastro added ✨ enhancement New feature or request 🌱 needs-seed Requires a seeder to be run labels Mar 24, 2025
@betsyecastro betsyecastro requested review from shukla-m and wunc March 24, 2025 14:24
@betsyecastro betsyecastro self-assigned this Mar 24, 2025
@betsyecastro betsyecastro added 🗃️ needs-migration Requires a migration and removed 🌱 needs-seed Requires a seeder to be run labels Apr 2, 2025
*/
public function updateStudent(Student $student): void
{
$existing_student = DB::table('students')->where('id', $student->id) ?? null;

Choose a reason for hiding this comment

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

The $existing_student retrieval/update can probably be skipped, while updating the $student instance directly:

$student?->update(['slug' => Str::ulid()]);

Copy link

@shukla-m shukla-m left a comment

Choose a reason for hiding this comment

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

@betsyecastro I pulled in the changes and tested locally and it seems to work as expected.

  • Please see notes on suggestion for a minor change in the seeder code.
  • I tested a user's bookmarks, and the bookmarked links work if they reload the bookmarks page after the migrations/seeder are run, otherwise they get a 404 error for a bookmark link, since the slug is changed.
  • Also, as I mentioned when we met, since there are multiple steps in the migration/seeder, wrapping the steps in a database transaction might make it more robust, so that it can be rolled back in case of failure.

Please let me know if you have any questions or need additional information. Thanks.

@betsyecastro betsyecastro added the ⬇️ priority:low Low priority issue label Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ enhancement New feature or request 🗃️ needs-migration Requires a migration ⬇️ priority:low Low priority issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants