self::creating expects a person entry to already be present

jlsjonas commented 6 years ago

As requested in #1, recreating

This is a bug.

Prerequisites

[x] Are you running the latest version?
[x] Are you reporting to the correct repository? (enso is made of many specialized packages: https://github.com/laravel-enso)
[x] Did you check the documentation?
[x] Did you perform a cursory search?

Description

from the 11.12 CHANGELOG:

update user and owner seeders with the ones from laravel-enso/core if they are not customized already (optional)

self::creating in the IsPerson trait seems to do an update; thus expecting an entry to already be there.

If I keep my existing UserSeeder configuration, the following error occurs

Steps to Reproduce

migrate:fresh --seed with a pre-11.12 userseeder (except for modifying owner to group)

Expected behavior

Seeding to happen

Actual behavior

Call to member function update() on null

aocneanu commented 6 years ago

This is not a bug, is the intended behaviour.

migrate:fresh --seed with a pre-11.12 userseeder (except for modifying owner to group)

The reason we have this checkbox in the issue template: [ ] Are you running the latest version? is because we don't spend time supporting old versions.

jlsjonas commented 6 years ago

I AM running the latest version; and following the changelog did clearly say that step was optional (which is clearly is not).

The lastest version IS the version having this issue: I can't create a user using User::create([..]) anymore. As described, this will be causing issues with adldap2 creating users that have an AD account but don't exist on our system yet (I have an attribute sync handler to populate the Person fields... but I don't control the fact that you need a user to sync first)

The fact that it was discovered in the migration doesn't change anything

aocneanu commented 6 years ago

changelog did clearly say that step was optional (which is clearly is not).

Could you point me to that section of the changelog? I want to update it but I'm unable to locate it.

I can't create a user using User::create([..]) anymore.

Well obviously, the user logic has changed.

Completely besides the point:

who's forcing you to upgrade? Why don't you fork v2.11.x and develop it on your needs?
what would you do in Enso with a user without name ? The whole front-end would be broken.
if you really want users without the person relation just add a migration in your local project that changes the person_id in the users table nullable(). So damn easy.
if you want to override the default user-person creation cycle extend the core user and
- rewrite your create method as you wish
- or use Observers
- or customize the factory
- or do whatever makes sense in your business logic.

(btw, maybe now you DO understand why we choose no using the App\User in the core package)

There must be tens of ways to overcome problems like this, but like we got used you don't provide any damn specifics of your problem, only an attitude

The point you're obviously missing is that Enso is the framework we use our company's projects. The development of Enso will be correlated to our needs first and not a third party package that you or somebody else is using.

Instead of approaching the issues you have with Enso in such a hurry and judgemental way, why wouldn't you just ask nicely:

"Hi, after thoroughly going over the logic and testing I found this problem that I'm clearly describing here, what do you think it's the best way to overcome it"?

jlsjonas commented 6 years ago

Could you point me to that section of the changelog? I want to update it but I'm unable to locate it.

It was actually part of the 2.11.12 upgrade; 2.12 isn't mentioning anything regarding Seeders in the upgrade steps; As we didn't do the 2.11.12 yet it got slightly mixed up (It's however still unclear in the changelog as it doesn't mention them in the 2.12.X upgrade steps) https://github.com/laravel-enso/Enso/blob/master/CHANGELOG.md#21112

Well obviously, the user logic has changed. Which I understand; however (and I feel this has been partially implemented?) creating a user should just propagate a Person::create if it doesn't exist yet; as they both require an email anyway.

This ensures a more standard approach to dealing with Users which would help extensibility a lot.

And to give you a bit more insight...

who's forcing you to upgrade?

No-one; but you've added features that are beneficial to us too. + upgrading regularly vs. after f.e. 2 years (because of features/bugfixes/whatever) would likely be more beneficial; from our past experience.

what would you do in Enso with a user without name ? The whole front-end would be broken.

Nothing; it's also not what I'm suggesting. All I'm suggesting is the self::creating to actually create a Person if it doesn't exist yet, instead of fail. (sorry if I was unclear about this)

if you really want users without the person relation...

We don't want that, see the previous answer.

if you want to override the default user-person creation cycle extend the core user

That was already our alternative approach, however, I feel that it would be better if part of the framework.

The point you're obviously missing is that Enso is the framework we use our company's projects. The development of Enso will be correlated to our needs first

I understand; I never expected you to "jump" to fix an issue like this as top priority ;)

I hope this clears some things up :) and I'm sorry for sounding judgmental in the past.

Also to clarify in general, when I create an issue it doesn't mean I don't want to do the work to fix it myself; it means it's something that I consider it something to be fixed at the framework level. If it's something that isn't beneficial enough for your company I'm perfectly ok with receiving a "Would accept a PR for this" instead of the issue being resolved by your team (not that I mind the latter, of course 😉 )

jlsjonas commented 6 years ago

bootIsPerson doesn't have access to enough data to pre-populate.

For anyone stumbling here later on, for adldap2-laravel you can use the attributeHandler (it executes before the creation process) to pre-populate the user.

example code:


    /**
     * Synchronizes ldap attributes to the specified model.
     *
     * @param LdapUser $ldapUser
     * @param EloquentUser $eloquentUser
     *
     * @return void
     */
    public function handle(LdapUser $ldapUser, EloquentUser $eloquentUser)
    {
        if ($eloquentUser->id) {
            Log::info('user exists');
            $eloquentUser->person->name = $ldapUser->getCommonName();
        } else {
            Log::warning('new user: ' . $ldapUser->getCommonName());
            $person = Person::firstOrCreate(['email' => $ldapUser->getUserPrincipalName()]
                , [
                    'title' => Titles::Mr,
                    'name' => $ldapUser->getCommonName(),
                    'appellative' => optional($ldapUser)->givenname[0],
                    'gender' => Genders::Male,
                    'birthday' => substr(optional($ldapUser)->whencreated[0],0,8), //you likely want something else here
                    'phone' => optional($ldapUser)->telephonenumber[0],
                ]);
            $eloquentUser->person()->associate($person);
            $eloquentUser->group()->associate(2); // 2 = Default
            $eloquentUser->role()->associate(2); // 2 = Default
            $eloquentUser->is_active = true;
        }
    }

The code obviously works with other tools too ;)

Going to close this for now as implementation would easily feel unintuitive

laravel-enso / people