Skip to content

Feature:add new formatters for Tickets#494

Open
andrestejerina97 wants to merge 5 commits intomainfrom
feature/add-ticket-formatters
Open

Feature:add new formatters for Tickets#494
andrestejerina97 wants to merge 5 commits intomainfrom
feature/add-ticket-formatters

Conversation

@andrestejerina97
Copy link
Contributor

@andrestejerina97 andrestejerina97 commented Feb 4, 2026

@andrestejerina97 andrestejerina97 force-pushed the feature/add-ticket-formatters branch from d325475 to 4b7ad9c Compare February 4, 2026 21:17
@andrestejerina97 andrestejerina97 marked this pull request as ready for review February 4, 2026 21:19
Copy link
Contributor

@martinquiroga-exo martinquiroga-exo left a comment

Choose a reason for hiding this comment

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

@andrestejerina97 please review comments. Thanks

Copy link
Contributor

@martinquiroga-exo martinquiroga-exo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

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

This is looking good but shouldn't we see the new formatters registered in config/audit_log.php?

Also, SummitTaxTypeAuditLogFormatter and SummitRefundPolicyTypeAuditLogFormatter are scaling rates incorrectly by multiplying by 100 before rendering, but these are already stored as percentage points (e.g., 21.0, 90.5). This causes audit logs to report inflated values (e.g., 2100% instead of 21%). Remove the extra * 100 in formatter output. The corresponding tests (SummitTaxTypeAuditLogFormatterTest, SummitRefundPolicyTypeAuditLogFormatterTest) also use decimal-fraction mock values
(e.g., 0.21, 1.0, 0.5) instead of real-world percentage-point values (e.g., 21.0, 100.0, 50.0).

@andrestejerina97
Copy link
Contributor Author

Ready to review @caseylocker

Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

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

LGTM but needs to have the merge conflicts fixed before merging.

@andrestejerina97 andrestejerina97 force-pushed the feature/add-ticket-formatters branch from 2697d22 to 2db0afa Compare February 18, 2026 18:10
Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

@andrestejerina97 please review coments

@andrestejerina97
Copy link
Contributor Author

it's ready to review

Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

@andrestejerina97 please review comments

@matiasperrone-exo matiasperrone-exo force-pushed the feature/add-ticket-formatters branch from b1be8b2 to 133388c Compare March 3, 2026 19:25
@matiasperrone-exo
Copy link
Contributor

Thank you for the comments.
All the tests were updated as requested.

image

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.

5 participants