[2.0] Allow logging webhook triggers

phroggyy commented 8 years ago

This is a fairly large PR, and it does a few things aside from what the title says:

[x] Allow users to enable logging of webhooks (defaults to active with a 50 logs/hook limit)
[x] Make code more testable by clearer separation
[x] Separate the execution of the event handler out into a separate job
[x] New migration system to allow creating multiple migrations, this will make upgrading easier as it just requires publishing the latest files

Upgrade notes

This introduces a logger as part of the Job. This means that users need to run the latest migration, by first publishing the latest migration files and calling php artisan migrate.

No need for a custom service provider, you now declare your listeners in the config file!
Request data is now not automatically JSON encoded, this is instead done by the transformer in config. By default, the function is a simple JSON encode.

Final comments

The package is still lacking some structure. However, I did not want to restructure the entire application in this pull request. In the future, a goal should be to get rid of getters and setters only used for testing purposes, and instead resolved out of the IoC container, for a start. Abstracting out Guzzle might also be useful for testing purposes, and same goes for the logging module.

codecov-io commented 8 years ago

Current coverage is `93.63%`

Merging #14 into master will decrease coverage by -0.38% as of 53585e8

@@            master     #14   diff @@
======================================
  Files            5       7     +2
  Stmts          117     157    +40
  Branches         0       0       
  Methods         22      25     +3
======================================
+ Hit            110     147    +37
  Partial          0       0       
- Missed           7      10     +3

Review entire Coverage Diff as of 53585e8

Powered by Codecov. Updated on successful CI builds.

mpociot commented 8 years ago

First of all - thank you for this PR.

There are a few things that I'd like to be changed prior to merging it and tagging it 2.0.

The log model is called CaptainHookLog while the hook model is called Webhook (same for the tables). Please adjust the log model (and table) name to WebhookLog so it remains consistent with the existing model.
Please add a hasMany relation between the Webhook and the WebhookLog models.

I think it might be better to have control of how many logs should be stored per webhook and not overall. Especially in a multi-tenant setup I want to be able to see the latest logs and don't have them cleaned up because some other tenant fired a hook.

So instead of configuring the storage_time I think it's better to configure the max amount of logs per webhook. This way it would be possible to always have at least one log of the last called hook.

What do you think of this?

phroggyy commented 8 years ago

Right, so you'd make the times an array with the webhook FQCN as key and amount as value? ti 26. tammikuuta 2016 klo 20.13 Marcel Pociot notifications@github.com kirjoitti:

First of all - thank you for this PR.

There are a few things that I'd like to be changed prior to merging it and tagging it 2.0.

-

The log model is called CaptainHookLog while the hook model is called Webhook (same for the tables). Please adjust the log model (and table) name to WebhookLog so it remains consistent with the existing model.

Please add a hasMany relation between the Webhook and the WebhookLog models.

I think it might be better to have control of how many logs should be stored per webhook and not overall. Especially in a multi-tenant setup I want to be able to see the latest logs and don't have them cleaned up because some other tenant fired a hook.

So instead of configuring the storage_time I think it's better to configure the max amount of logs per webhook. This way it would be possible to always have at least one log of the last called hook.

What do you think of this?

— Reply to this email directly or view it on GitHub https://github.com/mpociot/captainhook/pull/14#issuecomment-175185402.

mpociot commented 8 years ago

No I wouldn't make it that complicated. Just a single value to define how many logs per webhook will be stored.

phroggyy commented 8 years ago

Ah, sorry, misunderstood ya, so a single number per webhook :) I'll get that done

mpociot commented 8 years ago

:tada: thanks for this PR!

mpociot / captainhook