-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Plugin error logging #19
Changes from all commits
1556980
e4f7b84
dc8b308
438f084
bee1ef2
01435b5
5e3b184
211b687
2667063
419fe20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
<?php | ||
|
||
namespace App\Events; | ||
|
||
use App\Models\PluginError\PluginError; | ||
|
||
class PluginErrorReceivedEvent | ||
{ | ||
/** | ||
* @var PluginError | ||
*/ | ||
private $error; | ||
|
||
public function __construct(PluginError $error) | ||
{ | ||
$this->error = $error; | ||
} | ||
|
||
/** | ||
* @return PluginError | ||
*/ | ||
public function getError(): PluginError | ||
{ | ||
return $this->error; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?php | ||
|
||
namespace App\Exceptions; | ||
|
||
use Exception; | ||
|
||
/** | ||
* Class PluginErrorException | ||
* Custom exception to be passed to bugsnag to record the fact a new plugin | ||
* error has come in. | ||
*/ | ||
class PluginErrorException extends Exception | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
<?php | ||
|
||
namespace App\Http\Controllers; | ||
|
||
use App\Events\PluginErrorReceivedEvent; | ||
use App\Models\PluginError\PluginError; | ||
use Illuminate\Http\Request; | ||
use Illuminate\Http\Response; | ||
use Illuminate\Support\Facades\Auth; | ||
use Illuminate\Support\Facades\Event; | ||
|
||
class PluginErrorController extends BaseController | ||
{ | ||
/** | ||
* Records a plugin error | ||
* | ||
* @return Response | ||
*/ | ||
public function recordPluginError(Request $request) : Response | ||
{ | ||
$dataCheck = $this->checkForSuppliedData( | ||
$request, | ||
[ | ||
'user_report' => 'required|boolean', | ||
'data' => 'required|array', | ||
] | ||
); | ||
|
||
if ($dataCheck) { | ||
return response(null, 400); | ||
} | ||
|
||
$pluginError = PluginError::create( | ||
[ | ||
'user_id' => Auth::user()->id, | ||
'user_report' => $request->json('user_report'), | ||
'data' => json_encode($request->json('data')), | ||
] | ||
); | ||
|
||
Event::fire(new PluginErrorReceivedEvent($pluginError)); | ||
return response('', 204); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?php | ||
|
||
|
||
namespace App\Listeners\PluginError; | ||
|
||
use App\Events\PluginErrorReceivedEvent; | ||
use App\Exceptions\PluginErrorException; | ||
use Bugsnag\BugsnagLaravel\Facades\Bugsnag; | ||
|
||
class RecordPluginErrorInBugsnag | ||
{ | ||
/** | ||
* Handle any squawk allocation event | ||
* | ||
* @param PluginErrorReceivedEvent $errorReceivedEvent | ||
* @return bool | ||
*/ | ||
public function handle(PluginErrorReceivedEvent $errorReceivedEvent) : bool | ||
{ | ||
Bugsnag::notifyException(new PluginErrorException('New plugin error reported')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't required; Bugsnag will catch any exception that bubbles up to the |
||
return false; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?php | ||
|
||
namespace App\Models\PluginError; | ||
|
||
use Illuminate\Database\Eloquent\Model; | ||
|
||
class PluginError extends Model | ||
{ | ||
protected $table = 'plugin_error'; | ||
|
||
public $timestamps = true; | ||
|
||
/** | ||
* The attributes that are mass assignable. | ||
* | ||
* @var array | ||
*/ | ||
protected $fillable = [ | ||
'user_id', | ||
'user_report', | ||
'data', | ||
]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
<?php | ||
|
||
use Illuminate\Support\Facades\Schema; | ||
use Illuminate\Database\Schema\Blueprint; | ||
use Illuminate\Database\Migrations\Migration; | ||
|
||
class CreatePluginErrorsTable extends Migration | ||
{ | ||
/** | ||
* Run the migrations. | ||
* | ||
* @return void | ||
*/ | ||
public function up() | ||
{ | ||
Schema::create('plugin_error', function (Blueprint $table) { | ||
$table->increments('id'); | ||
$table->unsignedInteger('user_id')->comment('The user who the report is attributed to'); | ||
$table->boolean('user_report')->comment('Whether the report was generated by the user or the plugin'); | ||
$table->json('data')->comment('Report data'); | ||
$table->timestamps(); | ||
|
||
$table->foreign('user_id') | ||
->references('id')->on('user'); | ||
}); | ||
} | ||
|
||
/** | ||
* Reverse the migrations. | ||
* | ||
* @return void | ||
*/ | ||
public function down() | ||
{ | ||
Schema::dropIfExists('plugin_error'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?php | ||
|
||
namespace App\Events; | ||
|
||
use App\BaseUnitTestCase; | ||
use App\Models\PluginError\PluginError; | ||
|
||
class PluginErrorReceivedEventTest extends BaseUnitTestCase | ||
{ | ||
public function testItConstructs() | ||
{ | ||
$this->assertInstanceOf(PluginErrorReceivedEvent::class, new PluginErrorReceivedEvent(new PluginError)); | ||
} | ||
|
||
public function testItCanReturnTheError() | ||
{ | ||
$error = new PluginError; | ||
$event = new PluginErrorReceivedEvent($error); | ||
$this->assertEquals($error, $event->getError()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
<?php | ||
|
||
namespace App\Http\Controllers; | ||
|
||
use App\BaseApiTestCase; | ||
use App\Models\PluginError\PluginError; | ||
|
||
class PluginErrorControllerTest extends BaseApiTestCase | ||
{ | ||
public function testItConstructs() | ||
{ | ||
$this->assertInstanceOf(PluginErrorController::class, $this->app->make(PluginErrorController::class)); | ||
} | ||
|
||
public function testItReturns400OnMissingUserReport() | ||
{ | ||
$this->makeAuthenticatedApiRequest( | ||
self::METHOD_POST, | ||
'plugin-error', | ||
[ | ||
'data' => [ | ||
'foo' => 'bar' | ||
], | ||
] | ||
)->seeStatusCode(400); | ||
} | ||
|
||
public function testItReturns400OnMissingData() | ||
{ | ||
$this->makeAuthenticatedApiRequest( | ||
self::METHOD_POST, | ||
'plugin-error', | ||
[ | ||
'user_report' => true, | ||
] | ||
)->seeStatusCode(400); | ||
} | ||
|
||
public function testItReturnsNoContentOnSuccess() | ||
{ | ||
$this->makeAuthenticatedApiRequest( | ||
self::METHOD_POST, | ||
'plugin-error', | ||
[ | ||
'user_report' => true, | ||
'data' => [ | ||
'foo' => 'bar', | ||
'baz' => [ | ||
'foo' => 'bar', | ||
] | ||
], | ||
] | ||
)->seeStatusCode(204); | ||
} | ||
|
||
public function testItStoresUserReports() | ||
{ | ||
$data = [ | ||
'baz' => [ | ||
'foo' => 'bar', | ||
], | ||
'foo' => 'bar', | ||
]; | ||
|
||
$this->makeAuthenticatedApiRequest( | ||
self::METHOD_POST, | ||
'plugin-error', | ||
[ | ||
'user_report' => true, | ||
'data' => $data | ||
] | ||
); | ||
|
||
$pluginError = PluginError::orderBy('created_at', 'desc')->first(); | ||
$this->assertEquals(self::ACTIVE_USER_CID, $pluginError->user_id); | ||
$this->assertEquals(1, $pluginError->user_report); | ||
$this->assertEquals($data, json_decode($pluginError->data, true)); | ||
} | ||
|
||
public function testItStoresNonUserReports() | ||
{ | ||
$data = [ | ||
'baz' => [ | ||
'foo' => 'bar', | ||
], | ||
'foo' => 'bar', | ||
]; | ||
|
||
$this->makeAuthenticatedApiRequest( | ||
self::METHOD_POST, | ||
'plugin-error', | ||
[ | ||
'user_report' => false, | ||
'data' => $data | ||
] | ||
); | ||
|
||
$pluginError = PluginError::orderBy('created_at', 'desc')->first(); | ||
$this->assertEquals(self::ACTIVE_USER_CID, $pluginError->user_id); | ||
$this->assertEquals(0, $pluginError->user_report); | ||
$this->assertEquals($data, json_decode($pluginError->data, true)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
<?php | ||
|
||
namespace App\Listeners\PluginError; | ||
|
||
use App\BaseUnitTestCase; | ||
use App\Events\PluginErrorReceivedEvent; | ||
use App\Models\PluginError\PluginError; | ||
|
||
class RecordPluginErrorInBugsnagTest extends BaseUnitTestCase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this test class has much value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a stickler for high test coverage, that's why it exists xD There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If my Bugsnag comment above is addressed it should make this test (and the event) redundant |
||
{ | ||
public function testItConstructs() | ||
{ | ||
$listener = new RecordPluginErrorInBugsnag(); | ||
$this->assertInstanceOf(RecordPluginErrorInBugsnag::class, $listener); | ||
} | ||
|
||
public function testItStopsEventPropagation() | ||
{ | ||
$listener = new RecordPluginErrorInBugsnag(); | ||
$this->assertFalse($listener->handle(new PluginErrorReceivedEvent(new PluginError))); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure a 204 makes sense? The response should acknowledge the creation of an event (201) or confirm all is okay (200).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to 201