Log Buckets #1310
Log Buckets #1310
Conversation
| 'buckets' => array_reduce( $buckets, function ( array $bucketMap, Bucket $bucket ) { | ||
| $bucketMap[$bucket->getCampaign()->getName()] = $bucket->getName(); | ||
| return $bucketMap; | ||
| }, [] ) |
There was a problem hiding this comment.
One argument per line would be more readable here for me
Add BucketLogWriter interface and a file-based implementation. Add the BucketLogWriter to "AddDonation" route. TODO: - Add central location for event names. - Write E2E test case to make sure log is actually written - Add Logging to creation of memberships.
Improve BucketLogger Creation Add Edge2Edge test for adding donations.
Create proper value objects for the different logging events to enforce having the correct metadata and name strings.
|
|
||
| $logWriter->writeEvent( 'testLogged', [], ...[] ); | ||
|
|
||
| $this->assertLogValue( date( 'Y-m-d H:i' ), 'date', $logfile ); |
There was a problem hiding this comment.
pretty neat that you extracted the assertion into a function 👍
| public function testLogWriterAddsDate() { | ||
| $logfile = fopen( 'php://memory', 'a+' ); | ||
| $logWriter = new StreamBucketLogger( $logfile ); | ||
| $logWriter->setDateFormat( 'Y-m-d H:i' ); // Ignore seconds to avoid Heisentests |
There was a problem hiding this comment.
haha :)
Isn't the issue still there though? Just 60 times less likely to happen?
There was a problem hiding this comment.
The execution time where this is relevant is just a few milliseconds (maybe even less), so the odds of this being a problem is like a 1 in 60000 chance which should be good enough, otherwise we should start considering ditching Wikipedia and start playing the lottery.
There was a problem hiding this comment.
No need to have this chance at all though
| class StreamBucketLoggerTest extends TestCase { | ||
|
|
||
| public function testLogWriterAddsDate() { | ||
| $logfile = fopen( 'php://memory', 'a+' ); |
There was a problem hiding this comment.
What about creating this in setUp? I did not check fully if that makes sense but noticed low level details in test method "arrange" part and repetition. Even if setUp is not suitable extracting this to avoid the low level details might be good.
| ); | ||
| } | ||
|
|
||
| public function setDateFormat( string $dateFormat ): void { |
There was a problem hiding this comment.
Why is this not a constructor parameter?
| fwrite( | ||
| $this->stream, | ||
| json_encode( [ | ||
| 'date' => date( $this->dateFormat ), |
There was a problem hiding this comment.
This is a global function that relies on global state that gets changed by the system. As you noticed this causes issues when trying to test this code. What about creating a TimeThinghy service? That way this code no longer binds to that changing global state and also no longer needs to know about the time format.
There was a problem hiding this comment.
I will make a PR into this one with a SuchSuperTimeThinghy
|
I addressed my own comments in #1315, which is a PR into this one. So please review that one before merging this one, else we'll need to rebase and create a fresh PR, since GH does not handle that situation well. |
Avoid binding to changing global state
Add BucketLogWriter interface and a file-based implementation.
Add Logging events for adding donations and membership applications.
Explain how to process the log file for getting a CSV file.
Please do a "Squash and merge" for this PR.