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

Issues/43 incidents seed #44

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

socialskillsforitpeople
Copy link
Contributor

Fixes #43

@RaduCStefanescu
Copy link
Contributor

@claudiunicolaa could you please help with a review?
@socialskillsforitpeople thank you for the PR!

* and only pass the following values for the var
* incident_type_id when seeding the local db
*/
$incident_type_ids = [1,2,3,4,5,7,8,9,11,12];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use camelCase notation ($incidentTypeIds).

@claudiunicolaa
Copy link
Contributor

If in IncidentTypesTableSeeder one of the types change the id value this seeder can introduce invalid data / throw a foreign key error.
I suggest using in IncidentTypesTableSeeder an array const

const DATA = [
    [
        'id'    => 1,
        'label' => 'IT_OTHER',
        'code'  => 'OTH',
        'name'  => 'Altul'
    ],
.....
    [
        'id'    => 12,
        'label' => 'VOTE',
        'code'  => 'NUM',
        'name'  => 'Probleme în zilele votului'
    ]
];

iterate over DATA and insert

        foreach (self::DATA as $datum) {
            IncidentType::create($datum);
        }

in a static method iterate to obtain the ids

    public static function getIds()
    {
        $ids = [];
        foreach (self::DATA as $datum) {
            $ids[] = $datum['id'];
        }
        return $ids;
    }

and in IncidentsTableSeeder you can use the IncidentTypesTableSeeder::getIds() method.

It is just an idea. Ping me on slack (username: claudiunicolaa) if you have questions.

@RaduCStefanescu
Copy link
Contributor

Just a noob question, can we query for the IncidentType ids in the IncidentsTableSeeder? Because that will solve all our problems.
I agree with @claudiunicolaa the current solution is not ideal, so I will mark it as Needs work.

Copy link
Contributor

@RaduCStefanescu RaduCStefanescu left a comment

Choose a reason for hiding this comment

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

Hints in the comments.

@claudiunicolaa
Copy link
Contributor

Just a noob question, can we query for the IncidentType ids in the IncidentsTableSeeder? Because that will solve all our problems.
I agree with @claudiunicolaa the current solution is not ideal, so I will mark it as Needs work.

It is possible to query IncidentType in IncidentsTableSeeder.

* added camel case
* created a variable to hold incident types
* modified both seeders to use this variable as a data source
@socialskillsforitpeople
Copy link
Contributor Author

@claudiunicolaa Added a variable instead of a constant because of php constratints
@RaduCStefanescu @costeaalex updated as requested
@RaduCStefanescu we can of course also query DB. I prefer to keep it without this query because there are no other places in seeders where we are actually querying the DB. Also, since we now have this variable we don't need to call the db because we already have the data in the code.

Sorry for the long wait!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seed Incidents
5 participants