Implement retries #4
Conversation
| public function up(): void | ||
| { | ||
| Schema::table('magento_bulk_requests', function (Blueprint $table): void { | ||
| $table->unsignedBigInteger('retry_of')->after('id')->nullable(); |
There was a problem hiding this comment.
Would be nice if you could add a foreign key constraint that will set the value to NULL if the relation has been removed.
src/Actions/RetryBulkRequest.php
Outdated
| } | ||
|
|
||
| $payload[] = $request; | ||
| if ($operation->subject !== null) { |
There was a problem hiding this comment.
You are always adding the subject, even if it's null, so this check should not be needed. Does this not cause any unwanted side effects with e.g. events?
|
|
||
| class RetryBulkRequestCommand extends Command | ||
| { | ||
| protected $signature = 'magento:async:retry-bulk-request {id} {--only-failed}'; |
There was a problem hiding this comment.
In most cases, the --only-failed is preferable. Perhaps rename it to --all which would by false without any options? This way, you could only retry the failed transactions by default.
There was a problem hiding this comment.
Then I'd prefer to default it to true (which the Nova implementation does too), the action isn't called RetryFailed. This is just an optional filter.
There was a problem hiding this comment.
Fair! In that case, it can remain what it is
src/Actions/RetryBulkRequest.php
Outdated
| 'POST' => $pendingRequest->postBulk($bulkRequest->path, $payload), | ||
| 'PUT' => $pendingRequest->putBulk($bulkRequest->path, $payload), | ||
| 'DELETE' => $pendingRequest->deleteBulk($bulkRequest->path, $payload), | ||
| default => null, |
There was a problem hiding this comment.
I'd personally prefer an exception if an unsupported method would be used
src/Actions/RetryBulkRequest.php
Outdated
| }; | ||
|
|
||
| if ($retry !== null) { | ||
| $retry->update([ |
There was a problem hiding this comment.
I think you should be able use your relation for this.
$bulkRequest->retries()->save($retry);| ]), | ||
| ])->preventStrayRequests(); | ||
|
|
||
| foreach (['POST', 'PUT', 'DELETE'] as $method) { |
There was a problem hiding this comment.
I would prefer to see a data provider for this
| /** @var RetryBulkRequest $action */ | ||
| $action = app(RetryBulkRequest::class); | ||
|
|
||
| $action->retry($request, false); |
There was a problem hiding this comment.
You could additionally check if the returned value is NULL
| /** @var RetryBulkRequest $action */ | ||
| $action = app(RetryBulkRequest::class); | ||
|
|
||
| $action->retry($request, false); |
| '--only-failed' => true, | ||
| ]); | ||
|
|
||
| $this->assertFalse(is_int($result)); |
There was a problem hiding this comment.
I'd recommend to use doc blocks instead:
/** @var PendingCommand $artisan */
$artisan = $this->artisan(RetryBulkRequestCommand::class, [
'id' => $request->id,
'--only-failed' => true,
]);
2 participants
No description provided.