feat: Add offer providers - #1
Conversation
The discounts page had nothing behind it, so a product that already knew how to discount something could not show or create one here. A host registers a provider that answers what exists, makes one, and stops one, and declares the fields it needs so the console renders a form it can submit. Whatever stores the discount stays the host's business, which is what lets one product use coupons and another use credits. Registering nothing is still a supported state and answers with an empty list.
|
This repository is not connected to any of your workspaces. Please connect it at https://app.sourceant.ai to get reviews on it. |
An expiry built with the date library an application already uses was refused, so a provider could not return one at all. Either form is accepted now. A provider class that was misspelled or did not implement the contract disappeared without a word, and two providers claiming the same key quietly replaced one another. Both are refused with a message naming what is wrong. Validation for a new offer can be replaced the way the other requests here can, so a host can ask for its own fields without taking over the controller.
Code Review Summary✨ Adds an Contracts and support — New HTTP surface — Config and docs — Tests — |
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
|
|
||
| public function createOffer(Request $request, OfferRegistry $offers, string $provider): JsonResponse | ||
| { | ||
| $validated = app(config('admin.requests.create_offer'))->validated(); |
There was a problem hiding this comment.
config('admin.requests.create_offer') has no fallback, but AdminServiceProvider::register() only does a shallow mergeConfigFrom. A host that published config/admin.php in 0.1.0 has its own requests array (update_mail_template only), which wins wholesale over the package default, so this key is null and app(null) throws a BindingResolutionException on the first POST to offers/{provider} — a 500 for every upgrading host. Default to the shipped request class so the endpoint keeps working while still allowing a host override.
| $validated = app(config('admin.requests.create_offer'))->validated(); | |
| $validated = app(config('admin.requests.create_offer', \Whilesmart\Admin\Http\Requests\CreateOfferRequest::class))->validated(); |
| use Carbon\Carbon; | ||
|
|
There was a problem hiding this comment.
?Carbon (i.e. Carbon\Carbon) rejects every other date a host may already hold: Carbon\CarbonImmutable, DateTimeImmutable and plain DateTime are not subclasses of Carbon\Carbon and raise a TypeError at the constructor. Illuminate\Support\Carbon happens to work because it extends Carbon\Carbon, which is why the test only covers that case. Widening to DateTimeInterface costs nothing — DateTimeInterface::ATOM produces exactly the same string as toIso8601String() (the existing assertion 2026-01-01T00:00:00+00:00 still passes). Note this also replaces the now-unused use Carbon\Carbon; import, otherwise Pint's unused-import rule will fail.
| use Carbon\Carbon; | |
| use DateTimeInterface; | |
| /** | |
| * One discount, as the console shows it. A provider keeps whatever else its | |
| * own storage holds. | |
| */ | |
| final class Offer | |
| { | |
| /** | |
| * @param string $value Already formatted, because only the provider knows | |
| * whether 20 means a percentage, pennies, or seats. | |
| * @param DateTimeInterface|null $expiresAt Any date a provider hands over. | |
| * @param array<string, mixed> $meta | |
| */ | |
| public function __construct( | |
| public readonly string $id, | |
| public readonly string $code, | |
| public readonly string $value, | |
| public readonly ?DateTimeInterface $expiresAt = null, | |
| public readonly ?int $redemptions = null, | |
| public readonly ?int $maxRedemptions = null, | |
| public readonly bool $active = true, | |
| public readonly array $meta = [], | |
| ) {} | |
| /** @return array<string, mixed> */ | |
| public function toArray(): array | |
| { | |
| return [ | |
| 'id' => $this->id, | |
| 'code' => $this->code, | |
| 'value' => $this->value, | |
| 'expires_at' => $this->expiresAt?->format(DateTimeInterface::ATOM), |
| @@ -17,6 +19,48 @@ | |||
|
|
|||
| class AdminController extends Controller | |||
| { | |||
There was a problem hiding this comment.
The new write endpoints bypass the owner authorization the existing write path enforces. PUT admin/mail-templates/{key} resolves UpdateMailTemplateRequest, whose authorize() calls app(OwnerAuthorizer::class)->authorize($this->user(), $template->owner_type, $template->owner_id), and every mail-template read goes through authorizeTemplate(). CreateOfferRequest::authorize() returns true unconditionally, and revokeOffer() has no request class at all, so with the default admin.write_middleware (an empty array) any authenticated principal can POST /api/admin/offers/{provider} or DELETE /api/admin/offers/{provider}/{id} and receive 201/200 where the equivalent mail-template write returns 403. Triggering call: POST /api/admin/offers/coupons with {'attributes': {'code': 'PILOT20', 'percent_off': 20}} as a user the host's OwnerAuthorizer denies → offer is created instead of rejected. Guarding in the controller (rather than in CreateOfferRequest::authorize()) also survives a host swapping admin.requests.create_offer for a permissive request class, which StrictCreateOfferRequest shows hosts do.
Note the same gap exists on GET offers (lines 27-38): GET mail-templates is filtered through OwnerAuthorizer::scope and GET mail-templates/{key} aborts when authorizeTemplate denies, while the offers listing returns every provider's contents unfiltered.
| { | |
| public function createOffer(Request $request, OfferRegistry $offers, string $provider): JsonResponse | |
| { | |
| $this->authorizeOffers($request); | |
| $validated = app(config('admin.requests.create_offer'))->validated(); | |
| $offer = $this->offerProvider($offers, $provider)->create($validated['attributes']); | |
| return response()->json(['success' => true, 'data' => $offer->toArray()], 201); | |
| } | |
| public function revokeOffer(Request $request, OfferRegistry $offers, string $provider, string $id): JsonResponse | |
| { | |
| $this->authorizeOffers($request); | |
| $this->offerProvider($offers, $provider)->revoke($id); | |
| return response()->json(['success' => true]); | |
| } | |
| private function authorizeOffers(Request $request): void | |
| { | |
| $owner = config('admin.owner'); | |
| abort_unless(app(OwnerAuthorizer::class)->authorize( | |
| $request->user(), | |
| $owner['type'], | |
| $owner['id'], | |
| )); | |
| } |
|
|
||
| class CreateOfferRequest extends FormRequest | ||
| { | ||
| public function authorize(): bool |
There was a problem hiding this comment.
CreateOfferRequest is the write-side counterpart of UpdateMailTemplateRequest, but the two disagree about authorization. The existing request authorizes against the configured owner:
return app(OwnerAuthorizer::class)->authorize($this->user(), $template->owner_type, $template->owner_id);and AdminController::authorizeTemplate() does the same for reads. Here the parallel request authorizes unconditionally, and revokeOffer does not go through a request object at all. The consequence is concrete: a host that binds OwnerAuthorizer — documented in the README as the protection for the configured admin owner — gets 403 from PUT mail-templates/{key} for a denied user, but POST offers/{provider} and DELETE offers/{provider}/{id} still succeed. admin.write_middleware defaults to [], so it is not standing in for the owner check.
If offers are intentionally outside owner scope (the provider is expected to scope them), that should be stated next to the OfferProvider contract; otherwise run the same owner authorization the sibling write performs.
| public function authorize(): bool | |
| public function authorize(): bool | |
| { | |
| $owner = config('admin.owner', ['type' => 'platform', 'id' => 0]); | |
| return app(\Whilesmart\OwnerAccess\Contracts\OwnerAuthorizer::class) | |
| ->authorize($this->user(), $owner['type'], $owner['id']); | |
| } |
| return true; | ||
| } | ||
|
|
||
| /** Only the shape. What each field means is the provider's to say. */ |
There was a problem hiding this comment.
OfferField exists so a provider can declare what create() needs (required, type, options) and the console can render a form from it, but nothing on the server enforces that declaration: this request validates only the attributes envelope. The established implementation of the same responsibility, UpdateMailTemplateRequest::rules(), validates every field it declares (enabled, subject, body, cta_label, cta_url).
As written, POST offers/coupons {"attributes": {}} validates, reaches OfferProvider::create([]), and every provider has to re-validate or fail with an undefined-key warning (the shipped CouponOfferProvider does $attributes['code'] directly). Building the field rules from the provider's own declaration keeps the two implementations consistent and makes OfferField::$required more than a rendering hint.
| /** Only the shape. What each field means is the provider's to say. */ | |
| /** The envelope shape, plus the constraint each declared field asks for. */ | |
| public function rules(): array | |
| { | |
| $rules = ['attributes' => ['required', 'array']]; | |
| $registry = app(\Whilesmart\Admin\Support\OfferRegistry::class); | |
| $key = (string) $this->route('provider'); | |
| $provider = $registry->has($key) ? $registry->get($key) : null; | |
| foreach ($provider?->fields() ?? [] as $field) { | |
| $rules["attributes.{$field->name}"] = array_values(array_filter([ | |
| $field->required ? 'required' : 'nullable', | |
| match ($field->type) { | |
| 'number' => 'numeric', | |
| 'boolean' => 'boolean', | |
| 'date' => 'date', | |
| 'select' => 'in:'.implode(',', array_keys($field->options)), | |
| default => 'string', | |
| }, | |
| ])); | |
| } | |
| return $rules; | |
| } |
Any authenticated caller could create or revoke a discount, and read every registered provider's contents, where the equivalent mail template write is refused for the same caller. The three endpoints now answer to the configured owner the way the rest of the console does, checked before the request object so a host replacing that object cannot drop the check. An expiry had to be one particular date class. Immutable dates and plain ones were refused at the door, which ruled out what many applications already hold. Any date is taken now. A host that published this package's configuration before offers existed has a request map that replaces the one shipped here, so creating a discount resolved nothing and failed. It falls back to the request this package ships.
| ->assertJsonPath('data.0.offers', []); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Data provider metadata is declared as a doc-comment (@dataProvider) while the rest of the suite uses PHPUnit attributes (AdminAuthorizationTest imports PHPUnit\Framework\Attributes\Test and uses #[Test]). Doc-comment metadata is deprecated in current PHPUnit releases and removed in newer ones, where an unrecognised @dataProvider no longer injects arguments and the test fails with an ArgumentCountError instead of running five date cases. Using the attribute keeps the metadata version-proof and consistent with the sibling test. The fully qualified form below needs no new import; adding use PHPUnit\Framework\Attributes\DataProvider; lets you shorten it to #[DataProvider('expiryDates')].
| /** | |
| #[\PHPUnit\Framework\Attributes\DataProvider('expiryDates')] |
| ->assertJsonPath('data.0.offers', []); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The suite declares its PHPUnit metadata as attributes (#[Test] in tests/Feature/AdminAuthorizationTest.php), while this new data provider is the legacy doc-comment annotation form. Doc-comment metadata is deprecated in PHPUnit 11 and no longer honoured by newer runners; on such a version the provider is ignored and this case fails with a missing $make argument instead of running the five date shapes it was written for. Declaring it with the attribute keeps the file consistent with the rest of the suite and guarantees the cases actually execute. Add use PHPUnit\Framework\Attributes\DataProvider; next to the other imports.
| /** | |
| + #[DataProvider('expiryDates')] | |
| + public function test_any_date_a_host_holds_can_be_an_expiry(callable $make): void |
1 participant
Add an
OfferProviderseam so a host can list and create its own discounts from the console. Register one underadmin.offer_providersand the discounts page shows what it holds, with a form built from the fields it declares.Three endpoints:
GET offers,POST offers/{provider},DELETE offers/{provider}/{id}. The last two sit behindadmin.write_middleware.The page shipped with nothing behind it and told the reader to register a provider, which did not exist. Registering none is still supported and answers with an empty list.
Offer::$valueis a formatted string, because only the provider knows whether 20 is a percentage, pennies or seats.