hackgreenville-com-435 Build out db seeders to create an org and some org related data #438
Conversation
|
I need to track down why the tests are failing, I might need to combine this PR with #437 to help resolve the failure |
| <?php | ||
|
|
||
| namespace App\Traits; | ||
|
|
||
| trait HasSlug | ||
| { | ||
| public static bool $update_slug = false; | ||
|
|
||
| public static function bootHasSlug() | ||
| { | ||
| static::creating(function ($model) { | ||
| if ( ! $model[self::$slug_column ?? 'slug']) { |
There was a problem hiding this comment.
My initial reaction is, do we need this?
Also, if we need this to be "automatic" we should use a package that way we do not need to maintain this code.
There was a problem hiding this comment.
What happens when the package goes away or becomes unmaintained? This is a relatively straightforward approach
There was a problem hiding this comment.
I find it hard to believe that a spatie package goes away, again, I think the less custom stuff we need to maintain and bugfix the better.
There was a problem hiding this comment.
Agreed. This does seem like code we shouldn't maintain ourselves, personally.
There was a problem hiding this comment.
This missing functionality has been in place for a while. I tried to implement a simple solution using laravel methods and practices. If there is a suggested package, either links it or creates a PR to this branch or something to resolve the discrepancy.
| 'address' => $this->faker->address, | ||
| 'zipcode' => $this->faker->postcode, | ||
| 'country' => $this->faker->countryCode(), | ||
| 'phone' => $this->faker->phoneNumber, | ||
| 'city' => $this->faker->city, | ||
| 'state_id' => State::factory(), |
There was a problem hiding this comment.
this should be a factory
There was a problem hiding this comment.
We don't want to generate random states for new venues. Take note of the next line where it selects a random existing state.
There was a problem hiding this comment.
Do we need to check to see if a state exists first or is that secured by the migration done here?
There was a problem hiding this comment.
Not against what is being done below, but what's the motivation behind this change? Just the fact that our tests are generating a new state value for each venue created?
There was a problem hiding this comment.
@irby the State::inRandomOrder()->first() code selects a random existing state from the database.
There was a problem hiding this comment.
We should not create a new state. Moving the states into a migration ensures they are safely created in the database. The migration only makes the state if it does not exist.
Doing it as a seeder in tests, working in general, is more of a pain since you need to make sure to run seeders and since this is required data for things to work. It should be a migration.
| $org->category()->associate($category); | ||
|
|
||
| // Recycle or create events | ||
| $events = Event::inRandomOrder()->limit(random_int(1, 10))->get(); |
There was a problem hiding this comment.
Why would we want to grab existing events and attach them to a new org? I would think we could just generate a random amount of new events with the factory and just attach them to the org.
Also, would we want to store random_int(1, 10) as a variable? Wouldn't referencing it as we are cause a new random value to be generated for each call?
| 'address' => $this->faker->address, | ||
| 'zipcode' => $this->faker->postcode, | ||
| 'country' => $this->faker->countryCode(), | ||
| 'phone' => $this->faker->phoneNumber, | ||
| 'city' => $this->faker->city, | ||
| 'state_id' => State::factory(), |
There was a problem hiding this comment.
Do we need to check to see if a state exists first or is that secured by the migration done here?
| 'address' => $this->faker->address, | ||
| 'zipcode' => $this->faker->postcode, | ||
| 'country' => $this->faker->countryCode(), | ||
| 'phone' => $this->faker->phoneNumber, | ||
| 'city' => $this->faker->city, | ||
| 'state_id' => State::factory(), |
There was a problem hiding this comment.
Not against what is being done below, but what's the motivation behind this change? Just the fact that our tests are generating a new state value for each venue created?
| <?php | ||
|
|
||
| namespace App\Traits; | ||
|
|
||
| trait HasSlug | ||
| { | ||
| public static bool $update_slug = false; | ||
|
|
||
| public static function bootHasSlug() | ||
| { | ||
| static::creating(function ($model) { | ||
| if ( ! $model[self::$slug_column ?? 'slug']) { |
There was a problem hiding this comment.
Agreed. This does seem like code we shouldn't maintain ourselves, personally.
resolves #435