0
\$\begingroup\$

In my app users can create and soft-delete payment gateways something like Paypal. Users of a website can connect to the gateway and make payments.
I've written the logic in a transaction also locking a matching record (on website field) which was soft-deleted by the user because of race-condition. Read code for a better understanding.

Already exists (soft-deleted)
These gateways must be verified by admin, if the user submits different website and description fields in comparison to the matching record which was already verified, set is_verified and rejection_reason to null

is_verified values:

  • true verified
  • false rejected
  • null pending

The Rule: Create if not exists, Restore if soft-deleted.

Somehow i think the controller can be simplified and refactored. Performance and Consistency both are important for me.

Migration

Schema::create('gateways', function (Blueprint $table) {
    $table->id();
    $table->foreignId('user_id')
        ->constrained()
        ->onUpdate('cascade')
        ->onDelete('cascade');
    $table->string('title', 50);
    $table->text('description');
    $table->string('website')->unique();
    $table->enum('wage', Gateway::getWageKeys());
    $table->text('rejection_reason')->nullable();
    $table->boolean('is_verified')->nullable()->default(null);
    $table->boolean('is_blocked')->default(false);
    $table->boolean('first_payment')->default(false);
    $table->dateTime('free_until')->nullable();
    $table->softDeletes();
    $table->timestamps();
});

Controller

/**
 * Create a new gateway.
 * 
 * @param  \App\Http\Requests\StoreGatewayRequest  $request
 * @return \App\Http\Resources\GatewayResource
 */
public function create(StoreGatewayRequest $request)
{
    try {
        $gateway = DB::transaction(function () use ($request) {
            $gateway = $request->user()->gateways()->onlyTrashed()
                                   ->where('website', $request->website)->lockForUpdate()->first();

            if (! $gateway) {
                return $request->user()->gateways()->create($request->validated());
            } else {
                if ($gateway->is_blocked)
                    abort(452);

                $gateway->fill($request->validated());

                if ($gateway->isDirty('website') || $gateway->isDirty('description')) {
                    $gateway->rejection_reason = null;
                    $gateway->is_verified = null;
                }

                $gateway->deleted_at = null;
                $gateway->save();

                return $gateway;
            }
        });
    } catch (QueryException $e) {
        if ($e->errorInfo[1] === 1062) {
            abort(409);
        } else {
            throw $e;
        }
    }

    return new GatewayResource($gateway);
}
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$
  1. I'd personally suggest separating a logic into different files.

i.e. Controller would look something like below if you remove the business logic.

public function create(StoreGatewayRequest $request)
{
    $gateway = app(IGatewayCreator::class)->handle($request->validated, $request->user());

    return new GatewayResource($gateway);
}

This way within controller class is easier to understand and test.

  1. The next part that we have here is related to the exception handling. We can leverage Laravel's Exception handler to handle this for us - https://laravel.com/docs/8.x/errors

  2. Within your logic we have two concrete strategies, one for when we create a new record and another for when we handle soft deleted records. Here we can separate concerns further.

class GatewayCreator implements IGatewayCreator
{
  // i've used $dto here instead of an array, 
  // since with DTO you can check what attributes are within the DTO
  public function handle(GatewayDTO $dto, User $user): Gateway
  {
    $strategy = $this->strategy($dto->website, $user);

    return app($strategy)->handle($dto, $user);
  }

  protected function strategy(string $website, User $user): string 
  {
    $recordExists = $user->gateways()
      ->onlyTrashed()
      ->hasWebsite($website) // local scope
      ->exists();

    return $recordExists 
      ? IStoreSoftDeletedGateway::class 
      : IStoreNewGateway::class;
  }
}
  1. The strategy for the IStoreNewGateway is self-explanatory, just use a store within database transactions

  2. The strategy for IStoreSoftDeletedGateway will have other logic.

public function handle(GatewayDTO $dto, User $user): Gateway
{
  $gateway = $user->gateways()
    ->onlyTrashed()
    ->hasWebsite($dto->website)
    ->lockForUpdate()
    ->first();

  if ($gateway->is_blocked) {
    throw new GatewayRecordIsLockedException();
  }

  return DB::transaction(function () use (Gateway $gateway, GatewayDTO $dto) {
    // for unit testing you can probably extract lines below to a different method.
    $gateway->fill($dto->toArray());
    // just set rejection_reason and is_verified to null without checking for dirty
    $gateway->resetRecord(); 

    $gateway->deleted_at = null;
    $gateway->save();

    return $gateway;
  });
}

NOTES

  1. I don't understand why you have lockForUpdate
  2. It's not clear why you had race conditions, shouldn't this be something to deal with on FE? You can also consider queueing requests.
  3. It's also not clear what performance issues you've experienced.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.