DirectoryTree / LdapRecord-Laravel

Multi-domain LDAP Authentication & Management for Laravel.
https://ldaprecord.com/docs/laravel/v3
MIT License
492 stars 51 forks source link

[Question] Event listener not triggering #545

Closed ubay25 closed 1 year ago

ubay25 commented 1 year ago

Environment:

Hi Steve, I'm trying to get the event listener to work. Basically, I wish to fire up an email notification when a user is being imported.

This is the command I use to import a single user which works fine. php artisan ldap:import ldap --filter "(cn=John Doe)"

I then have this on my EventServiceProvider file.

/**
* The event listener mappings for the application.
*
*/
protected $listen = [
        //...

        \LdapRecord\Laravel\Events\Importing::class => [
            \App\Ldap\Listeners\ObjectImporting::class
        ],

       // ...
];

This is my listener saved in App\Ldap\Listeners\ObjectImporting.php.

namespace App\Ldap\Listeners;

use LdapRecord\Laravel\Events\Importing;
use Stackkit\LaravelDatabaseEmails\Email;

class ObjectImporting
{
    public function handle(Importing $event)
    {
        $objectName = $event->getModel()->getName();

        Email::compose()
            ->label('test_ldap')
            ->recipient('notifications@local')
            ->subject('Test Ldap')
            ->view('emails.demo')
            ->variables([
                'name' => $objectName,
            ])
            ->send();
    }
}

The mail package I am using creates a record on my database for any email that will be sent. However if I issue the command to import, it's not triggerring the email.

Any ideas?

stevebauman commented 1 year ago

Hi @ubay25,

The Importing event only fires when new users are imported (i.e. ones that have not been imported before). Does an existing user already exist that has already been imported? If so, you can delete the user in your database and re-import, and you should then see the event being fired:

https://ldaprecord.com/docs/laravel/v3/auth/database/importing/#events

If you want to hook into an event whenever a user is being imported or synchronized, you can hook into the Synchronized event instead:

use LdapRecord\Laravel\Events\Synchronized;

Let me know if this helps 👍

ubay25 commented 1 year ago

Hi @stevebauman

Thanks for the quick response.

Yes I was deleting my test user first before I execute the import command as I want the event to trigger when a new user is being created.

I also tried the Synchronized option and the same thing happens, it's not triggering the event.

:(

stevebauman commented 1 year ago

Are you sure your listener isn't firing? The import command wouldn't work without the events being fired, as there are listeners that are registered to advance the progress bar when the Imported event is fired:

https://github.com/DirectoryTree/LdapRecord-Laravel/blob/770ef6bc3862897a198e61d0afba73ef14845acc/src/Commands/ImportLdapUsers.php#L204-L230

Try inserting a dd() in the handle() method to see if it's being reached.

ubay25 commented 1 year ago

Hi @stevebauman

So I edited the listener file to below as you suggested.

namespace App\Ldap\Listeners;

use LdapRecord\Laravel\Events\Importing;
use Stackkit\LaravelDatabaseEmails\Email;

class ObjectImporting
{
    public function handle(Importing $event)
    {
        dd('Test');
    }
}

I then deleted the user and executed the import command. See output below. image

I then go back to my app's database and I can see that the user has been re-created with a different id as you would expect for auto increment.

I assume the dd() outputs the value on the terminal? As you can see on the screenshot, there's none. Sorry :(

stevebauman commented 1 year ago

Ah, it's because the event class you're referencing is incorrect. It should be:

- \LdapRecord\Laravel\Events\Importing::class =>
+ \LdapRecord\Laravel\Events\Import\Importing::class => 

I realize this is a documentation mistake, sorry! I'll update it now 👍

ubay25 commented 1 year ago

Hi @stevebauman

Thanks! I changed it now. However I get this error when I do the import command.

App\Ldap\Listeners\ObjectImporting::handle(): Argument #1 ($event) must be of type LdapRecord\Laravel\Events\Importing, LdapRecord\Laravel\Events\Import\Importing given, called in /var/www/html/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php on line 424

Any ideas?

stevebauman commented 1 year ago

You have to update your listener with the new namespace as well:

namespace App\Ldap\Listeners;

- use LdapRecord\Laravel\Events\Importing;
+ use LdapRecord\Laravel\Events\Import\Importing;
use Stackkit\LaravelDatabaseEmails\Email;

class ObjectImporting
{
    public function handle(Importing $event)
    {
        $objectName = $event->getModel()->getName();

        Email::compose()
            ->label('test_ldap')
            ->recipient('notifications@local')
            ->subject('Test Ldap')
            ->view('emails.demo')
            ->variables([
                'name' => $objectName,
            ])
            ->send();
    }
}

That should resolve your issue 👍

ubay25 commented 1 year ago

Hi @stevebauman

Sorry to re-open this but although the import reports success, the user was not being added. See screenshot below.

image

stevebauman commented 1 year ago

Hi @ubay25, please check your storage/logs directory for the exception that's being generated during import 👍

ubay25 commented 1 year ago

Hi @stevebauman

Ok some progress, it works if I don't have anything inside the handle of my listener but if I put something like below.

public function handle(Imported $event)
    {
        $objectName = $event->getModel()->getName();

        // ...
    }

I get the error below. Call to undefined method LdapRecord\Laravel\Events\Import\Imported::getModel()

By the way, I changed the event from Importing to Imported because I realised I need to make sure the user has been imported (inserted on the database) to make my other app event to work.

ubay25 commented 1 year ago

Hi @stevebauman Any ideas on the above error?

stevebauman commented 1 year ago

Hi @ubay25,

I think you may be confusing the LdapRecord-Laravel import events with the core LdapRecord model events that have the getModel() method:

https://ldaprecord.com/docs/core/v3/events/#model-events https://github.com/DirectoryTree/LdapRecord/blob/master/src/Models/Events/Event.php

The imported events have public properties to retrieve the models from the event:

https://github.com/DirectoryTree/LdapRecord-Laravel/blob/master/src/Events/Import/Event.php#L10-L18

public function handle(Imported $event)
{
    $objectName = $event->object->getName();

    // ...
}
ubay25 commented 1 year ago

Thanks @stevebauman I knew it's something I missed, thank you!

By the way, getName() returns the name of the user I presume, how about the samaccountname?

stevebauman commented 1 year ago

No problem @ubay25.

By the way, getName() returns the name of the user I presume, how about the samaccountname?

getName() returns the cn (common name) of the user that's present in their distinguished name:

$user = User::findOrFail('cn=John Doe,dc=local,dc=com');

$name = $user->getName(); // "John Doe"

To retrieve the user's samaccountname, use getFirstAttribute():

$user = User::findOrFail('...');

$accountName = $user->getFirstAttribute('samaccountname');
ubay25 commented 1 year ago

Awesome, thank you very much @stevebauman you're a star!