TrackWho (likely) prevents Avatar generation on signup

[jlsjonas]

This is a bug.

Prerequisites

• Are you running the latest version?
• Are you reporting to the correct repository?
• Did you check the documentation?
• Did you perform a cursory search?

Description

Can't login anymore after switching to a fresh copy of Enso (using adldap2-laravel as provider; but shouldn't affect anything & error only occurs after passing it's authentication);

seems to be caused by TrackWho wanting the userId for CreatedBy; even though there is none available yet.

Steps to Reproduce

1. Attempt to login using latest Enso (after full install procedure including npm run dev/hot)

Expected behavior

Login

Actual behavior

Trying to get property of non-object
C:...\Enso\vendor\laravel-enso\trackwho\src\app\Traits\CreatedBy.php#10
ErrorException
protected static function bootCreatedBy()
{
self::creating(function ($model) {
$model->created_by = auth()->user()->id;
});
}

[jlsjonas]

Also note that after overriding the function in app\User to failover (to 0) dashboard only seemed to load with <App> & <Progressbar> visible in the vue devtools (without any errors, that is; neither client nor server-side)
VueX mutations are being ran correctly btw

[aocneanu]

post here the full trace

[jlsjonas]

As I can't reproduce it today, it's likely related to the auto-import feature of adldap, which likely triggers the bootCreatedBy before the user is authorized (as there's no db entry yet)

Will reopen if reproducible again

[jlsjonas]

Going to reopen as I likely pinned down the underlying issue; I consistently noticed the users detail page couldn't be loaded for my "2nd" user; the created_by is likely caused by the activity log entry for generating a missing avatar during login (account exists; can log in; but can't open the user detail page nor see the profile menu item; as they both error out on TypeError: Cannot read property 'id' of null (because avatar is null by default)

(Not sure if the bug is in AvatarManager or related to it's implementation in core so keeping it in this issue for now)

edit: clarifying that manually adding an avatar entry does fix the problem; so it is 100% related to a missing initial avatar (which you need to reach the page to be able to generate/upload an avatar 😅 )

edit2: while checking the code; the issue appeared because creating the user failed so I ended up duplicating the row in the users table. a missing avatar should not cause half of the framework to fail though; so keeping the issue open to implement a fallback (will submit a PR if I have the time to)

[aocneanu]

User model uses the HasAvatar trait. This trait autogenerates the avatar on user creation, IF the user is creating with Eloquent.

If you want to insert users with a bulk insert from a seeder or something similar you can generate the avatars with php artisan enso:avatars:generate

a missing avatar should not cause half of the framework to fail though

so I ended up duplicating the row in the users table

You can always improve the framework or switch to another.

[jlsjonas]

You can always improve the framework or switch to another.

so keeping the issue open to implement a fallback (will submit a PR if I have the time to)

[jlsjonas]

Was easily mitigated by slightly altering avatar() in vendor/laravel-enso/avatarmanager/src/app/Traits.php/HasAvatar.php (or thus overriding said method when using the Trait, so it's storable in VCS)

    public function avatar()
    {
        return $this->hasOne(Avatar::class)->withDefault();
    }

leaving it here if someone stumbles upon it; a side-effect is that you can get an invalid image as avatar is basically a pivot table linking with files (which can't find the correct reference).

However, as we'll have the avatars soon enough it would be suitable enough for our purposes; but IMHO not polished enough to warrant a PR.

I'll leave the decision of leaving this issue open for now or closing it to you @aocneanu
(note that the fix workaround was done in AvatarManager but the core issue is most likely still related to it's implementation in Core (or possibly even /Enso itself; as most FE-files currently reside there; but afaik you're moving as much of that here))

(and as an aside: You/your team has done a great job with the framework, I wish I had the opportunity to dedicate more resources towards improving it (I have started the procedures with the company I work for to (hopefully) be able to opt-in to the support services as I do think you deserve it (and some of these issues ended up coming awfully close to the service). I'll be reaching out to you via email in case it gets approved ;)

[aocneanu]

It might not be a bad ideea to have withDefault() on this relationship. I'll look into it.

and as an aside

Noted, thanks.

[jlsjonas]

What I ended up doing is dedicate a record in avatar as a default avatar; and updating the store's setUser to have:

        setUser: (state, user) => {
            state.user = user;
            if (!state.user.avatar.id) state.user.avatar = { id: 1 };
            return state.user;
        },

instead; this allows FE to have a fallback without having to change too many parts (though not an ideal solution per see)

The withDefault option would be better; but would require defaulting a files entry too in it's current implementation

EDIT: using above workaround, note that you'll have to do the same everywhere you're fetching a profile.

f.e. in UserProfile.vue, replace the getProfile() with

                .then(response => {
                    this.profile = response.data.user;
                    if (!this.profile.avatar.id) this.profile.avatar = { id: 1 };
                    return this.profile;
                })

i.e.: it's likely better done on the backend (which involves withDefault())

[jlsjonas]

    public function avatar()
    {
        return $this->hasOne(Avatar::class)->withDefault([
            'id' => 1
        ]);
    }

should fix it on the backend (requiring a dummy record & file under id 1)

the clean way for us to implement this would be putting it in app\user; however it seems that profilebuilder is using ensoUser instead of app\user

[aocneanu]

How are you creating the new User?

LaravelEnso\Core\app\Models\User.php uses the HasAvatar trait.

This trait on the created event generates an avatar for the newly created user.

[jlsjonas]

This issue is about having a default fallback; and the last comments are the debugging/fixing steps to have exactly that.

The HasAvatar trait doesn't implement ->WithDefaults yet; and made me discover a couple discrepancies (creating a PR atm) regarding the user model reference (ProfileBuilder & UserController are both taking EnsoUser instead of the App\User, causing overrides only to apply to a part of the framework.

[aocneanu]

That's because both ProfileBuilder and UserController are part of the framework. But you can easily overwrite them.

[jlsjonas]

Correct, however, UserController is also using/expecting App\Http\Controllers\Controller; given that the framework by default includes an (empty) App\User it makes sense to be the referenced-to model in all cases.

[aocneanu]

You have a good point. Probably all controllers from Enso should extend the BaseController and use only the needed traits. I'll work on that :)

Regarding the User I really don't think that Core\UserController should use anything but Core\User.

If you need more than what's provided it's so easy to overwrite the routes, and use your own controller.

Or if suitable, you can extend the existing Core\UserController and just add your custom method that will use App\User or Core\User depending on what you need.

[jlsjonas]

Great!

regarding UserController/ProfileBuilder; the issue with the current implementation is that you basically have to copy/paste the file, just changing the "use"; as you'll be extending core in 99% of cases (and it's actually the default behaviour of Enso)

I would still opt to expect an App\User (which should by default extend from the Core\app\Models\user)
Or use some abstract way instead (the issue why I made the PR is that Laravel errors out because of the class mismatches; even though App\User is based of Core\App.

Interested to hear alternatives too (taking into account that the controller is already generic enough to just be kept the same even if you extend the EnsoUser (depending on your implementation ofc.)

[aocneanu]

Maybe you could share the code where you have the class mismatch.

[jlsjonas]

Simple... the Usercontroller methods requiring an EnsoUser instead of the App's user; which in turn passes it along to the ProfileBuilder. So any modifications made to the (by default provided) App\User model basically breaks the builder; just because it's taking the wrong model.

In this case, the avatar id fallback.

It's just locking down the core too much in my opinion; doesn't make sense if the core is always meant to be integrated into something...

my app\user.php exposes some extra fillables, which are not available due to the wrong model being used; and any custom methods are also unavailable/not executed.
Providing an extension of the User (by default) without being able to do anything with it doesn't make sense; but correct me if I'm wrong...

[aocneanu]

Jonas, the case with the avatar is not a relevant example because this will be fixed at the framework level, as it should be.

Our focus is not trying to do everything we can do, but to do everything that we do in a correct and consistent manner.

[jlsjonas]

The avatar is just an example though; you can't simply add something to the App\user fillable and have it exposed in the FE; even though App\User is a class that's provided by default, extending EnsoUser.

I understand your reasoning behind it too, but maybe these files belong in Enso instead of Core in that case?

[aocneanu]

you can't simply add something to the App\user fillable and have it exposed in the FE

That seems simple but would mean not respecting encapsulation for Enso's classes. Allowing devs to do anything they want on a model used in a core controller would potentially get them in all sorts of confusing situations. Enso being constantly upgraded we would not know when an upgrade will break something for somebody without us even knowing to warn the devs about the implications.

maybe these files belong in Enso instead of Core in that case

We had it like that for a while (including the auth part) but we decided to move the structures to core. We have over 20 applications live that don't have changes to the owner, user or auth structures. Every time we changed something at the framework level regarding this structures we had to manually change hundreds of files in order to upgrade our applications. So no:)

If you need more than the provided functionalities you can easily copy / paste the needed files and modify them to fit your needs. Simple and without breaking anything, without causing confusions or endless struggles on the maintenance part.

[jlsjonas]

Maybe adding a notice to the default App\User would be in place though; to prevent confusion ;)