Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[toggle-core#16] Create FeatureWasCreated event #24

Merged
merged 2 commits into from Oct 3, 2021
Merged

[toggle-core#16] Create FeatureWasCreated event #24

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e4bcdc
[toggle-core#16] Create FeatureWasCreated event
pcs289 Oct 2, 2021
97ae399
[#16] Tests for FeatureWasCreated event
pcs289 Oct 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/Write/Event/FeatureWasCreated.php
View file
Open in desktop
@@ -0,0 +1,34 @@
<?php

declare(strict_types=1);

namespace Pheature\Core\Toggle\Write\Event;

final class FeatureWasCreated
{

private string $eventType;

private string $featureId;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can define properties in the same order as they're assigned in the constructor:

Suggested change
private string $eventType;
private string $featureId;
private string $featureId;
private string $eventType;


public function __construct(string $featureId)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could pass the FeatureId VO instead of the primitive to ensure that it's a right FeatureId. Then, inside the constructor we can assign the string value to the property.

Copy link
Member

Choose a reason for hiding this comment

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

Is better to work with generics at this level to easy serialization of the object, we create the value object in the getter as it is immutable

{
$this->featureId = $featureId;
$this->eventType = "FEATURE_WAS_CREATED";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could put the string in lowercase and extract it into a constant for future references. @kpicaza what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

we don't need event type the event class name is the type itself

}

public function eventType(): string
{
kpicaza marked this conversation as resolved.
Show resolved Hide resolved
return $this->eventType;
}

public function featureId(): string
{
return $this->featureId;
}

public static function fromString(string $payload): self
{
return new self($payload);
}
}
2 changes: 2 additions & 0 deletions src/Write/Feature.php
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@

namespace Pheature\Core\Toggle\Write;

use Pheature\Core\Toggle\Write\Event\FeatureWasCreated;
use JsonSerializable;

use function array_map;
Expand Down Expand Up @@ -32,6 +33,7 @@ public function __construct(FeatureId $featureId, bool $enabled, array $strategi
foreach ($strategies as $strategy) {
$this->strategies[$strategy->id()->value()] = $strategy;
}
$this->events[] = new FeatureWasCreated($featureId->value());
}

/**
Expand Down
8 changes: 4 additions & 4 deletions test/Write/FeatureTest.php
View file
Open in desktop
Expand Up @@ -48,7 +48,7 @@ public function testItShouldBeEnabled(): void
$feature->enable();
$this->assertTrue($feature->isEnabled());
$events = $feature->release();
$this->assertCount(0, $events);
$this->assertCount(1, $events); // Released FeatureWasCreated event
Copy link
Contributor

Choose a reason for hiding this comment

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

As @kpicaza commented, we could add an extra line here, checking the event type, for example:

Suggested change
$this->assertCount(1, $events); // Released FeatureWasCreated event
$this->assertCount(1, $events);
$this->assertInstanceOf(FeatureWasCreated::class, reset($events));

Copy link
Member Author

Choose a reason for hiding this comment

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

We added a specific test for FeatureWasCreated event

}

public function testItShouldBeDisabled(): void
Expand All @@ -58,15 +58,15 @@ public function testItShouldBeDisabled(): void
$feature->disable();
$this->assertFalse($feature->isEnabled());
$events = $feature->release();
$this->assertCount(0, $events);
$this->assertCount(1, $events); // Released FeatureWasCreated event
}

public function testItShouldHaveAddedStrategies(): void
{
$feature = $this->getFeatureWithAnStrategy();
$this->assertCount(1, $feature->strategies());
$events = $feature->release();
$this->assertCount(0, $events);
$this->assertCount(1, $events); // Released FeatureWasCreated event
}

public function testItShouldRemoveAddedStrategies(): void
Expand All @@ -77,7 +77,7 @@ public function testItShouldRemoveAddedStrategies(): void
$feature->removeStrategy(StrategyId::fromString(self::STRATEGY_ID));
$this->assertCount(0, $feature->strategies());
$events = $feature->release();
$this->assertCount(0, $events);
$this->assertCount(1, $events); // Released FeatureWasCreated event
}

private function createFeature(): Feature
Expand Down