Skip to content

feat: Add a person as an offer field type - #5

Merged
nfebe merged 1 commit into
devfrom
feat/an-offer-field-for-a-person
Sep 23, 2026
Merged

nfebe merged 1 commit into
devfrom
feat/an-offer-field-for-a-person

Conversation

@nfebe

@nfebe nfebe commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

A provider can declare a field of type user, and the console searches its own directory for an address while still taking one typed in full.

A provider can declare a field of type user, and the console searches its own
directory for an address while still taking one typed in full.
@sourceant

sourceant Bot commented Sep 23, 2026

Copy link
Copy Markdown

Code Review Summary

✨ Adds a new 'user' field type to the OfferField contract in src/Contracts/OfferField.php, letting a provider declare a field whose value is an address the console can look up in its own directory while still accepting a fully typed-in address. The change is documentation-only at the type level: the @param union now advertises 'user' alongside the existing types, with an inline note explaining the resolution behaviour. tests/Feature/OfferProviderTest.php gains test_a_field_may_ask_for_a_person, which constructs an OfferField of type 'user' and checks the value survives into toArray(). The review found no functional defects; the only notes concern how much the new test actually pins down.

💡 Minor Suggestions

  • As written, the assertion only verifies that the constructor echoes back the value it was given, so it would still pass for any string (e.g. a typo like 'usr'). Asserting the complete toArray() payload makes the test a real regression guard for the DTO shape and the newly advertised 'user' type.
  • The new test verifies the contract at the producer boundary only through toArray()['type']. Asserting the full toArray() shape pins the output schema that the consumer (offer/field JSON) depends on, so any future change to the field keys or defaults is caught here rather than silently changing what consumers receive. This keeps the producer contract explicit without introducing a second formatter.

@sourceant sourceant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

type: 'user',
);

$this->assertSame('user', $field->toArray()['type']);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, the assertion only verifies that the constructor echoes back the value it was given, so it would still pass for any string (e.g. a typo like 'usr'). Asserting the complete toArray() payload makes the test a real regression guard for the DTO shape and the newly advertised 'user' type.

Suggested change
$this->assertSame('user', $field->toArray()['type']);
$this->assertSame([
'name' => 'for_email',
'label' => 'Only for',
'type' => 'user',
'required' => false,
'help' => null,
'options' => [],
], $field->toArray());

];
}

public function test_a_field_may_ask_for_a_person(): void

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test verifies the contract at the producer boundary only through toArray()['type']. Asserting the full toArray() shape pins the output schema that the consumer (offer/field JSON) depends on, so any future change to the field keys or defaults is caught here rather than silently changing what consumers receive. This keeps the producer contract explicit without introducing a second formatter.

Suggested change
public function test_a_field_may_ask_for_a_person(): void
public function test_a_field_may_ask_for_a_person(): void
{
$field = new OfferField(
name: 'for_email',
label: 'Only for',
type: 'user',
);
$this->assertSame([
'name' => 'for_email',
'label' => 'Only for',
'type' => 'user',
'required' => false,
'help' => null,
'options' => [],
], $field->toArray());
}

@nfebe
nfebe merged commit b9050bc into dev Sep 23, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant