Merged
Conversation
andrestejerina97
changed the title
andrestejerina97
force-pushed
the
chore/refactor-to-strategy-formatters
branch
3 times, most recently
from
7b3c6b8 to
1cc3cbb
Compare
smarcet
reviewed
Dec 5, 2025
|
|
||
| private function matchesStrategy(array $strategy, AuditContext $ctx): bool | ||
| { | ||
| if (isset($strategy['route']) && !$this->routeMatches($strategy['route'], $ctx->route)) { |
Collaborator
There was a problem hiding this comment.
@andrestejerina97 here we are missing to check the HTTP verb too
a route is compound of 2 attributes
- HTTP verb
- URI
smarcet
reviewed
Dec 5, 2025
|
|
||
| private function routeMatches(string $pattern, string $actual_route): bool | ||
| { | ||
| $normalized_pattern = preg_replace('/\{[a-zA-Z_]+\}/', '\d+', $pattern); |
Collaborator
There was a problem hiding this comment.
@andrestejerina97 we dont need this lets just add the rawRoute ( without the bindings at AuditContext)
here
summit-api/app/Audit/AuditEventListener.php
Line 111 in ee9422d
like this
rawRoute: Route::getRoutes()->match($req);
and then it just a simple string comparison
smarcet
requested changes
Dec 5, 2025
Collaborator
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@andrestejerina97 please review comments
smarcet
reviewed
Dec 8, 2025
app/Audit/AuditEventListener.php
Outdated
| $req = request(); | ||
|
|
||
|
|
||
| $route = Route::getRoutes()->match(request()); |
Collaborator
There was a problem hiding this comment.
use $req here we are already getting the ref
smarcet
reviewed
Dec 8, 2025
app/Audit/AuditEventListener.php
Outdated
|
|
||
| $route = Route::getRoutes()->match(request()); | ||
| $method = isset($route->methods[0]) ? $route->methods[0] : null; | ||
| $rawRoute = $method."|".$route->uri; |
Collaborator
There was a problem hiding this comment.
use a constant here to communicate the intention
smarcet
reviewed
Dec 8, 2025
|
|
||
| private function matchesStrategy(array $strategy, AuditContext $ctx): bool | ||
| { | ||
| if (isset($strategy['route']) && !$this->routeMatches($strategy['route'], $ctx->route)) { |
Collaborator
There was a problem hiding this comment.
u need to user $ctx->rawRoute not $ctx->route
smarcet
requested changes
Dec 8, 2025
Collaborator
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@andrestejerina97 @martinquiroga-exo pleare review comments
andrestejerina97
force-pushed
the
chore/refactor-to-strategy-formatters
branch
from
ceb8212 to
88ad555
Compare
…context to the formatters
andrestejerina97
force-pushed
the
chore/refactor-to-strategy-formatters
branch
from
88ad555 to
e6043e6
Compare
smarcet
reviewed
Dec 9, 2025
config/audit_log.php
Outdated
| 'formatter' => \App\Audit\ConcreteFormatters\PresentationFormatters\PresentationEventApiAuditLogFormatter::class, | ||
| ], | ||
| [ | ||
| 'route' => 'PUT|api/v1/summits/{id}/events', |
Collaborator
There was a problem hiding this comment.
this is missing param {event_id}
route is api/v1/summits/{id}/events/{event_id}
matiasperrone-exo
pushed a commit
that referenced
this pull request
Dec 9, 2025
* feat: add news formatter for entitys related to Presentation for speakers * feat: add a formatter for entities related to speaker presentations * chore: replace the otlp interface with abstractFactory. Add the user context to the formatters * chore: refactor getUserInfo to AbstractAuditLog * chore: refactor strategy for get Formatters * chore: change path to actual routes * fix: change path to actual routes * fix: OpenAPI doc * fix: add missing param --------- Co-authored-by: Matias Perrone <github@matiasperrone.com>
matiasperrone-exo
pushed a commit
that referenced
this pull request
Dec 10, 2025
* feat: add news formatter for entitys related to Presentation for speakers * feat: add a formatter for entities related to speaker presentations * chore: replace the otlp interface with abstractFactory. Add the user context to the formatters * chore: refactor getUserInfo to AbstractAuditLog * chore: refactor strategy for get Formatters * chore: change path to actual routes * fix: change path to actual routes * fix: OpenAPI doc * fix: add missing param --------- Co-authored-by: Matias Perrone <github@matiasperrone.com>
andrestejerina97
added a commit
that referenced
this pull request
Dec 16, 2025
This reverts commit 7e817d7.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.