diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index a16d44373..38e6adf46 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -59,6 +59,7 @@ jobs: - { name: "SummitRSVPServiceTest", filter: "--filter SummitRSVPServiceTest" } - { name: "SummitRSVPInvitationServiceTest", filter: "--filter SummitRSVPInvitationServiceTest" } - { name: "EntityModelUnitTests", filter: "tests/Unit/Entities/" } + - { name: "AuditUnitTests", filter: "tests/Unit/Audit/" } - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" } - { name: "AuditEventTypesTest", filter: "--filter AuditEventTypesTest" } - { name: "GuzzleTracingTest", filter: "--filter GuzzleTracingTest" } diff --git a/app/Events/SponsorServices/SummitSponsorshipAddOnCreatedEventDTO.php b/app/Events/SponsorServices/SummitSponsorshipAddOnCreatedEventDTO.php index 7e6ac6c2a..235ccc15a 100644 --- a/app/Events/SponsorServices/SummitSponsorshipAddOnCreatedEventDTO.php +++ b/app/Events/SponsorServices/SummitSponsorshipAddOnCreatedEventDTO.php @@ -58,6 +58,7 @@ public static function fromSponsorshipAddOn(SummitSponsorshipAddOn $add_on): sel $sponsorship = $add_on->getSponsorship(); $summit_sponsorship_type = $sponsorship->getType(); $sponsorship_type = $summit_sponsorship_type->getType(); + $sponsor = $sponsorship->getSponsor(); return new self( $add_on->getId(), @@ -67,8 +68,8 @@ public static function fromSponsorshipAddOn(SummitSponsorshipAddOn $add_on): sel $sponsorship_type->getName(), $add_on->getType(), $add_on->getName(), - $add_on->getSponsorship()->getSponsor()->getId(), - $add_on->getSponsorship()->getSponsor()->getSummitId(), + $sponsor?->getId() ?? 0, + $sponsor?->getSummitId() ?? 0, ); } diff --git a/app/Events/SponsorServices/SummitSponsorshipCreatedEventDTO.php b/app/Events/SponsorServices/SummitSponsorshipCreatedEventDTO.php index ed3d2e6b6..717eeffed 100644 --- a/app/Events/SponsorServices/SummitSponsorshipCreatedEventDTO.php +++ b/app/Events/SponsorServices/SummitSponsorshipCreatedEventDTO.php @@ -50,11 +50,11 @@ public static function fromSponsorship(SummitSponsorship $sponsorship): self return new self( $sponsorship->getId(), - $sponsor->getId(), + $sponsor?->getId() ?? 0, $summit_sponsorship_type->getId(), $sponsorship_type->getId(), $sponsorship_type->getName(), - $sponsor->getSummitId() + $sponsor?->getSummitId() ?? 0 ); } diff --git a/app/Jobs/Emails/Registration/PromoCodes/SponsorPromoCodeEmail.php b/app/Jobs/Emails/Registration/PromoCodes/SponsorPromoCodeEmail.php index 572a4b1e7..01e48933a 100644 --- a/app/Jobs/Emails/Registration/PromoCodes/SponsorPromoCodeEmail.php +++ b/app/Jobs/Emails/Registration/PromoCodes/SponsorPromoCodeEmail.php @@ -54,10 +54,10 @@ public function __construct $summit = $promo_code->getSummit(); $payload = []; $sponsor = $promo_code->getSponsor(); - $payload[IMailTemplatesConstants::sponsor_tier_name] = implode(',', $sponsor->getSponsorshipTierNames()); + $payload[IMailTemplatesConstants::sponsor_tier_name] = $sponsor ? implode(',', $sponsor->getSponsorshipTierNames()) : ''; $payload[IMailTemplatesConstants::promo_code] = $promo_code->getCode(); $payload[IMailTemplatesConstants::company_name] = ''; - $company = $sponsor->getCompany(); + $company = $sponsor?->getCompany(); if (!is_null($company)) $payload[IMailTemplatesConstants::company_name] = $company->getName(); @@ -97,4 +97,4 @@ public static function getEmailTemplateSchema(): array{ $payload[IMailTemplatesConstants::sponsor_tier_name]['type'] = 'string'; return $payload; } -} \ No newline at end of file +} diff --git a/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationDiscountCodeSerializer.php b/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationDiscountCodeSerializer.php index 40652f41e..87b7594b0 100644 --- a/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationDiscountCodeSerializer.php +++ b/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationDiscountCodeSerializer.php @@ -62,7 +62,9 @@ public function serialize($expand = null, array $fields = [], array $relations = } break; case 'sponsor_name':{ - $values['sponsor_name'] = $code->getSponsor()->getCompany()->getName(); + if($code->hasSponsor()) { + $values['sponsor_name'] = $code->getSponsor()->getCompany()->getName(); + } } break; } @@ -71,4 +73,4 @@ public function serialize($expand = null, array $fields = [], array $relations = return $values; } -} \ No newline at end of file +} diff --git a/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationPromoCodeSerializer.php b/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationPromoCodeSerializer.php index dab84efca..4b25e4a28 100644 --- a/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationPromoCodeSerializer.php +++ b/app/ModelSerializers/Summit/Registration/PromoCodes/SponsorSummitRegistrationPromoCodeSerializer.php @@ -63,7 +63,9 @@ public function serialize($expand = null, array $fields = [], array $relations = } break; case 'sponsor_name':{ - $values['sponsor_name'] = $code->getSponsor()->getCompany()->getName(); + if($code->hasSponsor()) { + $values['sponsor_name'] = $code->getSponsor()->getCompany()->getName(); + } } break; } @@ -72,4 +74,4 @@ public function serialize($expand = null, array $fields = [], array $relations = return $values; } -} \ No newline at end of file +} diff --git a/app/ModelSerializers/Summit/Registration/SponsorBadgeScanCSVSerializer.php b/app/ModelSerializers/Summit/Registration/SponsorBadgeScanCSVSerializer.php index 2c7e221c3..f6e399afe 100644 --- a/app/ModelSerializers/Summit/Registration/SponsorBadgeScanCSVSerializer.php +++ b/app/ModelSerializers/Summit/Registration/SponsorBadgeScanCSVSerializer.php @@ -65,6 +65,10 @@ public function serialize($expand = null, array $fields = array(), array $relati if (!$scan instanceof SponsorBadgeScan) return []; $values = parent::serialize($expand, $fields, $relations, $params); $sponsor = $scan->getSponsor(); + + //There are no sponsor questions to process without a sponsor + if (is_null($sponsor)) return $values; + $sponsor_questions = $sponsor->getExtraQuestions(); $setting = $sponsor->getSummit()->getLeadReportSettingFor($sponsor); $setting_columns = $setting->getColumns(); @@ -144,4 +148,4 @@ public function serialize($expand = null, array $fields = array(), array $relati return $values; } -} \ No newline at end of file +} diff --git a/app/ModelSerializers/Summit/Registration/SponsorUserInfoGrantCSVSerializer.php b/app/ModelSerializers/Summit/Registration/SponsorUserInfoGrantCSVSerializer.php index 59d932189..7d6d62d75 100644 --- a/app/ModelSerializers/Summit/Registration/SponsorUserInfoGrantCSVSerializer.php +++ b/app/ModelSerializers/Summit/Registration/SponsorUserInfoGrantCSVSerializer.php @@ -51,11 +51,14 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['notes'] = 'VIRTUAL'; $sponsor = $grant->getSponsor(); + + //There are no sponsor questions to process without a sponsor + if (is_null($sponsor)) return $values; + $sponsor_questions = $sponsor->getExtraQuestions(); $setting = $sponsor->getSummit()->getLeadReportSettingFor($sponsor); $setting_columns = $setting->getColumns(); - // remove not allowed string columns and sort them by setting columns order $new_values = []; foreach(array_values($setting_columns) as $column) { @@ -130,4 +133,4 @@ public function serialize($expand = null, array $fields = [], array $relations = return $values; } -} \ No newline at end of file +} diff --git a/app/Models/Foundation/Summit/ExtraQuestions/SummitSponsorExtraQuestionType.php b/app/Models/Foundation/Summit/ExtraQuestions/SummitSponsorExtraQuestionType.php index 2e28e1e7a..61deca119 100644 --- a/app/Models/Foundation/Summit/ExtraQuestions/SummitSponsorExtraQuestionType.php +++ b/app/Models/Foundation/Summit/ExtraQuestions/SummitSponsorExtraQuestionType.php @@ -35,16 +35,16 @@ class SummitSponsorExtraQuestionType extends ExtraQuestionType ]; /** - * @var Sponsor + * @var Sponsor|null */ #[ORM\JoinColumn(name: 'SponsorID', referencedColumnName: 'ID', onDelete: 'CASCADE')] #[ORM\ManyToOne(targetEntity: \models\summit\Sponsor::class, inversedBy: 'extra_questions')] private $sponsor; /** - * @return Sponsor + * @return Sponsor|null */ - public function getSponsor(): Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } @@ -61,4 +61,4 @@ public function clearSponsor(): void { $this->sponsor = null; } -} \ No newline at end of file +} diff --git a/app/Models/Foundation/Summit/Registration/PromoCodes/Traits/SponsorPromoCodeTrait.php b/app/Models/Foundation/Summit/Registration/PromoCodes/Traits/SponsorPromoCodeTrait.php index 813c45f1e..8dcd1ddcf 100644 --- a/app/Models/Foundation/Summit/Registration/PromoCodes/Traits/SponsorPromoCodeTrait.php +++ b/app/Models/Foundation/Summit/Registration/PromoCodes/Traits/SponsorPromoCodeTrait.php @@ -71,9 +71,9 @@ public function setContactEmail(string $contact_email): void } /** - * @return Sponsor + * @return Sponsor|null */ - public function getSponsor():Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } @@ -95,4 +95,4 @@ public function setSponsor(Sponsor $sponsor) public function checkSubject(string $email, ?string $company):bool{ return true; } -} \ No newline at end of file +} diff --git a/app/Models/Foundation/Summit/Registration/SponsorUserInfoGrant.php b/app/Models/Foundation/Summit/Registration/SponsorUserInfoGrant.php index 02ae8dbf5..9adfc7898 100644 --- a/app/Models/Foundation/Summit/Registration/SponsorUserInfoGrant.php +++ b/app/Models/Foundation/Summit/Registration/SponsorUserInfoGrant.php @@ -29,7 +29,7 @@ class SponsorUserInfoGrant extends SilverstripeBaseModel const ClassName = 'SponsorUserInfoGrant'; /** - * @var Sponsor + * @var Sponsor|null */ #[ORM\JoinColumn(name: 'SponsorID', referencedColumnName: 'ID')] #[ORM\ManyToOne(targetEntity: \models\summit\Sponsor::class, inversedBy: 'user_info_grants')] @@ -53,9 +53,9 @@ class SponsorUserInfoGrant extends SilverstripeBaseModel ]; /** - * @return Sponsor + * @return Sponsor|null */ - public function getSponsor(): Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } @@ -95,4 +95,4 @@ public function getAttendeeLastName():?string{ public function getAttendeeEmail():?string{ return $this->allowed_user->getEmail(); } -} \ No newline at end of file +} diff --git a/app/Models/Foundation/Summit/SponsorMaterial.php b/app/Models/Foundation/Summit/SponsorMaterial.php index fb0b039c7..00f552aa3 100644 --- a/app/Models/Foundation/Summit/SponsorMaterial.php +++ b/app/Models/Foundation/Summit/SponsorMaterial.php @@ -53,7 +53,7 @@ class SponsorMaterial extends SilverstripeBaseModel private $order; /** - * @var Sponsor + * @var Sponsor|null */ #[ORM\JoinColumn(name: 'SponsorID', referencedColumnName: 'ID', onDelete: 'CASCADE')] #[ORM\ManyToOne(targetEntity: \Sponsor::class, inversedBy: 'materials', fetch: 'EXTRA_LAZY')] @@ -144,9 +144,9 @@ public function setLink(string $link): void } /** - * @return Sponsor + * @return Sponsor|null */ - public function getSponsor(): Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } @@ -162,4 +162,4 @@ public function setSponsor(Sponsor $sponsor): void public function clearSponsor():void{ $this->sponsor = null; } -} \ No newline at end of file +} diff --git a/app/Models/Foundation/Summit/SponsorStatistics.php b/app/Models/Foundation/Summit/SponsorStatistics.php index 5d0d0c9da..1b6b2a0e3 100644 --- a/app/Models/Foundation/Summit/SponsorStatistics.php +++ b/app/Models/Foundation/Summit/SponsorStatistics.php @@ -27,7 +27,7 @@ class SponsorStatistics extends SilverstripeBaseModel use One2ManyPropertyTrait; /** - * @var Sponsor + * @var Sponsor|null */ #[ORM\JoinColumn(name: 'SponsorID', referencedColumnName: 'ID', onDelete: 'CASCADE')] #[ORM\OneToOne(targetEntity: Sponsor::class, inversedBy: 'sponsorservices_statistics', fetch: 'EXTRA_LAZY')] @@ -66,7 +66,7 @@ public function __construct() $this->documentsQty = 0; } - public function getSponsor(): Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } diff --git a/app/Models/Foundation/Summit/SummitLeadReportSetting.php b/app/Models/Foundation/Summit/SummitLeadReportSetting.php index 94235f809..7bee2df4e 100644 --- a/app/Models/Foundation/Summit/SummitLeadReportSetting.php +++ b/app/Models/Foundation/Summit/SummitLeadReportSetting.php @@ -29,7 +29,7 @@ class SummitLeadReportSetting extends SilverstripeBaseModel const SponsorExtraQuestionsKey = 'extra_questions'; /** - * @var Sponsor + * @var Sponsor|null */ #[ORM\JoinColumn(name: 'SponsorID', referencedColumnName: 'ID', onDelete: 'SET NULL')] #[ORM\OneToOne(targetEntity: \models\summit\Sponsor::class, inversedBy: 'lead_report_setting')] @@ -51,9 +51,9 @@ public function __construct() } /** - * @return Sponsor + * @return Sponsor|null */ - public function getSponsor(): Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } @@ -134,4 +134,4 @@ public function validateFor(Summit $summit, ?Sponsor $sponsor = null): void } } } -} \ No newline at end of file +} diff --git a/app/Models/Foundation/Summit/SummitSponsorship.php b/app/Models/Foundation/Summit/SummitSponsorship.php index 355236da6..0306523f7 100644 --- a/app/Models/Foundation/Summit/SummitSponsorship.php +++ b/app/Models/Foundation/Summit/SummitSponsorship.php @@ -39,7 +39,7 @@ class SummitSponsorship extends SilverstripeBaseModel ]; /** - * @var Sponsor + * @var Sponsor|null */ #[ORM\JoinColumn(name: 'SponsorID', referencedColumnName: 'ID')] #[ORM\ManyToOne(targetEntity: Sponsor::class)] @@ -64,7 +64,7 @@ public function __construct() $this->add_ons = new ArrayCollection(); } - public function getSponsor(): Sponsor + public function getSponsor(): ?Sponsor { return $this->sponsor; } @@ -133,4 +133,4 @@ public function setType(SummitSponsorshipType $type): void { $this->type = $type; } -} \ No newline at end of file +} diff --git a/tests/Unit/Audit/SummitSponsorExtraQuestionTypeAuditLogFormatterTest.php b/tests/Unit/Audit/SummitSponsorExtraQuestionTypeAuditLogFormatterTest.php new file mode 100644 index 000000000..c50aa12b8 --- /dev/null +++ b/tests/Unit/Audit/SummitSponsorExtraQuestionTypeAuditLogFormatterTest.php @@ -0,0 +1,216 @@ +clearSponsor(), + * which nulls the sponsor in-memory. On flush, AuditEventListener invokes + * this formatter, which calls $subject->getSponsor(). Before the fix, + * that method was typed `: Sponsor` (non-nullable) and threw TypeError + * because PHP refused to return null under a non-null return type. + * + * TypeError escaped the formatter's try/catch because it catches + * \Exception, but TypeError extends \Error (not \Exception), so the + * 500 reached the user. + * + * The fix widens getSponsor() to `: ?Sponsor`. These tests lock in that + * behavior: after clearSponsor() the formatter must return a non-null + * string without throwing, and fall back to the "Unknown Sponsor" label. + * + * @package Tests\Unit\Audit + */ +final class SummitSponsorExtraQuestionTypeAuditLogFormatterTest extends TestCase +{ + private const LABEL = 'What is your T-shirt size?'; + private const QUESTION_ID = 950; + private const SPONSOR_ID = 610; + + /** + * Build a SummitSponsorExtraQuestionType with the sponsor explicitly + * cleared - mirroring what Sponsor::removeExtraQuestion() does on the + * deletion path that caused the original production TypeError. + */ + private function makeQuestionWithClearedSponsor(): SummitSponsorExtraQuestionType + { + $company = new Company(); + $company->setName('Acme Corp'); + + $sponsor = new Sponsor(); + $sponsor->setCompany($company); + $this->setProtectedId($sponsor, self::SPONSOR_ID); + + $question = new SummitSponsorExtraQuestionType(); + $question->setName('tshirt_size'); + $question->setType(ExtraQuestionTypeConstants::TextQuestionType); + $question->setLabel(self::LABEL); + $question->setSponsor($sponsor); + $this->setProtectedId($question, self::QUESTION_ID); + + // Simulate what Sponsor::removeExtraQuestion() does: it calls + // clearSponsor() on the extra question to cut the bidirectional link + // BEFORE the flush reaches the audit listener. + $question->clearSponsor(); + + return $question; + } + + /** + * Build a SummitSponsorExtraQuestionType with an attached sponsor + * (happy path - sanity check that the formatter still works). + */ + private function makeQuestionWithSponsor(): SummitSponsorExtraQuestionType + { + $company = new Company(); + $company->setName('Acme Corp'); + + $sponsor = new Sponsor(); + $sponsor->setCompany($company); + $this->setProtectedId($sponsor, self::SPONSOR_ID); + + $question = new SummitSponsorExtraQuestionType(); + $question->setName('tshirt_size'); + $question->setType(ExtraQuestionTypeConstants::TextQuestionType); + $question->setLabel(self::LABEL); + $question->setSponsor($sponsor); + $this->setProtectedId($question, self::QUESTION_ID); + + return $question; + } + + /** + * BaseEntity::$id is protected with no public setter; use reflection + * so the formatter sees a stable numeric id in its output. + */ + private function setProtectedId(object $entity, int $id): void + { + $rc = new \ReflectionClass($entity); + while ($rc !== false && !$rc->hasProperty('id')) { + $rc = $rc->getParentClass(); + } + if ($rc === false) { + self::fail('Could not locate $id property on ' . get_class($entity)); + } + $prop = $rc->getProperty('id'); + $prop->setAccessible(true); + $prop->setValue($entity, $id); + } + + /** + * Reproduces the exact production failure: deletion flow where + * clearSponsor() has been invoked before the audit listener runs. + * + * Before the fix: TypeError from getSponsor() escaped through the + * try/catch(\Exception) and propagated up to the HTTP handler. + * + * After the fix: formatter returns a non-null string with the + * "Unknown Sponsor" fallback. + */ + public function test_deletion_format_does_not_crash_when_sponsor_is_cleared(): void + { + $question = $this->makeQuestionWithClearedSponsor(); + $formatter = new SummitSponsorExtraQuestionTypeAuditLogFormatter( + IAuditStrategy::EVENT_ENTITY_DELETION + ); + + $result = $formatter->format($question, []); + + self::assertIsString($result, 'Formatter must not throw or return null on deletion with cleared sponsor'); + self::assertStringContainsString(self::LABEL, $result); + self::assertStringContainsString('(ID: ' . self::QUESTION_ID . ')', $result); + self::assertStringContainsString('Unknown Sponsor', $result); + self::assertStringContainsString('deleted by user', $result); + } + + /** + * The audit formatter is also invoked on updates; ensure the same + * null-safe path is taken there. + */ + public function test_update_format_does_not_crash_when_sponsor_is_cleared(): void + { + $question = $this->makeQuestionWithClearedSponsor(); + $formatter = new SummitSponsorExtraQuestionTypeAuditLogFormatter( + IAuditStrategy::EVENT_ENTITY_UPDATE + ); + + $result = $formatter->format($question, []); + + self::assertIsString($result); + self::assertStringContainsString(self::LABEL, $result); + self::assertStringContainsString('Unknown Sponsor', $result); + self::assertStringContainsString('updated', $result); + } + + /** + * Defensive: creation formatting must also tolerate a null sponsor. + */ + public function test_creation_format_does_not_crash_when_sponsor_is_cleared(): void + { + $question = $this->makeQuestionWithClearedSponsor(); + $formatter = new SummitSponsorExtraQuestionTypeAuditLogFormatter( + IAuditStrategy::EVENT_ENTITY_CREATION + ); + + $result = $formatter->format($question, []); + + self::assertIsString($result); + self::assertStringContainsString(self::LABEL, $result); + self::assertStringContainsString('Unknown Sponsor', $result); + self::assertStringContainsString('created by user', $result); + } + + /** + * Happy-path sanity: when the sponsor is attached the formatter + * renders the company name and sponsor id (proves the null-safety + * fix didn't regress the normal output format). + */ + public function test_deletion_format_includes_company_and_sponsor_id_when_sponsor_present(): void + { + $question = $this->makeQuestionWithSponsor(); + $formatter = new SummitSponsorExtraQuestionTypeAuditLogFormatter( + IAuditStrategy::EVENT_ENTITY_DELETION + ); + + $result = $formatter->format($question, []); + + self::assertIsString($result); + self::assertStringContainsString('Acme Corp', $result); + self::assertStringContainsString('(ID: ' . self::SPONSOR_ID . ')', $result); + self::assertStringNotContainsString('Unknown Sponsor', $result); + } + + /** + * Guards the early-return: formatter must ignore subjects of other + * types without throwing. + */ + public function test_format_returns_null_for_unrelated_subject(): void + { + $formatter = new SummitSponsorExtraQuestionTypeAuditLogFormatter( + IAuditStrategy::EVENT_ENTITY_DELETION + ); + + self::assertNull($formatter->format(new \stdClass(), [])); + } +} diff --git a/tests/oauth2/OAuth2SummitLocationsApiTest.php b/tests/oauth2/OAuth2SummitLocationsApiTest.php index 10ebef119..caf98ea8b 100644 --- a/tests/oauth2/OAuth2SummitLocationsApiTest.php +++ b/tests/oauth2/OAuth2SummitLocationsApiTest.php @@ -665,7 +665,10 @@ public function testAddVenueFloor(){ 'venue_id' => self::$mainVenue->getId() ]; - $number = rand(0,1000); + // Collision-free floor number: start above the seed floor (number=1 in InsertSummitTestData) + // and monotonically increment to guarantee uniqueness across calls in the same process. + static $floor_number_seq = 1000; + $number = ++$floor_number_seq; $name = str_random(16).'_floor'; $data = [ 'name' => $name,