feat: add missing formatters in config and UT#508
feat: add missing formatters in config and UT#508andrestejerina97 wants to merge 3 commits intomainfrom
Conversation
andrestejerina97
force-pushed
the
feature/add-missing-ut-formatters
branch
from
953ae62 to
793b827
Compare
|
@martinquiroga-exo ready to review |
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@andrestejerina97 please see comment
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
Blockers
-
Inheritance gap —
RSVPQuestionTemplateAuditLogFormatteris dead code in production. Traced the path:AuditEventListener.php:45-58hands Doctrine's concrete entity to the strategy,AuditLogOtlpStrategy.php:59passes it through unchanged,AuditLogFormatterFactory.php:138does exactget_class($subject)lookup. No parent walk anywhere inapp/Audit— grep forgetRootEntityName|class_parents|is_subclass_of|getParentClassis empty. AndSummitRSVPTemplateQuestionFactory.php:38-80only ever instantiates concrete subtypes (9 cases, no default) — there's no production path that creates a bareRSVPQuestionTemplate. So every real audit event arrives asRSVPDropDownQuestionTemplate,RSVPCheckBoxListQuestionTemplate, etc.; the parent-class config key ataudit_log.php:283-286never matches. Same shape forPrivatePresentationCategoryGroupperPresentationCategoryGroup.php:31. Fix: register each concrete subclass individually — that's the precedent already in this file (SummitVenue/SummitVenueRoom/SummitExternalLocation/SummitHotel/SummitAirportat lines 107,111,135,139,143,147) — or havegetFormatterByContextfall back toclass_parents()on a miss. -
Martin's
'name'→'type'comment is still sitting there 6 weeks later.tests/OpenTelemetry/Formatters/PresentationTypeAuditLogFormatterTest.php:73uses change-set key'name', but the Doctrine property is$type(SummitEventType.php:37-38,#[ORM\Column(name: 'Type')]). Passes only becausebuildChangeDetailsdoesn't validate keys. One line. -
52 commits behind main, GitHub flags CONFLICTING. The conflict surface is just the new block at
config/audit_log.php:259-289. Rebase.
andrestejerina97
force-pushed
the
feature/add-missing-ut-formatters
branch
from
793b827 to
c853968
Compare
📝 WalkthroughWalkthroughThis PR wires audit formatter mappings in config, changes RSVPInvitationAuditLogFormatter to read invitee identity via getInvitee(), and adds PHPUnit tests for company, presentation, and RSVP-related formatters covering creation/update/deletion and edge cases. ChangesAudit Logging for Company, Presentation, and RSVP Entities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-508/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/OpenTelemetry/Formatters/RSVPTemplateAuditLogFormatterTest.php (1)
82-104: ⚡ Quick winStrengthen update/deletion assertions to validate payload details.
Current checks (
'updated'/'deleted') are a bit loose. Please assert stable identifiers/details (e.g., template ID/title and changed field values) so formatter regressions don’t slip through.Suggested assertion hardening
public function testSubjectUpdateAuditMessage(): void { @@ $this->assertNotNull($result); $this->assertStringContainsString('updated', $result); + $this->assertStringContainsString('Gala Dinner RSVP', $result); + $this->assertStringContainsString('title', $result); + $this->assertStringContainsString('Cocktail Dinner RSVP', $result); } @@ public function testSubjectDeletionAuditMessage(): void { @@ $this->assertNotNull($result); $this->assertStringContainsString('deleted', $result); + $this->assertStringContainsString('Gala Dinner RSVP', $result); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/OpenTelemetry/Formatters/RSVPTemplateAuditLogFormatterTest.php` around lines 82 - 104, The tests testSubjectUpdateAuditMessage and testSubjectDeletionAuditMessage assert only the presence of 'updated'/'deleted' which is too weak; update them to assert specific payload details produced by RSVPTemplateAuditLogFormatter: for testSubjectUpdateAuditMessage verify the formatted message includes the template identifier/title from $this->mockSubject and the change details from $changeSet (e.g., old and new 'title' values), and for testSubjectDeletionAuditMessage assert the message contains the template identifier/title from $this->mockSubject and a clear 'deleted' phrase; use RSVPTemplateAuditLogFormatter::format, the same AuditContextBuilder::default()->build() context, and existing $this->mockSubject/changeSet variables to locate the assertions to strengthen.config/audit_log.php (1)
323-382: ⚡ Quick winAdd a focused config-wiring test for these new entity mappings.
These entries are runtime bindings, so typos or future class renames can silently disable formatter resolution even when formatter unit tests pass. A small parameterized test asserting
entity => strategypairs would harden this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/audit_log.php` around lines 323 - 382, Add a parameterized config-wiring test that verifies each runtime mapping in the audit config resolves to an existing, instantiable formatter to prevent silent breakage; create a test (e.g., AuditConfigWiringTest) that loads the audit mapping (keys like \models\main\Company::class, \App\Models\Foundation\Summit\Events\RSVP\RSVPTemplate::class and strategy classes like \App\Audit\ConcreteFormatters\RSVPQuestionTemplateAuditLogFormatter::class), iterate expected entity=>strategy pairs, assert the mapping exists in the loaded config, assert class_exists(strategy) and that the IoC container (or new strategy via app()->make or new) can instantiate it without error, and parametrize the test so each pair is a separate assertion case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/OpenTelemetry/Formatters/CompanyAuditLogFormatterTest.php`:
- Around line 91-95: The test testFormatterReturnsNullForInvalidSubject should
set the formatter's context before calling format so the assertion isolates
invalid-subject behavior; update the test to configure the
CompanyAuditLogFormatter instance (constructed with
IAuditStrategy::EVENT_ENTITY_CREATION) with the same context used in other tests
(e.g. call the formatter's context-setting method used elsewhere in the suite)
and then call $formatter->format(new \stdClass(), []) and assertNull the result.
---
Nitpick comments:
In `@config/audit_log.php`:
- Around line 323-382: Add a parameterized config-wiring test that verifies each
runtime mapping in the audit config resolves to an existing, instantiable
formatter to prevent silent breakage; create a test (e.g.,
AuditConfigWiringTest) that loads the audit mapping (keys like
\models\main\Company::class,
\App\Models\Foundation\Summit\Events\RSVP\RSVPTemplate::class and strategy
classes like
\App\Audit\ConcreteFormatters\RSVPQuestionTemplateAuditLogFormatter::class),
iterate expected entity=>strategy pairs, assert the mapping exists in the loaded
config, assert class_exists(strategy) and that the IoC container (or new
strategy via app()->make or new) can instantiate it without error, and
parametrize the test so each pair is a separate assertion case.
In `@tests/OpenTelemetry/Formatters/RSVPTemplateAuditLogFormatterTest.php`:
- Around line 82-104: The tests testSubjectUpdateAuditMessage and
testSubjectDeletionAuditMessage assert only the presence of 'updated'/'deleted'
which is too weak; update them to assert specific payload details produced by
RSVPTemplateAuditLogFormatter: for testSubjectUpdateAuditMessage verify the
formatted message includes the template identifier/title from $this->mockSubject
and the change details from $changeSet (e.g., old and new 'title' values), and
for testSubjectDeletionAuditMessage assert the message contains the template
identifier/title from $this->mockSubject and a clear 'deleted' phrase; use
RSVPTemplateAuditLogFormatter::format, the same
AuditContextBuilder::default()->build() context, and existing
$this->mockSubject/changeSet variables to locate the assertions to strengthen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4288b86-83d2-4ceb-91cc-f251420c60e9
📒 Files selected for processing (10)
app/Audit/ConcreteFormatters/RSVPInvitationAuditLogFormatter.phpconfig/audit_log.phptests/OpenTelemetry/Formatters/CompanyAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/PresentationCategoryAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/PresentationTypeAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPInvitationAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPQuestionTemplateAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPTemplateAuditLogFormatterTest.php
| public function testFormatterReturnsNullForInvalidSubject(): void | ||
| { | ||
| $formatter = new CompanyAuditLogFormatter(IAuditStrategy::EVENT_ENTITY_CREATION); | ||
| $result = $formatter->format(new \stdClass(), []); | ||
| $this->assertNull($result); |
There was a problem hiding this comment.
Set formatter context in invalid-subject test to isolate the assertion.
This test currently verifies null without the same context setup used elsewhere, so a future context-related failure could mask the real intent (invalid subject handling).
Proposed patch
public function testFormatterReturnsNullForInvalidSubject(): void
{
$formatter = new CompanyAuditLogFormatter(IAuditStrategy::EVENT_ENTITY_CREATION);
+ $formatter->setContext(AuditContextBuilder::default()->build());
$result = $formatter->format(new \stdClass(), []);
$this->assertNull($result);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testFormatterReturnsNullForInvalidSubject(): void | |
| { | |
| $formatter = new CompanyAuditLogFormatter(IAuditStrategy::EVENT_ENTITY_CREATION); | |
| $result = $formatter->format(new \stdClass(), []); | |
| $this->assertNull($result); | |
| public function testFormatterReturnsNullForInvalidSubject(): void | |
| { | |
| $formatter = new CompanyAuditLogFormatter(IAuditStrategy::EVENT_ENTITY_CREATION); | |
| $formatter->setContext(AuditContextBuilder::default()->build()); | |
| $result = $formatter->format(new \stdClass(), []); | |
| $this->assertNull($result); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/OpenTelemetry/Formatters/CompanyAuditLogFormatterTest.php` around lines
91 - 95, The test testFormatterReturnsNullForInvalidSubject should set the
formatter's context before calling format so the assertion isolates
invalid-subject behavior; update the test to configure the
CompanyAuditLogFormatter instance (constructed with
IAuditStrategy::EVENT_ENTITY_CREATION) with the same context used in other tests
(e.g. call the formatter's context-setting method used elsewhere in the suite)
and then call $formatter->format(new \stdClass(), []) and assertNull the result.
|
it's ready to review @caseylocker |
Code reviewFound 1 issue:
For reference, the established pattern in the existing suite: 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
@andrestejerina97 I've verified that my requested changes are implemented, thank you. Once the test consistency request from romanetar is implemented it should be good to go. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-508/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterManyToManyTest.php (1)
49-49: ⚡ Quick winConsider splitting
LOG_NO_CHANGESinto two independent assertions to match the existing per-field pattern.
assertStringContainsString(self::LOG_NO_CHANGES, $result)passes only if'Added IDs: [], Removed IDs: []'appears as an exact contiguous substring. If the formatter places these on separate lines or uses a different separator, the assertion fails without clearly indicating which field is wrong.The parallel test
testManyToManyUpdateUsesAddedAndRemovedIdsFromCollectionDiffalready uses two separate assertions (lines 145–147), andLOG_REMOVED_IDS_EMPTY('Removed IDs: []') already exists for the removed half.♻️ Suggested refactor
+ private const LOG_ADDED_IDS_EMPTY = 'Added IDs: []'; private const LOG_NO_CHANGES = 'Added IDs: [], Removed IDs: []';$this->assertNotNull($result); $this->assertStringContainsString(self::LOG_UPDATED_M2M, $result); - $this->assertStringContainsString(self::LOG_NO_CHANGES, $result); + $this->assertStringContainsString(self::LOG_ADDED_IDS_EMPTY, $result); + $this->assertStringContainsString(self::LOG_REMOVED_IDS_EMPTY, $result);
LOG_NO_CHANGEScan then be removed if it has no other consumers.Also applies to: 160-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterManyToManyTest.php` at line 49, Replace the single contiguous assertion using the LOG_NO_CHANGES constant with two independent assertions that check for the added and removed parts separately (e.g. assertStringContainsString('Added IDs: []', $result) and assertStringContainsString(self::LOG_REMOVED_IDS_EMPTY, $result)) in the test method testManyToManyUpdateUsesAddedAndRemovedIdsFromCollectionDiff (and the other similar assertion at the later occurrence), and remove the LOG_NO_CHANGES constant if it has no other uses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@tests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterManyToManyTest.php`:
- Line 49: Replace the single contiguous assertion using the LOG_NO_CHANGES
constant with two independent assertions that check for the added and removed
parts separately (e.g. assertStringContainsString('Added IDs: []', $result) and
assertStringContainsString(self::LOG_REMOVED_IDS_EMPTY, $result)) in the test
method testManyToManyUpdateUsesAddedAndRemovedIdsFromCollectionDiff (and the
other similar assertion at the later occurrence), and remove the LOG_NO_CHANGES
constant if it has no other uses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89c212d3-bdea-4877-847d-539dc40c67e4
📒 Files selected for processing (9)
tests/OpenTelemetry/Formatters/CompanyAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/PresentationCategoryAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterManyToManyTest.phptests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/PresentationTypeAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPInvitationAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPQuestionTemplateAuditLogFormatterTest.phptests/OpenTelemetry/Formatters/RSVPTemplateAuditLogFormatterTest.php
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/OpenTelemetry/Formatters/PresentationCategoryGroupAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/RSVPTemplateAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/RSVPInvitationAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/RSVPQuestionTemplateAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/RSVPAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/PresentationTypeAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/CompanyAuditLogFormatterTest.php
- tests/OpenTelemetry/Formatters/PresentationCategoryAuditLogFormatterTest.php
|
@romanetar changes the tests were added |
4 participants
ref: https://app.clickup.com/t/86b8nemnj
Summary by CodeRabbit
Bug Fixes
New Features
Tests