monicahq / monica

Personal CRM. Remember everything about your friends, family and business relationships.
https://beta.monicahq.com
GNU Affero General Public License v3.0
21.45k stars 2.14k forks source link

Implement webhooks #318

Open phrohdoh opened 7 years ago

phrohdoh commented 7 years ago

This can be seen as part of building out the API but is the reverse side of the coin.

I have a working prototype that I will clean up and submit for review hopefully later today.

degan6 commented 7 years ago

Can you give a few examples of when the a webhook would fire?

djaiss commented 7 years ago

@Phrohdoh yeah I would be interested in knowing for which case you would like to use webhooks for.

phrohdoh commented 7 years ago

Sorry I have been quite busy in the past day.

Think of the API as another service letting Monica know it has done something and the Webhooks Monica letting other service know something was done.

The idea is that most, if not all, user actions would trigger a webhook.

etc.

This opens the door for extension that isn't dependent on the Monica vision / developers.

Legally consuming services do not have to be AGPL because copyright law does not extend that far so this allows monetization that wouldn't hinder the core product (I believe this is important to the product's success -- a walled garden is good for no-one).

A developer could wire monica<->(other service, think budgeting) together [with the API+Webhooks] so all user data is in sync and available everywhere.

My WIP implementation is wired up for note creation.

https://github.com/monicahq/monica/compare/master...Phrohdoh:wip-webhooks?diff=unified&name=wip-webhooks

Unfortunately I have been unable to test my implementation more than once (and I don't know why it worked only that single time) because artisan is throwing:

$ php artisan migrate

Warning: require(monica/bootstrap/../vendor/autoload.php): failed to open stream: No such file or directory in monica/bootstrap/autoload.php on line 17

Fatal error: require(): Failed opening required 'monica/bootstrap/../vendor/autoload.php' (include_path='.:') in monica/bootstrap/autoload.php on line 17
phrohdoh commented 7 years ago

In the spirit of (partial-)transparency I do have plans that I'd eventually like to bring to light once further developed but they do depend on API+Webhook functionality.

djaiss commented 7 years ago

I don't anything against webhooks. On the contrary, I'm all for it.

First, API. It's the first natural step and where I'll put my efforts. Webhooks is a natural extension to that.

phrohdoh commented 7 years ago

I just realized that my rebase was a bit bogus so I have more work to do.

The meat of the branch linked above is the changes to storeNote.

    /**
     * Store the note for this people.
     * @param  [type] $peopleId [description]
     * @return [type]           [description]
     */
    public function storeNote(Request $request, $contactId)
    {
        $user_id = Auth::user()->id;
        $contact = Contact::findOrFail($contactId);

        if ($contact->account_id !== Auth::user()->account_id) {
            return redirect()->route('people.index');
        }

        $body = $request->input('body');
        $contact->addNote($body);
        $request->session()->flash('success', trans('people.notes_add_success'));

        $webhook = DB::table('webhooks')
            ->where([
                ['user_id', '=', $user_id],
                ['event_type', '=', 'note.new'],
                ['is_active', '=', 1],
            ])->first();

        if ($webhook !== null)
        {
            $webhook_url = urldecode($webhook->urlencoded_endpoint);
            // TODO(review): Need to decide what data to send and in what format (JSON is loved right now and our data isn't too complex).
            // Can we searlize an entire model (sans explicitly blacklisted properties) with something in the stdlib?
            $webhook_data = json_encode([
                "contact" => url_encode($contactId),
                "body" => url_encode($body),
                "event_type" => $webhook->event_type(),
            ]);
            $req = new Client();
            $client->post($webhook_url, [ "json" => $webhook_data ]);
        }

        return redirect('/people/' . $contact->id);
    }
djaiss commented 7 years ago

I would make the webhook asynchronous, as a job perhaps, not through the main thread.

So the controller here would simply dispatch the job, with what it should do in parameters.

phrohdoh commented 7 years ago

Indeed these would be async in a merge-able implementation.

phrohdoh commented 7 years ago

@djaiss Do you have any idea why artisan is erroring for me?

phrohdoh commented 7 years ago

Ok monica will automatically run any new migrations for me once I run it, that is nice. Now I can't seem to figure out how to log strings so that I can inspect errors in the webhook code.

Nothing I try is showing up in stdout and I don't know if Docker is eating my logs or how this is being printed:

monicahq_1  | No scheduled commands are ready to run.

I don't see that string in the codebase.

djaiss commented 7 years ago

Have you tried the Log library https://laravel.com/docs/5.4/errors#logging?

phrohdoh commented 7 years ago

@djaiss Yes, I suspect Docker is eating those logs too.

Without the ability to log or debug I can't develop anything serious on Monica which is quite disappointing. 😢

BranTurner commented 7 years ago

@Phrohdoh Perhaps not ideal, but you could use dd() to dump variables in the absence of logs. Might work as a temporary fix until the log problem is resolved?

phrohdoh commented 7 years ago

@BranTurner Unfortunately that doesn't work either.

I just get sent to http://localhost/people/<id>/note/save with none of my logs / dumps written to stdout (not that I can see at least) and I am never redirected.

I suppose without the ability to debug this my run with Monica is over. Shame.

If anyone has any input or ideas I'd love to hear them.

BrunoMRP commented 7 years ago

I can debug Monica just fine trough PHPStorm with Monica running in Laravel Homestead. Would this work, or am i missing something?

phrohdoh commented 7 years ago

It will probably work but to be frank I'm not going to setup Homestead and Vagrant to develop a single project.

BrunoMRP commented 7 years ago

It's really simple actually, and it is the recommended way to develop Monica anyway.

phrohdoh commented 7 years ago

Ok so the issue turned out to be that .env wasn't ADDed into the docker container.

I believe this is because the .dockerignore contains .env. This should probably not be the case.

At any rate I seem to have developed out a workflow that is allowing me to make progress on this.

Will report back when I have something to show.

phrohdoh commented 7 years ago

@djaiss please provide feedback on this when you have time:

    /**
     * Store a newly created resource in storage.
     *
     * @param NotesRequest $request
     * @param Contact $contact
     * @return \Illuminate\Http\Response
     */
    public function store(NotesRequest $request, Contact $contact)
    {
        $account_id = $contact->account_id;

        $body_arr = $request->only(['body']);
        $note = $contact->notes()->create(
            $body_arr + ['account_id' => $account_id]
        );

        $contact->logEvent('note', $note->id, 'create');

        $event_type = 'note.new';

        $webhook = Webhook::where([
            ['account_id', '=', $account_id],
            ['event_type', '=', $event_type],
            ['is_active',  '=', 1],
        ])->first();

        if ($webhook === null)
        {
            Log::debug("An active webhook for [event '$event_type', account $account_id] was not found.");
        }
        else
        {
            $url = $webhook->endpoint();
            $note_text = $body_arr['body'];

            $data = [
                'account' => $account_id,
                'text' => $note_text,
                'event_type' => $event_type,
            ];

            $client = new \GuzzleHttp\Client();
            $client->postAsync($url, ['body' => json_encode($data)])->then(function ($resp) {
                // TODO: We should probably log responses
                // or at least whether we got a response or not which may indicate failure on our side
            });
        }

        return redirect('/people/' . $contact->id)
            ->with('success', trans('people.notes_create_success'));
    }
BranTurner commented 7 years ago

@Phrohdoh If you have to add the webhook to every controller I think you're at risk of bloating the controllers. Perhaps using middleware to manage webhooks would be more appropriate.

It might also be easier to make $event_type equal to the name of the route that is being used. In the case above, it would be equal to people.notes.store as defined in routes.php. I only mention this as you can dynamically get the route name from middleware using $request->route()->getName().

phrohdoh commented 7 years ago

@BranTurner Ooh that does look nice. I like it. Will investigate.

Do you have any feedback on the stubbed out design above, though?

Eventually I think we could grow a 'marketplace' (similar to GitHub and countless other online services) that could be given access to certain scopes (which could be the route name + whatever else is necessary).

BranTurner commented 7 years ago

Seems okay - can't see any major problems!

Regarding middleware, it might be better to fire an event after every task (e.g. note creation, add person, etc.). The webhook "controller" can then be added as a wildcard listener so it fires after every event. It seems slightly more flexible. It also means the Guzzle request would be queued to avoid increasing response times.

And the marketplace idea is interesting, it would provide a lot of utility!

djaiss commented 7 years ago

Love the discussion here!

For the marketplace, we'd have to see how it would work since Monica is not a central service but can be downloaded by anyone...