Feat/triumph service #8
Conversation
WalkthroughThis change migrates the "Triumph" feature to a dedicated domain module, introducing new entities, enums, services, configuration, and event handling under Changes
Sequence Diagram(s)sequenceDiagram
participant EventSource as DaedalusCycleEvent / DaedalusEvent
participant TriumphSubscriber as TriumphSourceEventSubscriber
participant TriumphService as ChangeTriumphFromEventService
participant TriumphRepo as TriumphConfigRepository
participant Player as Player
participant LogService as RoomLogService
EventSource->>TriumphSubscriber: Event triggered (e.g., new cycle)
TriumphSubscriber->>TriumphService: execute(EventSource)
TriumphService->>TriumphRepo: findAllByTargetedEvent(EventSource)
TriumphRepo-->>TriumphService: [TriumphConfig...]
TriumphService->>TriumphService: filter configs by tags
TriumphService->>Player: add triumph(s) based on config & event
TriumphService->>LogService: emit TriumphChangedEvent
LogService->>LogService: createLog with event details
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
Api/src/Player/Service/PlayerService.php (1)
315-334: 🛠️ Refactor suggestion
handleTriumphChange()is dead code – delete or re-wire
handleNewCycle()no longer invokes this method, and no other call site exists. Keeping it:• inflates class size,
• leaves an unusedTriumphEnumimport,
• risks divergence from the new event-driven triumph flow.Unless you plan to re-use this locally, drop the method and its import.
- private function handleTriumphChange(Player $player, \DateTime $date): void - { - $gameConfig = $player->getDaedalus()->getGameConfig(); - - $humanTriumph = $gameConfig->getTriumphConfig()->getByNameOrThrow(TriumphEnum::CYCLE_HUMAN); - $mushTriumph = $gameConfig->getTriumphConfig()->getByNameOrThrow(TriumphEnum::CYCLE_MUSH); - $triumphChange = $player->isMush() ? $mushTriumph->getQuantity() : $humanTriumph->getQuantity(); - - $player->addTriumph($triumphChange); - - $this->roomLogService->createLog( - PlayerModifierLogEnum::GAIN_TRIUMPH, - $player->getPlace(), - VisibilityEnum::PRIVATE, - 'event_log', - $player, - ['quantity' => $triumphChange], - $date - ); - }Api/src/Triumph/Entity/TriumphConfig.php (1)
1-134: 💡 Verification agent🧩 Analysis chain
Consider adding a getId method for consistency
Most entity classes provide a getter for the ID property. While this might be intentionally omitted for domain design reasons, consider adding it for consistency with other entities and to facilitate integration with frameworks that might expect it:
public function getId(): int { return $this->id; }Also, adding some PHPDoc comments explaining the purpose of this entity and its role in the triumph system would improve code documentation.
🏁 Script executed:
#!/bin/bash # Check if other entity classes in the codebase have getId methods echo "Checking for getId methods in other entity classes..." rg -g "*.php" --multiline "class \w+[\s\n]+{.*?public function getId\(\)" --type phpLength of output: 348
🏁 Script executed:
#!/bin/bash echo "Searching for any getId() method definitions in PHP files..." rg -g '*.php' 'public function getId\(' -A 2Length of output: 15380
Add getId() and PHPDoc to TriumphConfig for consistency
TriumphConfig is the only entity in Api/src/Triumph/Entity without a public getId() method, yet nearly every other Doctrine entity in the codebase exposes its identifier. To align with existing conventions and improve compatibility with tools/frameworks, please:
• In Api/src/Triumph/Entity/TriumphConfig.php, add:
/** * Get the unique identifier for this Triumph configuration. */ public function getId(): int { return $this->id; }• Add a class-level PHPDoc block summarizing the purpose of this entity within the triumph system. For example:
/** * TriumphConfig represents the configuration parameters * for a single triumph event, including its scope, visibility, * target values, and regression behavior. */ #[ORM\Entity] #[ORM\Table(name: 'triumph_config')] class TriumphConfig { // … }These additions will maintain consistency across entities and enhance code documentation.
🧹 Nitpick comments (18)
Api/src/Triumph/Enum/TriumphScope.php (1)
7-14: Consider renaming the NULL case for better clarity.The use of
NULLas an enum case name might be confusing since it could be mistaken for PHP'snullvalue, while it actually represents an empty string. A more descriptive name likeNONEorEMPTYwould better convey its purpose.- case NULL = ''; + case NONE = '';Api/src/Triumph/config/services.yaml (1)
1-12: Service configuration looks good but missing newline at end of file.The service configuration correctly sets up autowiring and autoconfiguration for the Triumph module, following Symfony best practices. However, the file is missing a newline character at the end, as flagged by the static analysis tool.
Add a newline at the end of the file to follow coding standards.
- '../Entity/' - '../Kernel.php' - '../Tests/' +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/src/Triumph/Enum/TriumphVisibility.php (1)
7-16: Consider__toString()& clearer case naming
A
__toString()magic method would let you cast the enum directly in string contexts ((string)$visibility) and avoids a dedicatedtoString()call.
case NULL = ''can be confusing because:
•NULLis a PHP keyword.
• The empty string does not obviously map to a “null” concept—NONEorUNSPECIFIEDwould be more explicit.Example diff:
enum TriumphVisibility: string { @@ - case NULL = ''; + case NONE = ''; + + public function __toString(): string + { + return $this->value; + } - public function toString(): string - { - return $this->value; - } }Api/src/Daedalus/Entity/Daedalus.php (1)
209-217: Consider naming consistency between player collection methodsBoth methods effectively delegate to their counterparts in the PlayerCollection class, but there's a slight naming inconsistency. The methods here use plural form (
getHumanPlayers,getMushPlayers) while the called methods use singular form (getHumanPlayer,getMushPlayer). Since both return collections of players, consistent naming would improve code readability.Two options to consider:
- Update PlayerCollection methods to use plural forms for consistency
- Add documentation to clarify the naming pattern
public function getHumanPlayers(): PlayerCollection { + // Returns collection of human players using PlayerCollection::getHumanPlayer return $this->getPlayers()->getHumanPlayer(); } public function getMushPlayers(): PlayerCollection { + // Returns collection of mush players using PlayerCollection::getMushPlayer return $this->getPlayers()->getMushPlayer(); }Api/src/Player/Entity/Collection/PlayerCollection.php (1)
174-177: Clarify the difference between getAllActive and getActivePlayersThe new
getAllActivemethod filters only on player active status, while the existinggetActivePlayersmethod (line 85) filters on both alive status and active status. This distinction may not be immediately clear from the method names.Consider adding a documentation comment to clarify the difference:
/** + * Returns all active players, regardless of alive status. + * @see getActivePlayers() which returns only players that are both alive and active + */ public function getAllActive(): self { return $this->filter(static fn (Player $player) => $player->isActive()); }Api/src/Daedalus/Factory/DaedalusFactory.php (1)
210-212: Code now uses DTO pattern consistently.The implementation now uses
TriumphConfigData::getAll()to retrieve DTOs andTriumphConfig::fromDto()to create entities, which is more consistent with modern design patterns. This approach provides better encapsulation and flexibility compared to directly accessing static arrays.However, I noticed a typo in the variable name
$triumpthConfigs(extra 'h'). While this doesn't affect functionality, it's inconsistent with naming conventions.- /** @var ArrayCollection<array-key, TriumphConfig> $triumpthConfigs */ - $triumpthConfigs = new ArrayCollection(); - foreach (TriumphConfigData::getAll() as $triumpthConfigData) { - $triumpthConfigs->add(TriumphConfig::fromDto($triumpthConfigData)); + /** @var ArrayCollection<array-key, TriumphConfig> $triumphConfigs */ + $triumphConfigs = new ArrayCollection(); + foreach (TriumphConfigData::getAll() as $triumphConfigData) { + $triumphConfigs->add(TriumphConfig::fromDto($triumphConfigData));Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php (1)
1-40: Well-structured functional test that verifies triumph awarding on cycle eventsThis test correctly validates that a player receives triumph points when a new Daedalus cycle occurs. The test setup and assertion are straightforward and effective.
Consider expanding test coverage by adding cases for:
- Mush players (who might receive different triumph values)
- Multiple cycle events (to verify cumulative triumph)
- Players who already have triumph points
Api/src/Triumph/Enum/TriumphEnum.php (1)
16-24: Utility methods enhance the enum's functionalityBoth methods are helpful additions that improve the enum's usability throughout the codebase.
For the
toConfigKey()method, consider adding validation or normalization of the input$configKeyto ensure consistent formatting (e.g., handling cases where the key might already contain underscores or special characters).Api/src/Triumph/Event/TriumphChangedEvent.php (1)
43-46: Consider preserving sign information in triumph quantity.The
getQuantity()method returns the absolute value of the triumph change, which hides whether the triumph was increased or decreased. This might be important for consumers of this event that need to know the direction of the change.public function getQuantity(): int { - return abs($this->triumphConfig->getQuantity()); + return $this->triumphConfig->getQuantity(); } +public function getAbsoluteQuantity(): int +{ + return abs($this->triumphConfig->getQuantity()); +}Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php (1)
41-42: Consider using an import for Mockery.Using
\Mockery::spy()directly works but adding an import would improve code clarity.+use Mockery; // ... -$this->eventService = \Mockery::spy(EventServiceInterface::class); +$this->eventService = Mockery::spy(EventServiceInterface::class);Api/src/Triumph/Repository/TriumphConfigRepository.php (1)
22-25: Consider adding pagination support for large result sets.The
findAllByTargetedEventmethod might return a large number of triumph configurations. Consider adding pagination support or a limit parameter to avoid potential performance issues.-public function findAllByTargetedEvent(TriumphSourceEventInterface $targetedEvent): array +public function findAllByTargetedEvent( + TriumphSourceEventInterface $targetedEvent, + ?int $limit = null, + ?int $offset = null +): array { - return $this->findBy(['targetedEvent' => $targetedEvent->getEventName()]); + $criteria = ['targetedEvent' => $targetedEvent->getEventName()]; + return $this->findBy($criteria, null, $limit, $offset); }Api/migrations/Version20250501184525.php (1)
37-54: Remove unnecessary schema creation in down migration.The
CREATE SCHEMA publicstatement in the down method is unnecessary and could potentially cause issues if applied to databases where this schema already exists.- $this->addSql('CREATE SCHEMA public');Api/src/Triumph/Service/ChangeTriumphFromEventService.php (1)
26-34: Preferforeachovermap()for side-effect-only iteration
map()is intended to transform a collection and return a new one.
Here the return value is discarded, and the callback’s only purpose is the side-effect (addTriumphToPlayer).Using
map():
• Creates an unnecessary intermediate collection.
• Slightly obscures intent (readers may expect a transformed result).Switching to a simple
foreach(or a dedicatedforAll/eachmethod if provided by the collection type) is clearer and marginally more efficient:- $event->getTargetsForTriumph($triumphConfig)->map( - fn (Player $player) => $this->addTriumphToPlayer($triumphConfig, $player) - ); + foreach ($event->getTargetsForTriumph($triumphConfig) as $player) { + $this->addTriumphToPlayer($triumphConfig, $player); + }Api/src/Game/ConfigData/GameConfigData.php (1)
527-528: Potential duplicate keys & repeated heavy mapping
array_map(static fn (…) , TriumphConfigData::getAll())is executed every timeGameConfigData::getAll()is called.
If this method is invoked frequently (e.g. per request) it re-instantiates the full triumph config list, which may become a hotspot.Suggestions:
- Cache the computed array in a static variable to avoid rebuilding.
- Add a uniqueness assertion (e.g.
assert(count($list) === count(array_unique($list)))) to fail fast if two DTOs map to the same config key.Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php (2)
80-85: Use a mock to verify event dispatchThe goal of these tests is triumph calculation, but the service also dispatches a
TriumphChangedEvent.
Replacing the stub with a PHPUnit mock lets you assert thatcallEvent()is invoked exactly once per player, guarding against future regressions without extra tests.$this->eventService = $this->createMock(EventServiceInterface::class); $this->eventService ->expects(self::atLeastOnce()) ->method('callEvent');
150-153: Minor: add assertion message for easier failure diagnosisAdding the triumphant reason in the assertion message speeds up debugging when a test fails.
-self::assertEquals($expectedPoints, $player->getTriumph()); +self::assertEquals( + $expectedPoints, + $player->getTriumph(), + sprintf('Expected %d triumph points for %s, got %d', $expectedPoints, $player->getName(), $player->getTriumph()) +);Api/src/Triumph/Entity/TriumphConfig.php (2)
19-20: Remove unnecessary length constraint on integer columnThe
lengthproperty in the column definition for the integer ID field is unnecessary and might cause issues with some database systems. Integer columns typically don't need a length specification.- #[ORM\Column(type: 'integer', length: 255, nullable: false)] + #[ORM\Column(type: 'integer', nullable: false)]
34-36: Consider adding type hint for array elementsWhile the property is correctly typed as
array, consider using PHP 8 docblock to provide more specific type information about the array elements for better static analysis:#[ORM\Column(type: 'array', nullable: false, options: ['default' => 'a:0:{}'])] + /** @var array<string> */ private array $targetedEventExpectedTags;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250427095647.php(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(2 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/DaedalusCycleEventSubscriber.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)
💤 Files with no reviewable changes (7)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/Enum/TriumphEnum.php
🧰 Additional context used
🧬 Code Graph Analysis (12)
Api/src/Game/Service/ConfigDataLoaderService.php (1)
Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php (1)
TriumphConfigDataLoader(10-28)
Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php (2)
Api/src/Triumph/Repository/TriumphConfigRepository.php (1)
findAllByTargetedEvent(22-25)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php (1)
findAllByTargetedEvent(16-25)
Api/src/Daedalus/Entity/Daedalus.php (2)
Api/src/Player/Entity/Collection/PlayerCollection.php (3)
PlayerCollection(13-183)getHumanPlayer(35-38)getMushPlayer(30-33)Api/src/Place/Entity/Place.php (1)
getPlayers(187-194)
Api/src/Triumph/Enum/TriumphVisibility.php (1)
Api/src/Triumph/Enum/TriumphEnum.php (1)
toString(21-24)
Api/src/Triumph/Event/TriumphSourceEventInterface.php (3)
Api/src/Player/Entity/Collection/PlayerCollection.php (1)
PlayerCollection(13-183)Api/src/Daedalus/Event/DaedalusCycleEvent.php (1)
getTargetsForTriumph(55-64)Api/src/Triumph/Event/TriumphSourceEventTrait.php (1)
hasExpectedTagsFor(11-14)
Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php (4)
Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php (1)
findAllByTargetedEvent(15-15)Api/src/Triumph/Repository/TriumphConfigRepository.php (1)
findAllByTargetedEvent(22-25)Api/src/Triumph/Entity/TriumphConfig.php (1)
getTargetedEvent(91-94)Api/src/Triumph/Event/TriumphSourceEventInterface.php (1)
getEventName(12-12)
Api/src/Triumph/Event/TriumphSourceEventTrait.php (3)
Api/src/Triumph/Event/TriumphSourceEventInterface.php (1)
hasExpectedTagsFor(16-16)Api/src/Game/Event/AbstractGameEvent.php (1)
hasAllTags(117-120)Api/src/Triumph/Entity/TriumphConfig.php (1)
getTargetedEventExpectedTags(96-99)
Api/src/Game/Entity/GameConfig.php (1)
Api/src/Triumph/Entity/TriumphConfigCollection.php (1)
TriumphConfigCollection(13-34)
Api/src/Triumph/ConfigData/TriumphConfigData.php (5)
Api/src/Daedalus/Event/DaedalusCycleEvent.php (1)
DaedalusCycleEvent(14-65)Api/src/Game/Enum/CharacterEnum.php (1)
CharacterEnum(5-132)Api/src/Game/Enum/EventEnum.php (1)
EventEnum(5-16)Api/src/Triumph/Dto/TriumphConfigDto.php (1)
TriumphConfigDto(11-24)Api/src/Triumph/Enum/TriumphEnum.php (2)
toConfigKey(16-19)toString(21-24)
Api/src/Triumph/Entity/TriumphConfigCollection.php (1)
Api/src/Triumph/Entity/TriumphConfig.php (1)
getName(71-74)
Api/src/Triumph/Entity/TriumphConfig.php (3)
Api/src/Game/Entity/GameConfig.php (2)
ORM(32-556)__construct(97-114)Api/src/Triumph/Dto/TriumphConfigDto.php (2)
TriumphConfigDto(11-24)__construct(13-23)Api/src/Triumph/Repository/TriumphConfigRepository.php (1)
__construct(17-20)
Api/src/Triumph/Enum/TriumphEnum.php (1)
Api/src/Triumph/Enum/TriumphVisibility.php (1)
toString(13-16)
🪛 YAMLlint (1.35.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (50)
Api/composer.json (1)
238-241: LGTM: Clean addition of Triumph test scriptThis addition properly follows the existing pattern of test scripts and will allow for targeted testing of the new Triumph module's functionality.
Api/src/Game/Service/ConfigDataLoaderService.php (1)
60-60: Import updated to reflect module restructuringThe import statement now correctly references the TriumphConfigDataLoader from its new location in the Triumph module, which aligns with the broader refactoring of Triumph-related functionality into its own module.
Api/src/Game/Enum/EventPriorityEnum.php (1)
21-21: Appropriate event priority for Triumph processingThe new PLAYER_TRIUMPH constant with priority 9 is properly positioned between PLAYERS (10) and EQUIPMENTS (8), ensuring that triumph processing happens at the right time in the event sequence. The descriptive comment clearly explains its purpose.
Api/config/services.yaml (1)
31-31: Correctly registered Triumph module servicesThe addition of the Triumph module's services configuration follows the established pattern and ensures that all services from the new module will be properly integrated into the application's service container.
Api/config/api_platform/resources/game_config.yaml (1)
40-40: Path reference correctly updated to reflect the new namespace structure.The resource class reference for the
triumphConfigsubresource has been properly updated to point to the new namespaceMush\Triumph\Entity\TriumphConfig. This change aligns with the PR's objective of refactoring the Triumph feature into its own dedicated module.Api/config/api_platform/resources/triumph_config.yaml (1)
1-1: Namespace reference correctly updated.The entity reference has been properly updated to use the new dedicated Triumph namespace while maintaining all the original API platform configurations. This is consistent with the overall refactoring strategy of moving triumph-related code to its own namespace.
Api/migrations/Version20250427095647.php (1)
29-29: Improved migration rollback with default value.Adding
DEFAULT 0to the non-nullabletriumphcolumn in the down migration is a good practice. This ensures that when rolling back the migration on a database with existing records, the addition of the new non-nullable column won't fail due to existing rows lacking a value.Api/src/Triumph/Enum/TriumphTeam.php (1)
7-13: Well-structured enum for team identification.Using a PHP enum for team identification provides type safety and better IDE support compared to string constants. The cases cover the main team types (MUSH, HUMAN) as well as special cases (ANY, NULL).
One observation: the NULL case with an empty string value is an interesting choice. Ensure this is used consistently throughout the codebase to avoid confusion between NULL and empty string values.
Api/src/Game/Entity/GameConfig.php (1)
29-30: Import changes look good.The updated import statements correctly reference the new
Mush\Triumph\Entitynamespace, which aligns with the refactoring of Triumph-related code into its own module.Api/config/packages/doctrine.yaml (1)
127-132: Entity mapping configuration correctly added.The Doctrine mapping for the new Triumph module follows the same consistent pattern as other mappings in the file. This ensures that the Triumph entities will be properly recognized and managed by Doctrine ORM.
Api/src/Game/ConfigData/GameConfigDataLoader.php (3)
24-24: Updated import correctly references the new namespace.The import statement properly references the
TriumphConfigentity from its new location in theMush\Triumph\Entitynamespace, aligning with the overall refactoring.
36-36: Method call togetAll()is more maintainable than direct property access.Using
GameConfigData::getAll()instead of directly accessing a static property improves encapsulation and maintainability by providing a stable API for accessing configuration data.
141-141: Repository query updated to match new entity structure.The query has been changed to search by the
keyfield instead ofname, reflecting changes in theTriumphConfigentity structure in the new module.Api/translations/fr/triumph+intl-icu.fr.xlf (1)
5-26: LGTM – translations read wellThe ICU placeholders (
{quantity}) and emoji markers look correct and the French wording is natural. No issues spotted.Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php (1)
10-16: Well-defined repository interfaceThe interface is appropriately designed with a single method that clearly defines its purpose: finding triumph configurations by their targeted event. The return type annotation correctly specifies an array of TriumphConfig objects.
Api/src/Player/Entity/Collection/PlayerCollection.php (1)
55-58: New collection filtering methodGood addition that allows filtering players by name, returning all matches rather than just the first one. This complements the existing
getPlayerByNamemethod that returns a single player.Api/src/Daedalus/Factory/DaedalusFactory.php (1)
37-38: Import statements updated to use new Triumph namespace.The imports now correctly reference the new
Mush\Triumphnamespace instead of the previousMush\Gamenamespace, aligning with the overall refactoring to move triumph-related code to its own module.Api/src/Daedalus/Event/DaedalusCycleEvent.php (2)
8-13: Event now implements TriumphSourceEventInterface.Added imports support the implementation of the TriumphSourceEventInterface, allowing this event to trigger triumph-related logic.
14-17: Interface implementation and trait usage.The class correctly implements the TriumphSourceEventInterface and uses the TriumphSourceEventTrait to reuse common implementation logic.
Api/config/api_platform/serialization/triumph_config.yaml (3)
1-1: Updated entity namespace.The serialization configuration now correctly references the new namespace
Mush\Triumph\Entity\TriumphConfiginstead of the previousMush\Game\Entity\TriumphConfig.
9-9: Attribute renamed from isAllCrew to isIndividual.The attribute has been renamed to better reflect its purpose, improving code readability.
13-24: New attributes added to serialization groups.All new attributes are consistently configured with both read and write access, which is appropriate for their usage. The naming conventions are clear and descriptive.
Api/src/Triumph/Event/TriumphSourceEventInterface.php (2)
1-9: New interface for triumph source events.This interface establishes a clear contract for events that can trigger triumphs. The namespace, imports, and declarations follow PHP best practices.
10-17: Well-defined method declarations.The interface declares three essential methods with appropriate return types:
getEventName(): Returns the event identifiergetTargetsForTriumph(): Determines player targets based on triumph configurationhasExpectedTagsFor(): Validates if the event has the expected tags for a triumphThis design enables flexible triumph awarding based on various event types and conditions.
Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php (1)
10-28: Clean implementation of configuration data loaderThis loader correctly handles both creation and updating of triumph configurations using the DTO pattern. The handling of existing vs. new entities is well-implemented.
Api/src/Triumph/Listener/DaedalusCycleEventSubscriber.php (1)
12-29: Well-designed event subscriber with good separation of concernsThis subscriber properly implements Symfony's EventSubscriberInterface and follows best practices:
- Single responsibility for handling DaedalusCycleEvents
- Clear priority definition using an enum constant
- Delegation of actual processing to a dedicated service
Api/src/Triumph/Enum/TriumphEnum.php (1)
7-15: Modern PHP enum implementation with appropriate triumph typesThe enum correctly defines various triumph types with string backing values, following PHP 8.1+ best practices.
Api/src/RoomLog/Listener/TriumphEventSubscriber.php (1)
26-39: Good implementation of the event handler.The event handler properly extracts all necessary information from the TriumphChangedEvent and creates an appropriate log entry.
Api/src/Triumph/Event/TriumphChangedEvent.php (1)
12-22: Good event implementation with proper inheritance.The event class correctly extends AbstractGameEvent and passes the required parameters to the parent constructor.
Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php (2)
50-153: Well-structured tests following Given-When-Then pattern.The test cases are comprehensive and well-organized using the Given-When-Then pattern, which makes them clear and easy to follow. They cover all important scenarios for triumph changes.
170-246: Good use of helper methods for test setup and assertions.The private helper methods improve code readability and maintainability by abstracting common test setup and assertion patterns.
Api/src/Triumph/Repository/TriumphConfigRepository.php (1)
12-20: Good repository implementation with proper typing.The repository class correctly extends ServiceEntityRepository with a generic type and implements the appropriate interface. The class is also marked as final, which is a good practice for repository classes.
Api/config/api_platform/validation/triumph_config.yaml (1)
1-27: Validation constraints look good!The validation constraints for the
TriumphConfigentity have been properly updated to match the new namespace and property structure. Each field has appropriate type constraints defined.Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php (1)
1-35: LGTM otherwise!The fixture class implementation follows good practices by:
- Properly implementing the DependentFixtureInterface to ensure correct loading order
- Using the DTO pattern for configuration
- Adding references to allow other fixtures to access these entities
- Persisting both the individual configs and updating the game config
Api/src/Triumph/ConfigData/TriumphConfigData.php (1)
23-70: Good use of named parameters and strong typingThe triumph configuration data is well-structured with named parameters for clarity and proper enum typing for better type safety. The configuration data provides clear mappings between triumph events, scopes, and targets.
Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php (1)
11-36: The in-memory repository pattern is well implementedThe test double correctly implements the repository interface with methods for finding configs by event, saving new configs, and clearing the repository. This provides a good foundation for unit testing without database dependencies.
Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php (1)
1-51: LGTM! Well-structured test for the new triumph service.This functional test effectively verifies that the
ChangeTriumphFromEventServiceproperly handles theDaedalusCycleEventand produces the expected log entries. The test structure is clean with proper setup and assertions.Api/src/Triumph/Dto/TriumphConfigDto.php (1)
11-24: Great use of readonly class and strong typing.The DTO is well-designed with proper type safety using PHP 8.2+ features:
- Uses readonly properties for immutability
- Leverages enum types for name, scope, and visibility
- Provides sensible defaults for optional parameters
- Follows best practices for DTOs with clean parameter declarations
This immutable design will help prevent accidental mutations and make the code more predictable.
Api/src/Triumph/Entity/TriumphConfigCollection.php (5)
3-5: Good addition of strict types and namespace change.The change to a dedicated Triumph namespace and addition of strict typing improves code organization and type safety.
7-8: Appropriate import update for the enum.Properly updating imports to match the new namespace structure.
13-14: Good use of final keyword.Making the class final prevents inheritance, which is appropriate for a specialized collection class.
15-24: Type safety improvement with enum parameter.Changing the parameter from string to TriumphEnum improves type safety and makes the API more explicit.
26-33:Details
✅ Verification successful
Verify enum comparison method.
The strict equality check (
===) between TriumphEnum instances may not work as expected if they are different instances of the same enum value. Consider using a value comparison instead.
🏁 Script executed:
#!/bin/bash # Check if the getName method is implemented to return the enum instance or a value ast-grep --pattern $'public function getName\(\): TriumphEnum { $$$ }' # Also check how the TriumphEnum is implemented and if it overrides equals method ast-grep --pattern $'enum TriumphEnum { $$$ }'Length of output: 123
🏁 Script executed:
#!/bin/bash # Search for the TriumphEnum definition (enum or class or interface) rg -n "^\s*enum\s+TriumphEnum\b" || true rg -n "^\s*(class|interface)\s+TriumphEnum\b" || true # Locate how getName() is implemented in TriumphConfig rg -n "class\s+TriumphConfig" -A5 rg -n "function\s+getName\(" -A5 Api/src/Triumph/Entity/TriumphConfig.phpLength of output: 4474
No action required: strict enum comparison is correct.
In PHP 8.1, each enum case is a singleton, so comparing two instances of the same backed enum case with
===reliably returnstrue. The existing===check ingetByName()will correctly match on enum identity—no changes needed.Api/migrations/Version20250501184525.php (1)
23-35:Details
❓ Verification inconclusive
The migration looks comprehensive, but consider some refinements.
The changes to the triumph_config table schema are well-structured, adding new columns with appropriate defaults and removing/renaming existing ones. A few suggestions:
- The drop of the unique index on 'name' without a replacement constraint could allow duplicates - ensure this is intentional.
- The serialized array format for targeted_event_expected_tags ('a:0:{}') requires proper handling in the application code.
🏁 Script executed:
#!/bin/bash # Check if the application code handles the serialized array format correctly ast-grep --pattern $'public function getTargetedEventExpectedTags\(\): array { $$$ }' # Check if there's any unique constraint defined in the entity rg -A 3 "@ORM\\UniqueConstraint" --type phpLength of output: 223
🏁 Script executed:
#!/bin/bash # Re-check handling of targeted_event_expected_tags in PHP getters rg -n "function getTargetedEventExpectedTags" --type php # Properly escape backslashes to find UniqueConstraint annotations rg -n -e '@ORM\\\\UniqueConstraint' --type phpLength of output: 209
Please verify the dropped unique index and array handling
- The migration drops the unique index on
name(DROP INDEX uniq_a53fc4e45e237e06), and we did not find any@ORM\UniqueConstrainton theTriumphConfigentity. Dropping this index will allow duplicatenamevalues at the database level—ensure this is intentional or consider adding a corresponding ORM-level constraint.- The new
targeted_event_expected_tagscolumn is annotated withCOMMENT ON COLUMN … IS '(DC2Type:array)'. Doctrine’s nativearraytype will automatically unserialize the stored value into a PHP array, and the existing getter in Api/src/Triumph/Entity/TriumphConfig.php (line 96)will return it correctly—no extra handling is required.public function getTargetedEventExpectedTags(): arrayApi/src/Triumph/Service/ChangeTriumphFromEventService.php (1)
49-55: Guard against missing Mush player data
$player->getDaedalus()->getMushPlayers()->getPlayerAlive()assumes every link in the chain returns a valid object.
If the Daedalus instance is mis-configured (e.g.getMushPlayers()returnsnull) this will throw immediately.Consider adding early validation or null-safe navigation (
?->) to avoid hard crashes, or at least document the pre-conditions in the PHPDoc block.Api/src/Game/ConfigData/GameConfigData.php (1)
22-24: Nice decoupling of Triumph config dataImporting the new
TriumphConfigDataand DTO keeps this monster config file from having to know individual triumph keys. Good move.Api/src/Triumph/Entity/TriumphConfig.php (4)
22-33: LGTM! Well-structured entity properties with appropriate typesThe entity properties are well-defined with appropriate ORM annotations and default values. The use of PHP enums (
TriumphEnum,TriumphScope) provides strong type safety for domain-specific values.
49-69: LGTM! Well-designed private constructor with comprehensive parameter listThe private constructor with a comprehensive parameter list ensures that all required properties are set during object creation. This enforces the use of the factory method
fromDtoas the preferred way to create instances, which is a good design pattern.
106-119: LGTM! Well-implemented factory methodThe static factory method
fromDtoprovides a clean way to create new instances from a DTO. This approach encapsulates the creation logic and makes the code more maintainable.
121-132: LGTM! Comprehensive update method for entity modificationThe
updateFromDtomethod provides a clean way to update all properties of the entity from a DTO. This approach ensures that all properties are updated consistently.
|
|
||
| if ($triumphConfig === null) { | ||
| throw new \Exception("Triumph config {$triumphConfigName} not found"); | ||
| throw new \Exception("Triumph config {$triumphConfigName->value} not found"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential error in exception message formatting.
The exception message assumes that $triumphConfigName is an enum object with a value property, but it's being used in a loop over $gameConfigData['triumphConfigs']. If these configs are not enum instances, this will cause a runtime error.
- throw new \Exception("Triumph config {$triumphConfigName->value} not found");
+ throw new \Exception("Triumph config {$triumphConfigName} not found");Alternatively, if $triumphConfigName is indeed an enum, ensure proper string representation:
- throw new \Exception("Triumph config {$triumphConfigName->value} not found");
+ throw new \Exception("Triumph config {$triumphConfigName->name} not found");🏁 Script executed:
#!/bin/bash
# Description: Check how $triumphConfigName is used in GameConfigData
# Look for the definition of triumphConfigs in GameConfigData
echo "Checking GameConfigData for triumphConfigs definition:"
rg -A 5 "triumphConfigs" --glob "*/src/Game/ConfigData/GameConfigData.php"Length of output: 869
Use the raw string in the exception message
In GameConfigData you map each TriumphConfigDto to
$triumphConfig->name->toConfigKey('default')which returns a string (e.g. "ASTEROID_default"). In the loader you’re looping over those strings, so $triumphConfigName is already a string—calling ->value on it will cause a fatal error. Please update the exception to use the string directly:
File: Api/src/Game/ConfigData/GameConfigDataLoader.php (around line 144)
- throw new \Exception("Triumph config {$triumphConfigName->value} not found");
+ throw new \Exception("Triumph config {$triumphConfigName} not found");| trait TriumphSourceEventTrait | ||
| { | ||
| public function hasExpectedTagsFor(TriumphConfig $triumphConfig): bool | ||
| { | ||
| return $this->hasAllTags($triumphConfig->getTargetedEventExpectedTags()); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Implementation of required interface method
The trait provides a clean implementation of the hasExpectedTagsFor method required by the TriumphSourceEventInterface. However, this trait assumes the implementing class has a hasAllTags method, which is not explicitly documented.
Consider adding a comment to document this dependency:
trait TriumphSourceEventTrait
{
+ /**
+ * Implements TriumphSourceEventInterface::hasExpectedTagsFor
+ * Note: Classes using this trait must implement a hasAllTags method.
+ */
public function hasExpectedTagsFor(TriumphConfig $triumphConfig): bool
{
return $this->hasAllTags($triumphConfig->getTargetedEventExpectedTags());
}
}Let's verify where this trait is used to ensure all classes implement hasAllTags:
🏁 Script executed:
#!/bin/bash
# Find all classes that use this trait
echo "Classes using TriumphSourceEventTrait:"
rg -l "use .*TriumphSourceEventTrait" --type php
# Check if they extend AbstractGameEvent or implement hasAllTags
echo -e "\nChecking if these classes have access to hasAllTags method:"
rg -l "use .*TriumphSourceEventTrait" --type php | xargs rg "extends AbstractGameEvent|function hasAllTags"Length of output: 518
Document hasAllTags dependency in TriumphSourceEventTrait
We verified that all consumers of this trait (e.g., DaedalusCycleEvent) extend AbstractGameEvent, which provides the required hasAllTags method. To improve discoverability, add a PHPDoc block above hasExpectedTagsFor noting this dependency:
File: Api/src/Triumph/Event/TriumphSourceEventTrait.php
trait TriumphSourceEventTrait
{
+ /**
+ * Implements TriumphSourceEventInterface::hasExpectedTagsFor.
+ *
+ * Note: Classes using this trait must provide a `hasAllTags(string[]): bool`
+ * method (e.g., via extending AbstractGameEvent).
+ */
public function hasExpectedTagsFor(TriumphConfig $triumphConfig): bool
{
return $this->hasAllTags($triumphConfig->getTargetedEventExpectedTags());
}
}📝 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.
| trait TriumphSourceEventTrait | |
| { | |
| public function hasExpectedTagsFor(TriumphConfig $triumphConfig): bool | |
| { | |
| return $this->hasAllTags($triumphConfig->getTargetedEventExpectedTags()); | |
| } | |
| } | |
| trait TriumphSourceEventTrait | |
| { | |
| /** | |
| * Implements TriumphSourceEventInterface::hasExpectedTagsFor. | |
| * | |
| * Note: Classes using this trait must provide a `hasAllTags(string[]): bool` | |
| * method (e.g., via extending AbstractGameEvent). | |
| */ | |
| public function hasExpectedTagsFor(TriumphConfig $triumphConfig): bool | |
| { | |
| return $this->hasAllTags($triumphConfig->getTargetedEventExpectedTags()); | |
| } | |
| } |
| public function getTargetsForTriumph(TriumphConfig $triumphConfig): PlayerCollection | ||
| { | ||
| return match ($triumphConfig->getScope()) { | ||
| TriumphScope::ALL_ACTIVE_HUMAN => $this->daedalus->getHumanPlayers()->getActivePlayers(), | ||
| TriumphScope::ALL_HUMAN => $this->daedalus->getAlivePlayers()->getHumanPlayer(), | ||
| TriumphScope::ALL_MUSH => $this->daedalus->getAlivePlayers()->getMushPlayer(), | ||
| TriumphScope::PERSONAL => $this->daedalus->getAlivePlayers()->getAllByName($triumphConfig->getTarget()), | ||
| default => throw new \LogicException('Unsupported triumph scope: ' . $triumphConfig->getScope()->value), | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-structured implementation of getTargetsForTriumph.
Good use of the match expression to handle different triumph scopes. The implementation correctly filters players based on the scope defined in the TriumphConfig.
There's an inconsistency in method naming conventions: getHumanPlayer() (singular) vs getActivePlayers() (plural), both returning collections. This inconsistency might confuse developers.
🏁 Script executed:
#!/bin/bash
# Check if the inconsistent method names exist in the PlayerCollection class
rg "getHumanPlayer\(\): self" -A 5
rg "getMushPlayer\(\): self" -A 5Length of output: 1092
Standardize method naming for collection-returning methods
Great use of match to handle each TriumphScope, and the logic correctly filters players. However, there’s an inconsistency in the names of methods that both return collections—some are plural (getActivePlayers(), getAlivePlayers()) while others are singular (getHumanPlayer(), getMushPlayer()). This can be confusing.
Please update the following in Api/src/Player/Entity/Collection/PlayerCollection.php:
- Rename
public function getHumanPlayer(): self→public function getHumanPlayers(): self - Rename
public function getMushPlayer(): self→public function getMushPlayers(): self
And update all call sites (e.g., in Api/src/Daedalus/Event/DaedalusCycleEvent.php) to use the new pluralized names.
| public function __construct( | ||
| private RoomLogServiceInterface $roomLogService, | ||
| private TranslationServiceInterface $translationService, | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused TranslationServiceInterface dependency.
The TranslationServiceInterface is injected in the constructor but never used in any method, violating the principle of only injecting what you need.
public function __construct(
private RoomLogServiceInterface $roomLogService,
- private TranslationServiceInterface $translationService,
) {}📝 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 __construct( | |
| private RoomLogServiceInterface $roomLogService, | |
| private TranslationServiceInterface $translationService, | |
| ) {} | |
| public function __construct( | |
| private RoomLogServiceInterface $roomLogService, | |
| ) {} |
| public static function getByName(TriumphEnum $name): TriumphConfigDto | ||
| { | ||
| return current( | ||
| array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle potential empty result in getByName method
The current implementation of getByName() uses current() on the filtered array without checking if any items were found. If no DTO matches the provided name, this will return false instead of a TriumphConfigDto, potentially causing type errors downstream.
public static function getByName(TriumphEnum $name): TriumphConfigDto
{
- return current(
- array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name)
- );
+ $filtered = array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name);
+
+ if (empty($filtered)) {
+ throw new \InvalidArgumentException(sprintf('No triumph config found for name: %s', $name->value));
+ }
+
+ return current($filtered);
}📝 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 static function getByName(TriumphEnum $name): TriumphConfigDto | |
| { | |
| return current( | |
| array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name) | |
| ); | |
| } | |
| public static function getByName(TriumphEnum $name): TriumphConfigDto | |
| { | |
| - return current( | |
| - array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name) | |
| - ); | |
| + $filtered = array_filter( | |
| + self::getAll(), | |
| + static fn (TriumphConfigDto $dto) => $dto->name === $name | |
| + ); | |
| + | |
| + if (empty($filtered)) { | |
| + throw new \InvalidArgumentException( | |
| + sprintf('No triumph config found for name: %s', $name->value) | |
| + ); | |
| + } | |
| + | |
| + return current($filtered); | |
| } |
Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
Show resolved
Hide resolved
| public function getName(): TriumphEnum | ||
| { | ||
| return $this->name; | ||
| } | ||
|
|
||
| public function getQuantity(): int | ||
| { | ||
| return $this->quantity; | ||
| } | ||
|
|
||
| public function getScope(): TriumphScope | ||
| { | ||
| return $this->scope; | ||
| } | ||
|
|
||
| public function getTarget(): string | ||
| { | ||
| return $this->target; | ||
| } | ||
|
|
||
| public function getTargetedEvent(): string | ||
| { | ||
| return $this->targetedEvent; | ||
| } | ||
|
|
||
| public function getTargetedEventExpectedTags(): array | ||
| { | ||
| return $this->targetedEventExpectedTags; | ||
| } | ||
|
|
||
| public function getVisibility(): TriumphVisibility | ||
| { | ||
| return $this->visibility; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing getters for key and regressiveFactor properties
The class provides getters for most properties but is missing getters for key and regressiveFactor. For consistency and to ensure complete access to all properties, these getters should be added:
public function getKey(): string
{
return $this->key;
}
public function getRegressiveFactor(): int
{
return $this->regressiveFactor;
}
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
b6f5fad to
1c92daa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Api/src/Triumph/ConfigData/TriumphConfigData.php (1)
72-77: Handle potential empty result in getByName methodThe current implementation of
getByName()usescurrent()on the filtered array without checking if any items were found. If no DTO matches the provided name, this will returnfalseinstead of aTriumphConfigDto, potentially causing type errors downstream.public static function getByName(TriumphEnum $name): TriumphConfigDto { - return current( - array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name) - ); + $filtered = array_filter(self::getAll(), static fn (TriumphConfigDto $dto) => $dto->name === $name); + + if (empty($filtered)) { + throw new \InvalidArgumentException(sprintf('No triumph config found for name: %s', $name->value)); + } + + return current($filtered); }
🧹 Nitpick comments (2)
Api/src/Triumph/config/services.yaml (2)
6-12: Consider restricting the resource to PHP files only.
Right now,resource: '../'will scan all file types undersrc/Triumph/. To avoid unintentionally loading non-PHP files (YAML, JSON, etc.), you can narrow the pattern to PHP classes:- Mush\Triumph\: - resource: '../' + Mush\Triumph\: + resource: '../**/*.php' exclude: - '../DependencyInjection/' - '../Entity/' - '../Kernel.php' - '../Tests/'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
12-12: Add a newline at end-of-file.
YAMLLint reports a missing trailing newline. Please ensure the file ends with a blank line.--- a/Api/src/Triumph/config/services.yaml +++ b/Api/src/Triumph/config/services.yaml @@ -12 +12 - - '../Tests/' + - '../Tests/' +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(2 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/DaedalusCycleEventSubscriber.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)
💤 Files with no reviewable changes (7)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/Enum/TriumphEnum.php
✅ Files skipped from review due to trivial changes (1)
- Api/translations/fr/triumph+intl-icu.fr.xlf
🚧 Files skipped from review as they are similar to previous changes (40)
- Api/config/services.yaml
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/config/api_platform/resources/game_config.yaml
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/composer.json
- Api/config/api_platform/resources/triumph_config.yaml
- Api/src/Player/Service/PlayerService.php
- Api/config/packages/doctrine.yaml
- Api/src/Game/Entity/GameConfig.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Daedalus/Entity/Daedalus.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/migrations/Version20250501184525.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/src/Game/ConfigData/GameConfigData.php
- Api/src/Triumph/Entity/TriumphConfig.php
- Api/src/Triumph/Listener/DaedalusCycleEventSubscriber.php
🧰 Additional context used
🪛 YAMLlint (1.35.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
Api/src/Triumph/config/services.yaml (1)
1-5: Defaults block looks good.
The_defaultssection correctly enablesautowireandautoconfigure, matching Symfony best practices for service loading in this module.Api/src/Triumph/ConfigData/TriumphConfigData.php (1)
21-70: LGTM! Well-structured triumph configuration data.The
getAll()method provides a clean way to define triumph configurations using the DTO pattern. Each triumph config is clearly defined with appropriate parameters for key, name, targeted events, scope, and quantity.
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
1c92daa to
b6f5fad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Api/migrations/Version20250427095647.php (1)
17-17: Consider adding a descriptive migration messageProviding a meaningful description would help other developers understand the purpose of this migration without having to read the code.
- return ''; + return 'Remove triumph column from player table as part of Triumph domain refactoring';Api/src/Triumph/config/services.yaml (1)
12-12: Add newline at end of fileStatic analysis indicates there's no newline at the end of file, which should be added for consistency with coding standards.
- '../Entity/' - '../Kernel.php' - - '../Tests/' + - '../Tests/' +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/tests/functional/Triumph/Event/DaedalusEventCest.php (1)
38-38: Consider adding more test coverageConsider adding additional test cases to cover different scenarios such as:
- Events that should not award triumph points
- Different tag combinations that award different amounts
- Multiple players receiving triumph points
This would ensure the triumph awarding mechanism is thoroughly tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250427095647.php(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(2 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php(1 hunks)Api/tests/functional/Triumph/Event/DaedalusEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)
💤 Files with no reviewable changes (7)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/Enum/TriumphEnum.php
🚧 Files skipped from review as they are similar to previous changes (41)
- Api/config/services.yaml
- Api/composer.json
- Api/config/api_platform/resources/triumph_config.yaml
- Api/config/api_platform/resources/game_config.yaml
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/src/Player/Service/PlayerService.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/src/Game/Entity/GameConfig.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/config/packages/doctrine.yaml
- Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Daedalus/Entity/Daedalus.php
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/migrations/Version20250501184525.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Game/ConfigData/GameConfigData.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/src/Triumph/Entity/TriumphConfig.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
Api/tests/functional/Triumph/Event/DaedalusEventCest.php (3)
App/src/entities/Daedalus.ts (1)
Daedalus(23-124)Api/tests/_support/AbstractFunctionalTest.php (1)
AbstractFunctionalTest(41-333)Api/src/Player/Entity/Player.php (2)
setTriumph(765-770)getTriumph(760-763)
🪛 YAMLlint (1.35.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
Api/migrations/Version20250427095647.php (1)
29-29: LGTM - Appropriate default value for rollbackThe
NOT NULL DEFAULT 0constraint ensures that existing player records won't have null values when rolling back the migration, which is essential for database integrity.Api/src/Triumph/config/services.yaml (1)
1-12: Service configuration looks goodThe configuration follows standard Symfony service pattern with appropriate autowiring and exclusions for the new Triumph namespace.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/tests/functional/Triumph/Event/DaedalusEventCest.php (1)
27-39: Test correctly verifies triumph awarding functionalityThe test properly validates that a DaedalusEvent with FINISH_DAEDALUS type and RETURN_TO_SOL tag awards 20 triumph points to the player.
Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php (1)
13-36: Well-structured event subscriber with appropriate event prioritiesThe subscriber properly bridges Daedalus lifecycle events to the triumph system with appropriate priorities, following the single responsibility principle by delegating actual processing to the specialized service.
cmnemoi
force-pushed
the
feat/triumph-service
branch
2 times, most recently
from
b6f5fad to
00bf8e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Api/src/Triumph/config/services.yaml (1)
12-12: Add a newline at end of fileYAML files should end with a newline character to satisfy common linters (e.g., yamllint) and POSIX standards.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(2 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php(1 hunks)Api/tests/functional/Triumph/Event/DaedalusEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)
💤 Files with no reviewable changes (7)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/Enum/TriumphEnum.php
🚧 Files skipped from review as they are similar to previous changes (43)
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/composer.json
- Api/config/services.yaml
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/src/Game/Entity/GameConfig.php
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Player/Service/PlayerService.php
- Api/config/api_platform/resources/triumph_config.yaml
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Daedalus/Entity/Daedalus.php
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/config/api_platform/resources/game_config.yaml
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/tests/functional/Triumph/Event/DaedalusEventCest.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/migrations/Version20250501184525.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Game/ConfigData/GameConfigData.php
- Api/src/Triumph/Entity/TriumphConfig.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/config/packages/doctrine.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
Api/src/Triumph/config/services.yaml (2)
1-5: Autowiring and autoconfiguration defaults set correctlySetting
autowire: trueandautoconfigure: trueunder_defaultsaligns with Symfony best practices, ensuring that services under this namespace are automatically wired and configured.
6-12: Namespace resource and exclusion rules look correctThe
Mush\Triumph\:resource definition correctly points to the parent directory, and excludingDependencyInjection/,Entity/,Kernel.php, andTests/is appropriate to avoid wiring non-service classes. This matches the recommended Symfony service loading pattern.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
00bf8e9 to
14cd40b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Api/src/Triumph/config/services.yaml (3)
6-7: Use an explicit glob for the resource path.Symfony’s service loader typically expects a glob pattern to target PHP files. Changing from a bare directory to a glob (e.g.
../*) makes it clear which files are picked up:- Mush\Triumph\: - resource: '../' + Mush\Triumph\: + resource: '../*'
8-12: Simplify the exclude list with brace expansion.You can reduce repetition and improve readability by grouping excluded paths in one line:
- exclude: - - '../DependencyInjection/' - - '../Entity/' - - '../Kernel.php' - - '../Tests/' + exclude: '../{DependencyInjection,Entity,Kernel.php,Tests}'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
12-12: Add a trailing newline.YAML linters expect a newline at end-of-file. Please append a blank line to satisfy lint rules.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(2 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php(1 hunks)Api/tests/functional/Triumph/Event/DaedalusEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)
💤 Files with no reviewable changes (7)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/Enum/TriumphEnum.php
✅ Files skipped from review due to trivial changes (1)
- Api/migrations/Version20250501184525.php
🚧 Files skipped from review as they are similar to previous changes (42)
- Api/composer.json
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/config/api_platform/resources/triumph_config.yaml
- Api/config/api_platform/resources/game_config.yaml
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/config/packages/doctrine.yaml
- Api/config/services.yaml
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Player/Service/PlayerService.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/tests/functional/Triumph/Event/DaedalusEventCest.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/Entity/TriumphConfig.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/src/Game/Entity/GameConfig.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/src/Daedalus/Entity/Daedalus.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/src/Game/ConfigData/GameConfigData.php
🧰 Additional context used
🪛 YAMLlint (1.35.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
Api/src/Triumph/config/services.yaml (1)
1-12: Verify import in main service config.Make sure this new Triumph service definition is actually loaded by your kernel. For instance, in
Api/config/services.yamlyou should have:imports: - { resource: '../src/Triumph/config/services.yaml' }Otherwise Symfony won’t register the
Mush\Triumph\services.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Api/src/Triumph/Enum/TriumphScope.php (2)
9-12: Consider adding PHPDoc comments for improved documentation.Adding PHPDoc comments explaining the purpose and usage context of each scope would improve code readability and assist developers who are new to the codebase in understanding when to use each scope type.
+/** + * Defines the scope of players eligible for triumphs. + */ enum TriumphScope: string { + /** All human players who are still active in the game */ case ALL_ACTIVE_HUMANS = 'all_active_humans'; + /** All human players who are still alive */ case ALL_ALIVE_HUMANS = 'all_alive_humans'; + /** All players who are Mushs */ case ALL_MUSHS = 'all_mushs'; + /** Only applies to a specific player */ case PERSONAL = 'personal';
14-14: Reconsider NULL case naming vs value representation.The NULL case with an empty string value might lead to confusion. In PHP, empty strings are falsy but not NULL. Consider either renaming this to NONE/EMPTY or using a more descriptive name that better indicates the empty string value.
- case NULL = ''; + case NONE = '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
🔇 Additional comments (1)
Api/src/Triumph/Enum/TriumphScope.php (1)
1-15: Well-structured enum implementation for triumph scopes.The implementation of this string-backed enum for triumph scopes is clean and follows PHP coding conventions. Using an enum for this purpose is an excellent choice for type safety and code readability.
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
e0b515c to
79134d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Api/src/Triumph/config/services.yaml (2)
6-12: Validate resource path and exclusions.
Theresource: '../'is intended to load everything in the Triumph module (relative to this config file). Theexcludeentries will skipDependencyInjection/,Entity/,Kernel.php, andTests/directories.
Nitpick: consider quoting the key to avoid any YAML parsing edge cases, e.g.:"Mush\\Triumph\\": resource: '../' exclude: - '../DependencyInjection/' - '../Entity/' - '../Kernel.php' - '../Tests/'🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
12-12: Add a trailing newline at end-of-file.
YAML linters commonly expect a final newline; adding one will eliminate the “no new line character at end of file” warning.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(2 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php(1 hunks)Api/tests/functional/Triumph/Event/DaedalusEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)
💤 Files with no reviewable changes (7)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/Enum/TriumphEnum.php
🚧 Files skipped from review as they are similar to previous changes (43)
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/config/services.yaml
- Api/config/api_platform/resources/game_config.yaml
- Api/composer.json
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/config/packages/doctrine.yaml
- Api/config/api_platform/resources/triumph_config.yaml
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/src/Game/Entity/GameConfig.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Player/Service/PlayerService.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/src/Daedalus/Entity/Daedalus.php
- Api/tests/functional/Triumph/Event/DaedalusEventCest.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/tests/functional/Triumph/Event/DaedalusCycleEventCest.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/src/Game/ConfigData/GameConfigData.php
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/migrations/Version20250501184525.php
- Api/src/Triumph/Entity/TriumphConfig.php
🧰 Additional context used
🪛 YAMLlint (1.37.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
Api/src/Triumph/config/services.yaml (1)
2-4: Defaults are correctly configured for autowiring and autoconfiguration.
This ensures all Triumph services under theMush\Triumph\namespace will be auto-discovered, wired, and tagged appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php (3)
28-41: Consider making expected triumph values configurable.The test correctly verifies that a
DaedalusCycleEventawards 1 triumph point. However, the expected value1is hardcoded, which could make the test brittle if triumph configurations change.Consider extracting the expected value to a constant or loading it from the triumph configuration:
public function shouldGiveTriumphOnDaedalusNewCycleEvent(FunctionalTester $I): void { $this->player->setTriumph(0); + $expectedTriumph = 1; // or load from TriumphConfig $event = new DaedalusCycleEvent( daedalus: $this->daedalus, tags: [], time: new \DateTime(), ); $this->eventService->callEvent($event, DaedalusCycleEvent::DAEDALUS_NEW_CYCLE); // human cyclic triumph - $I->assertEquals(1, $this->player->getTriumph()); + $I->assertEquals($expectedTriumph, $this->player->getTriumph()); }
43-56: Consider making expected triumph values configurable.The test correctly verifies that a
DaedalusEventwithRETURN_TO_SOLtag awards 20 triumph points. Similar to the previous test, the hardcoded expected value could be made more maintainable.Consider extracting the expected value to make the test more maintainable:
public function shouldGiveTriumphOnDaedalusEvent(FunctionalTester $I): void { $this->player->setTriumph(0); + $expectedTriumph = 20; // or load from TriumphConfig $event = new DaedalusEvent( daedalus: $this->daedalus, tags: [ActionEnum::RETURN_TO_SOL->toString()], time: new \DateTime(), ); $this->eventService->callEvent($event, DaedalusEvent::FINISH_DAEDALUS); // return to sol human triumph - $I->assertEquals(20, $this->player->getTriumph()); + $I->assertEquals($expectedTriumph, $this->player->getTriumph()); }
35-35: Consider potential DateTime-related test flakiness.Using
new \DateTime()could potentially cause rare test flakiness if the system clock changes between event creation and processing, though this is unlikely in practice.For more deterministic tests, consider using a fixed DateTime:
-time: new \DateTime(), +time: new \DateTime('2023-01-01 12:00:00'),Also applies to: 50-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php(1 hunks)
🔇 Additional comments (2)
Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php (2)
1-13: LGTM: Clean imports and file structure.The file follows PHP best practices with proper strict typing, appropriate namespace, and well-organized imports. The use of
@internalannotation correctly indicates this is an internal test class.
17-26: LGTM: Proper test setup.The class properly extends
AbstractFunctionalTestand correctly initializes theEventServiceInterfacefrom the service container in the setup method.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Api/src/Triumph/ConfigData/TriumphConfigData.php (1)
79-84: Handle potential empty result in getByName methodThe current implementation has a type safety issue that was previously identified.
This method can return
falsewhen no matching DTO is found, which violates the return type declaration and can cause downstream type errors.
🧹 Nitpick comments (9)
Api/src/Triumph/Enum/TriumphScope.php (2)
9-12: Consider grammatical consistency in naming.The plural form "MUSHS" is grammatically incorrect. Consider using "MUSHES" for consistency with English pluralization rules.
- case ALL_ALIVE_MUSHS = 'all_alive_mushs'; - case ALL_MUSHS = 'all_mushs'; + case ALL_ALIVE_MUSHES = 'all_alive_mushes'; + case ALL_MUSHES = 'all_mushes';
15-15: Clarify the purpose of the NULL case.Having a
NULLcase with an empty string value might be confusing. Consider adding a docblock to explain when this case should be used, or rename it to be more descriptive (e.g.,NONE,UNSPECIFIED).Api/src/Triumph/README.md (2)
6-9: Fix markdown list indentation for consistency.The nested list items have incorrect indentation according to markdown standards.
- If the triumph listens to a new event, it should : - - implement [TriumphSourceEventInterface](./Event/TriumphSourceEventInterface.php) - - be added to [TriumphSourceEventSubscriber](./Listener/TriumphSourceEventSubscriber.php) + - implement [TriumphSourceEventInterface](./Event/TriumphSourceEventInterface.php) + - be added to [TriumphSourceEventSubscriber](./Listener/TriumphSourceEventSubscriber.php)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
1-10: Consider expanding documentation with examples.The current documentation covers the basic steps but could benefit from concrete examples to help developers understand the triumph system better.
Consider adding:
- An example of a complete triumph configuration
- Explanation of different triumph scopes and when to use them
- Example event implementation with TriumphSourceEventInterface
- Common patterns and best practices
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php (5)
38-49: Add @test annotation and consider using a constant for the magic number.The test method lacks the
@testannotation which may be required depending on PHPUnit configuration. Also, the magic number120should be extracted to a constant or retrieved from configuration for better maintainability.+ /** @test */ public function shouldMushPlayersGain120TriumphPointsWhenDaedalusIsFull(): void { + $expectedTriumphPoints = 120; // Or better: retrieve from TriumphConfigData $daedalus = DaedalusFactory::createDaedalus(); $mushPlayer1 = $this->givenMushPlayer($daedalus); $mushPlayer2 = $this->givenMushPlayer($daedalus); $this->givenPlayersInitialTriumph([$mushPlayer1, $mushPlayer2], 0); $this->givenATriumphConfig(TriumphEnum::MUSH_INITIAL_BONUS); $this->whenDaedalusIsFull($daedalus); - $this->thenPlayersShouldHaveTriumph([$mushPlayer1, $mushPlayer2], 120); + $this->thenPlayersShouldHaveTriumph([$mushPlayer1, $mushPlayer2], $expectedTriumphPoints); }
51-62: Add @test annotation for consistency.The second test method also needs the
@testannotation for consistency and to ensure it's properly recognized by PHPUnit.+ /** @test */ public function shouldHumanPlayersNotGainTriumphPointsWhenDaedalusIsFull(): void
92-97: Add specific type hint for the array parameter.The
arraytype hint could be more specific to improve type safety and code documentation.- private function givenPlayersInitialTriumph(array $players, int $initialTriumph): void + /** @param Player[] $players */ + private function givenPlayersInitialTriumph(array $players, int $initialTriumph): void
107-112: Specify timezone for DateTime to ensure consistent behavior.Creating a DateTime without specifying the timezone can lead to inconsistent behavior across different environments.
$event = new DaedalusEvent( daedalus: $daedalus, tags: [], - time: new \DateTime(), + time: new \DateTime('now', new \DateTimeZone('UTC')), );
117-122: Add specific type hint and return type declaration.Similar to the previous array parameter, this method would benefit from more specific type hints and a return type declaration.
- private function thenPlayersShouldHaveTriumph(array $players, int $expectedTriumph): void + /** @param Player[] $players */ + private function thenPlayersShouldHaveTriumph(array $players, int $expectedTriumph): void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/README.md(1 hunks)Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php(1 hunks)Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)App/src/components/Game/BannerPanel.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
Api/src/Triumph/README.md
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (2)
Api/src/Triumph/ConfigData/TriumphConfigData.php (1)
36-36:Details
❓ Verification inconclusive
Verify intentional use of negative triumph quantities.
Some triumph configurations use negative quantities (e.g.,
-2for CYCLE_MUSH,-10for SOL_MUSH_INTRUDER). Please confirm this is intentional behavior for penalty/deduction scenarios.Also applies to: 67-67
🏁 Script executed:
#!/bin/bash # Description: Check if negative triumph quantities are handled properly in the codebase # Expected: Find code that handles both positive and negative triumph values echo "Searching for triumph quantity handling..." rg -A 5 -B 5 "triumph.*quantity|quantity.*triumph" --type php echo "Searching for negative number handling in triumph contexts..." rg -A 3 -B 3 "\-\d+.*triumph|triumph.*\-\d+" --type phpLength of output: 6638
Confirm intentional use of negative Triumph quantities
Please verify that using negative
quantityvalues in your Triumph configurations (e.g.-2on line 36 and-10on line 67 ofApi/src/Triumph/ConfigData/TriumphConfigData.php) is deliberate for penalty/deduction scenarios, since there’s no explicit code path filtering or adjusting negative amounts.• Review
computeTriumphForPlayer()inApi/src/Triumph/Service/ChangeTriumphFromEventService.phpto ensure negative quantities are applied as intended.
• Ensure downstream logic (e.g.,Player::addTriumph(), event handlers) handles a negativequantitycorrectly.Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php (1)
25-123: Well-structured test class with good separation of concerns.The test class follows good practices:
- Uses the Given-When-Then pattern for readability
- Properly isolates test setup
- Tests both positive and negative scenarios
- Uses appropriate factories for object creation
- Follows PHPUnit conventions
The implementation correctly verifies that only mush players receive triumph points when the Daedalus is full, while human players do not.
| <span> | ||
| {{ player.triumph?.quantity }} | ||
| <img v-if="player.isMush()" :src="getImgUrl('ui_icons/player_variables/triumph_mush.png')"> | ||
| <img v-else :src="getImgUrl('ui_icons/player_variables/triumph.png')"> | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add alt attributes for accessibility and improve defensive programming.
The conditional triumph icon rendering looks correct, but there are opportunities for improvement:
- Both images are missing
altattributes for accessibility - The code could be more defensive against potential null/undefined values
<span>
{{ player.triumph?.quantity }}
- <img v-if="player.isMush()" :src="getImgUrl('ui_icons/player_variables/triumph_mush.png')">
- <img v-else :src="getImgUrl('ui_icons/player_variables/triumph.png')">
+ <img
+ v-if="player?.isMush()"
+ :src="getImgUrl('ui_icons/player_variables/triumph_mush.png')"
+ alt="Mush triumph"
+ >
+ <img
+ v-else-if="player"
+ :src="getImgUrl('ui_icons/player_variables/triumph.png')"
+ alt="Human triumph"
+ >
</span>📝 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.
| <span> | |
| {{ player.triumph?.quantity }} | |
| <img v-if="player.isMush()" :src="getImgUrl('ui_icons/player_variables/triumph_mush.png')"> | |
| <img v-else :src="getImgUrl('ui_icons/player_variables/triumph.png')"> | |
| </span> | |
| <span> | |
| {{ player.triumph?.quantity }} | |
| <img | |
| v-if="player?.isMush()" | |
| :src="getImgUrl('ui_icons/player_variables/triumph_mush.png')" | |
| alt="Mush triumph" | |
| > | |
| <img | |
| v-else-if="player" | |
| :src="getImgUrl('ui_icons/player_variables/triumph.png')" | |
| alt="Human triumph" | |
| > | |
| </span> |
🤖 Prompt for AI Agents
In App/src/components/Game/BannerPanel.vue around lines 62 to 66, add meaningful
alt attributes to both img tags to improve accessibility. Also, enhance
defensive programming by ensuring player.triumph and player.isMush() are safely
accessed, for example by checking their existence before usage or using optional
chaining to avoid runtime errors if these values are null or undefined.
f0fae08 to
2049d28
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Api/src/Triumph/README.md (7)
1-3: Fix pluralization in the introduction
Change “Triumph are points awarded to players based on specific game events.” to “Triumphs are points awarded to players based on specific game events.” for correct subject-verb agreement.
7-11: Standardize heading capitalization
Update the section title from## Triumph config Attributesto
## Triumph Config Attributesto match title-case styling used elsewhere in the README.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...following relevant attributes: -name: triumph name (from [TriumphEnum](./En...(UNLIKELY_OPENING_PUNCTUATION)
14-14: Clarify the TODO intargetedEventExpectedTags
Currently the note says “(for the moment, all tags must be present for the triumph to apply - TODO)”. Consider splitting this into a clear two-sentence note or link to an issue so it reads as a deliberate caveat, e.g.:For now, all tags in
targetedEventExpectedTagsmust be present.
TODO: support partial tag matching.
20-20: Correct grammar inregressiveFactordescription
Change “the gains has 2x less chance to be earned” to “the gains have half the chance of being earned” (or similar), for proper verb agreement and clarity.🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...(NOUN_PLURAL_HAS)
32-32: Use “restrict” instead of “restrain”
Modify the sentence:You can add new scopes to restrain your triumph targets.to
You can add new scopes to restrict which players receive triumphs.for more accurate wording.
34-41: Clarify “add the triumph log” step
In step 3, “Add the triumph log in [triumph+intl-icu.fr.xlf]” could be rephrased to:Add a translation entry for this triumph in
translations/fr/triumph+intl-icu.fr.xlf.
This makes it clearer that you’re updating the French translation file.
44-58: Polish the concrete example phrasing and code snippet
- Reword the introduction to the snippet as:
Here’s how to configure the personal “Chun lives!” triumph for Chun (+1 point per day):
- Add the missing semicolon in the PHP example to make it syntactically valid:
```php new TriumphConfigDto( key: TriumphEnum::CHUN_LIVES->toConfigKey('default'), name: TriumphEnum::CHUN_LIVES, targetedEvent: DaedalusCycleEvent::DAEDALUS_NEW_CYCLE, targetedEventExpectedTags: [ EventEnum::NEW_DAY, ], scope: TriumphScope::PERSONAL, target: CharacterEnum::CHUN, quantity: 1, ) // ← add ';' here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Api/src/Triumph/README.md(1 hunks)Api/tests/functional/Player/Event/PlayerCycleEventCest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Api/tests/functional/Player/Event/PlayerCycleEventCest.php
🧰 Additional context used
🪛 LanguageTool
Api/src/Triumph/README.md
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...following relevant attributes: - name: triumph name (from [TriumphEnum](./En...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...
(NOUN_PLURAL_HAS)
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...eceive a triumph: - ALL_ACTIVE_HUMANS: All human players who are currently act...
(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Api/src/Triumph/Enum/TriumphVisibility.php (2)
13-16: Consider if the toString() method is necessary.The
toString()method is redundant since PHP string-backed enums can be used directly in string contexts and already expose their value via the$enum->valueproperty.However, I notice this pattern is consistent with
TriumphEnumbased on the relevant code snippets, so this might be intentional for codebase consistency.If string conversion is needed frequently, consider implementing
__toString()instead for automatic string casting:- public function toString(): string + public function __toString(): string { return $this->value; }Or if consistency with other triumph enums is the goal, consider adding a docblock explaining the purpose.
7-17: Add documentation to clarify the purpose and usage of each visibility level.The enum lacks documentation explaining when each visibility level should be used and their business impact within the triumph system.
Consider adding class and case documentation:
+/** + * Defines visibility levels for triumph configurations. + * Used to control how triumphs are displayed and logged. + */ enum TriumphVisibility: string { + /** Triumph is visible only to the player who achieved it */ case PRIVATE = 'private'; + + /** Triumph is not displayed but may be logged internally */ case HIDDEN = 'hidden'; + + /** Default visibility level */ case NONE = ''; + /** + * Returns the string representation of the visibility level. + */ public function toString(): string { return $this->value; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Triumph/Entity/TriumphConfig.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
Api/src/Triumph/Enum/TriumphVisibility.php (1)
Api/src/Triumph/Enum/TriumphEnum.php (1)
toString(22-25)
| enum TriumphVisibility: string | ||
| { | ||
| case PRIVATE = 'private'; | ||
| case HIDDEN = 'hidden'; | ||
| case NONE = ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a non-empty string value for the NONE case.
The NONE case uses an empty string value, which could lead to potential issues:
- Database and API concerns: Empty strings can cause confusion in persistence layers and API responses
- Validation complexity: Empty string values may require special handling in validation rules
- Semantic clarity: The distinction between
HIDDENandNONE(empty) might not be immediately clear to developers
Consider using a descriptive string value instead:
- case NONE = '';
+ case NONE = 'none';Or if the empty string is intentional for specific business logic, please add documentation explaining its purpose.
📝 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.
| enum TriumphVisibility: string | |
| { | |
| case PRIVATE = 'private'; | |
| case HIDDEN = 'hidden'; | |
| case NONE = ''; | |
| enum TriumphVisibility: string | |
| { | |
| case PRIVATE = 'private'; | |
| case HIDDEN = 'hidden'; | |
| - case NONE = ''; | |
| + case NONE = 'none'; | |
| } |
🤖 Prompt for AI Agents
In Api/src/Triumph/Enum/TriumphVisibility.php around lines 7 to 11, the NONE
enum case uses an empty string which can cause issues with database storage, API
responses, and validation. Replace the empty string with a descriptive non-empty
string value that clearly distinguishes NONE from other cases, or if the empty
string is required for specific logic, add a comment explaining its purpose to
improve code clarity and maintainability.
e5aff52 to
98bc8b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Api/src/Triumph/config/services.yaml (1)
12-12: Add missing newline at end of file.The file should end with a newline character for better POSIX compliance.
- - '../Tests/' + - '../Tests/' +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/src/Triumph/README.md (2)
20-20: Fix grammar error in regressiveFactor description.The sentence has a subject-verb agreement error.
-- `regressiveFactor`: Determines after how many gains the gains has 2x less chance to be earned (TODO) +- `regressiveFactor`: Determines after how many gains the gain has 2x less chance to be earned (TODO)🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...(NOUN_PLURAL_HAS)
3-3: Fix grammar in triumph definition.The sentence should use plural verb form.
-Triumph are points awarded to players based on specific game events. +Triumphs are points awarded to players based on specific game events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(0 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/README.md(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Player/Event/PlayerCycleEventCest.php(2 hunks)Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Player/Service/PlayerServiceTest.php(0 hunks)Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)App/src/components/Game/BannerPanel.vue(1 hunks)
💤 Files with no reviewable changes (9)
- Api/config/routes/annotations.yaml
- Api/src/Player/Service/PlayerService.php
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/tests/unit/Player/Service/PlayerServiceTest.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/Enum/TriumphEnum.php
✅ Files skipped from review due to trivial changes (1)
- Api/src/Triumph/Event/TriumphChangedEvent.php
🚧 Files skipped from review as they are similar to previous changes (42)
- Api/config/services.yaml
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/composer.json
- App/src/components/Game/BannerPanel.vue
- Api/config/api_platform/resources/triumph_config.yaml
- Api/src/Game/Entity/GameConfig.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/config/packages/doctrine.yaml
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Daedalus/Entity/Daedalus.php
- Api/tests/functional/Player/Event/PlayerCycleEventCest.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/config/api_platform/resources/game_config.yaml
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/src/Game/ConfigData/GameConfigData.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php
- Api/src/Triumph/Entity/TriumphConfig.php
🧰 Additional context used
🪛 LanguageTool
Api/src/Triumph/README.md
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...following relevant attributes: - name: triumph name (from [TriumphEnum](./En...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...
(NOUN_PLURAL_HAS)
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...eceive a triumph: - ALL_ACTIVE_HUMANS: All human players who are currently act...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 YAMLlint (1.37.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
Api/src/Triumph/config/services.yaml (1)
1-12: LGTM! Service configuration follows Symfony best practices.The configuration properly sets up autowiring and autoconfiguration for the Triumph domain with appropriate exclusions.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/migrations/Version20250501184525.php (4)
24-29: Schema additions look good with safe defaults.The new columns are added with appropriate default values and NOT NULL constraints, ensuring data integrity during migration.
37-53: Down migration appears complete but verify data restoration.The down migration properly reverses all changes, but the restoration of dropped data (particularly the
is_all_crewcolumn) will use default values, potentially losing original data.Consider adding a comment or warning about potential data loss during rollback, especially for the
is_all_crewcolumn that gets a default FALSE value.
23-23:Details
❓ Verification inconclusive
Verify that removing the unique constraint on 'name' is intentional.
Dropping the unique index on the
namecolumn will allow duplicate triumph names, which may not be the intended behavior.
🏁 Script executed:
#!/bin/bash # Description: Check if the new schema still maintains uniqueness through other means # Expected: Find if there are other unique constraints or if duplicates are now allowed by design # Check the new TriumphConfig entity for unique constraints ast-grep --pattern 'class TriumphConfig { $$$ @UniqueEntity($_) $$$ }' # Check if the key field (renamed from team) has unique constraints rg -A 5 -B 5 "@UniqueEntity\|unique.*key\|unique.*name" --type phpLength of output: 147
I can’t find any evidence of a replacement unique constraint on
nameorkeyin annotations or mapping files. Let’s search more broadly for anyuniqueConstraint(s)orUniqueEntitydeclarations:
🏁 Script executed:
#!/bin/bash # Search for any unique-constraint declarations across the codebase # 1. Look for Doctrine annotation-based unique constraints on entities rg -n "@UniqueEntity" . rg -n "@ORM\\UniqueConstraint" . # 2. Look for table-level unique constraints in mapping files (YAML, XML, PHP) rg -n "uniqueConstraints" . rg -n "triumph_config" . # 3. Verify no ALTER TABLE … ADD CONSTRAINT statements later in migrations rg -n "ADD CONSTRAINT.*triumph_config" Api/migrationsLength of output: 8481
Confirm removal of the unique index on
nameI didn’t find any
@UniqueEntityor@ORM\UniqueConstraintannotation on theTriumphConfigentity, nor any table-leveluniqueConstraintsin mappings that would replace it. Dropping the index in:
- Api/migrations/Version20250501184525.php (line 23):
$this->addSql('DROP INDEX uniq_a53fc4e45e237e06');will allow duplicate
namevalues. Please verify this change is intentional and that no part of the application relies onnameremaining unique.
30-34:Details
❓ Verification inconclusive
Column renames and removals need coordination verification.
The migration renames critical columns (
teamtokey,triumphtoquantity) and removesis_all_crew. Ensure all code references have been updated to use the new column names.
🏁 Script executed:
#!/bin/bash # Description: Verify that all code references to old column names have been updated # Expected: No references to old column names in PHP code echo "Checking for references to old column names..." # Check for references to old 'team' property rg -t php "->team\b|\['team'\]|\[\"team\"\]" --exclude="*Migration*" # Check for references to old 'triumph' property rg -t php "->triumph\b|\['triumph'\]|\[\"triumph\"\]" --exclude="*Migration*" # Check for references to old 'is_all_crew' property rg -t php "->is_all_crew\b|isAllCrew|\['is_all_crew'\]|\[\"is_all_crew\"\]" --exclude="*Migration*"Length of output: 1582
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Rechecking code references to old column names…" # Patterns to search for old columns patterns=( "->team\b" "\['team'\]" '["]team["]' "->triumph\b" "\['triumph'\]" '["]triumph["]' "->is_all_crew\b" "isAllCrew\b" "\['is_all_crew'\]" '["]is_all_crew["]' ) for p in "${patterns[@]}"; do echo -e "\nSearching for pattern: $p" rg -t php -e "$p" --exclude="*Migration*" || echo " → No matches for '$p'" doneLength of output: 7007
#!/usr/bin/env bash set -euo pipefail echo "Verifying no old column references remain…" search_dir="." exclude_dir="Api/migrations" function search() { local pattern="$1" local desc="$2" echo -e "\n--- Searching for $desc ---" grep -R -n -P "$pattern" "$search_dir" --exclude-dir="$exclude_dir" || echo "No matches for: $desc" } search '->team\b' "property access ->team" search '\[\'team\'\]' "array key ['team']" search '->triumph\b' "property access ->triumph" search '\[\'triumph\'\]' "array key ['triumph']" search '->is_all_crew\b' "property access ->is_all_crew" search 'isAllCrew\b' "method or getter isAllCrew" search '\[\'is_all_crew\'\]' "array key ['is_all_crew']"
#!/usr/bin/env bash set -euo pipefail echo "Verifying references to old column names outside migrations…" # Patterns to search for old column names patterns=( '->team\b' "['\"]team['\"]" '->triumph\b' "['\"]triumph['\"]" '->is_all_crew\b' 'isAllCrew\b' "['\"]is_all_crew['\"]" ) for pat in "${patterns[@]}"; do echo -e "\n--- Searching for pattern: $pat ---" grep -R -n -P "$pat" . --exclude-dir=Api/migrations || echo "No matches for: $pat" done
Please verify removal of all references to renamed or removed columns
Ensure no lingering usages of the old column names (
team,triumph,is_all_crew) remain outside of migrations. You can run the following to double-check:grep -R -n -E "->team\b|\['team'\]|->triumph\b|\['triumph'\]|->is_all_crew\b|isAllCrew\b|\['is_all_crew'\]" . --exclude-dir=Api/migrationsMake sure that:
- All
->teamand['team']references are updated to use->key/['key'].- All
->triumphand['triumph']references are updated to use->quantity/['quantity'].- All checks or getters for
is_all_crew(e.g.isAllCrew) are removed or replaced.Api/src/Triumph/README.md (1)
1-65: Excellent documentation with comprehensive coverage of the Triumph system.The README provides clear explanations of the triumph system, configuration attributes, scopes, and implementation steps with a practical example.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...following relevant attributes: -name: triumph name (from [TriumphEnum](./En...(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...(NOUN_PLURAL_HAS)
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...eceive a triumph: -ALL_ACTIVE_HUMANS: All human players who are currently act...(UNLIKELY_OPENING_PUNCTUATION)
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
98bc8b6 to
a13c275
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Api/src/Triumph/config/services.yaml (1)
12-12: Add newline at end of file.
YAML files should end with a newline to satisfy linting tools.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/src/Triumph/README.md (5)
3-5: Grammar tweak: pluralize “Triumph”.
Change “Triumph are points awarded…” to “Triumphs are points awarded…” for grammatical correctness.
14-14: Clarify TODO for tag matching.
The note on “all tags must be present” needs follow-up. Consider creating an issue or linking to a ticket to implement partial tag matching in the future.Would you like me to draft an issue template or resolve this TODO now?
20-20: Fix subject–verb agreement.
Modify “the gains has 2x less chance” to “the gains have 2x less chance.”🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...(NOUN_PLURAL_HAS)
36-40: Normalize list indentation.
The steps under “How to add a new triumph?” should use consistent numbering and indentation. For example, indent sub-items by two spaces under step 2.
46-58: Add semicolon in PHP example.
The instantiation ofTriumphConfigDtoshould end with a semicolon to reflect valid PHP syntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Service/PlayerService.php(0 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/README.md(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Player/Event/PlayerCycleEventCest.php(2 hunks)Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Player/Service/PlayerServiceTest.php(0 hunks)Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)App/src/components/Game/BannerPanel.vue(1 hunks)
💤 Files with no reviewable changes (9)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Player/Service/PlayerService.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/tests/unit/Player/Service/PlayerServiceTest.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/src/Game/Enum/TriumphEnum.php
🚧 Files skipped from review as they are similar to previous changes (44)
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/config/services.yaml
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/config/api_platform/resources/triumph_config.yaml
- Api/config/api_platform/resources/game_config.yaml
- Api/config/packages/doctrine.yaml
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/composer.json
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/tests/functional/Player/Event/PlayerCycleEventCest.php
- App/src/components/Game/BannerPanel.vue
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Game/Entity/GameConfig.php
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/src/Daedalus/Entity/Daedalus.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/migrations/Version20250501184525.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/src/Game/ConfigData/GameConfigData.php
- Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php
- Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Triumph/Entity/TriumphConfig.php
🧰 Additional context used
🪛 LanguageTool
Api/src/Triumph/README.md
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...following relevant attributes: - name: triumph name (from [TriumphEnum](./En...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...
(NOUN_PLURAL_HAS)
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...eceive a triumph: - ALL_ACTIVE_HUMANS: All human players who are currently act...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 YAMLlint (1.37.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
Api/src/Triumph/config/services.yaml (2)
1-5: Defaults are correctly configured.
Enabling autowiring and autoconfiguration for the Triumph namespace will simplify service wiring.
6-12: Service resource and exclusions look good.
TheMush\Triumph\namespace is properly imported and critical directories/files (DependencyInjection, Entity, Kernel, Tests) are excluded as expected.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/src/Triumph/README.md (2)
1-2: Header is clear and concise.
The title properly introduces the Triumph feature.
60-63: Outcome summary is clear.
The bullet points correctly restate the example behavior.
…PlayerNewCycle event. It currently handles triumph for individual human and mush targets, and personal ones. Next step is listing all triumph given at cycle change, and cover different scopes, then let's do that for all events.
…ead players do not get triumph
…lated logic for player triumph calculation
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
214b1a5 to
edfc087
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Api/config/api_platform/validation/triumph_config.yaml (3)
3-5: Validatekeyis non-blank and a string
NotBlank+Type: stringensures clients supply a non-empty key.Consider adding a
Lengthconstraint (e.g. max 255) or a regex pattern to enforce key format consistency:key: - NotBlank: ~ - Type: string + - Length: + max: 255 + - Regex: + pattern: '/^[a-z0-9_.-]+$/' + message: 'Key may only contain lowercase letters, numbers, dots, underscores, and hyphens.'
15-17: Strengthen validation for tag arrays
NotBlank+Type: arrayensures the field exists, but does not validate element types. Recommend usingAllto enforce each tag is a non-blank string:targetedEventExpectedTags: - - NotBlank: ~ - - Type: array + - NotBlank: ~ + - Type: array + - All: + constraints: + - NotBlank: ~ + - Type: string
21-23: Consider numeric range constraints
NotBlank+Type: integerdisallows null/blank, but does not restrict the domain (e.g.quantity >= 1,regressiveFactor >= 0). You may want to add:quantity: - NotBlank: ~ - Type: integer + - Range: + min: 1 + minMessage: 'Quantity must be at least {{ limit }}.' regressiveFactor: - NotBlank: ~ - Type: integer + - Range: + min: 0 + minMessage: 'Regressive factor cannot be negative.'Also applies to: 27-29
🛑 Comments failed to post (1)
Api/config/api_platform/validation/triumph_config.yaml (1)
7-8:
⚠️ Potential issueEnum fields require a proper enum-value constraint
UsingType: Mush\Triumph\Enum\TriumphEnum|TriumphScope|TriumphVisibilityonly checks object type at runtime and will fail to validate raw string input. You should enforce allowed values with Symfony’sEnumorChoiceconstraint:- name: - - NotBlank: ~ - - Type: Mush\Triumph\Enum\TriumphEnum + name: + - NotBlank: ~ + - Enum: + class: Mush\Triumph\Enum\TriumphEnum + allowNull: falseRepeat similarly for
scopeandvisibility.Also applies to: 9-11, 24-26
🤖 Prompt for AI Agents
In Api/config/api_platform/validation/triumph_config.yaml around lines 7 to 8 and also lines 9-11 and 24-26, the current use of Type constraint with enum classes only checks the object type at runtime and does not validate raw string inputs properly. Replace the Type constraint with Symfony's Enum or Choice constraint to enforce allowed enum values for the fields, ensuring validation works correctly for string inputs. Apply this change consistently to the enum fields including scope and visibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Api/src/Triumph/config/services.yaml (1)
12-12: Add missing newline at end-of-file
YAML parsers and linters expect a trailing newline. Please append one to satisfynew-line-at-end-of-file.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/src/Triumph/README.md (3)
7-7: Normalize heading capitalization
Change## Triumph config Attributesto## Triumph Config Attributesfor consistency with other headings.
20-20: Fix subject–verb agreement in description
The phrase “gains has” should read “gains have” to match the plural noun.Proposed diff:
- - `regressiveFactor`: Determines after how many gains the gains has 2x less chance to be earned (TODO) + - `regressiveFactor`: Determines after how many gains the gains have 2x less chance to be earned (TODO)🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...(NOUN_PLURAL_HAS)
26-30: Make bullet punctuation consistent
Add a period at the end of each scope description for uniform style.Proposed diff:
- - `ALL_ACTIVE_HUMANS`: All human players who are currently active in the game + - `ALL_ACTIVE_HUMANS`: All human players who are currently active in the game. - - `ALL_ALIVE_MUSHS`: All Mush players who are still alive + - `ALL_ALIVE_MUSHS`: All Mush players who are still alive. - - `ALL_ALIVE_HUMANS`: All human players who are still alive + - `ALL_ALIVE_HUMANS`: All human players who are still alive. - - `ALL_MUSHS`: All Mush players + - `ALL_MUSHS`: All Mush players. - - `PERSONAL`: Only applies to a specific character (defined in the `target` attribute) + - `PERSONAL`: Only applies to a specific character (defined in the `target` attribute).🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...eceive a triumph: -ALL_ACTIVE_HUMANS: All human players who are currently act...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
Api/composer.json(1 hunks)Api/config/api_platform/resources/game_config.yaml(1 hunks)Api/config/api_platform/resources/triumph_config.yaml(1 hunks)Api/config/api_platform/serialization/triumph_config.yaml(1 hunks)Api/config/api_platform/validation/triumph_config.yaml(1 hunks)Api/config/packages/doctrine.yaml(1 hunks)Api/config/routes/annotations.yaml(0 hunks)Api/config/services.yaml(1 hunks)Api/migrations/Version20250501184525.php(1 hunks)Api/src/Daedalus/Entity/Daedalus.php(1 hunks)Api/src/Daedalus/Event/DaedalusCycleEvent.php(2 hunks)Api/src/Daedalus/Factory/DaedalusFactory.php(2 hunks)Api/src/Game/ConfigData/GameConfigData.php(1 hunks)Api/src/Game/ConfigData/GameConfigDataLoader.php(3 hunks)Api/src/Game/ConfigData/TriumphConfigData.php(0 hunks)Api/src/Game/ConfigData/TriumphConfigDataLoader.php(0 hunks)Api/src/Game/DataFixtures/TriumphConfigFixtures.php(0 hunks)Api/src/Game/Entity/GameConfig.php(1 hunks)Api/src/Game/Entity/TriumphConfig.php(0 hunks)Api/src/Game/Enum/EventPriorityEnum.php(1 hunks)Api/src/Game/Enum/TriumphEnum.php(0 hunks)Api/src/Game/Repository/TriumphConfigRepository.php(0 hunks)Api/src/Game/Service/ConfigDataLoaderService.php(1 hunks)Api/src/Player/Entity/Collection/PlayerCollection.php(2 hunks)Api/src/Player/Entity/PlayerVariables.php(1 hunks)Api/src/Player/Service/PlayerService.php(0 hunks)Api/src/RoomLog/Listener/TriumphEventSubscriber.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigData.php(1 hunks)Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php(1 hunks)Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php(1 hunks)Api/src/Triumph/Dto/TriumphConfigDto.php(1 hunks)Api/src/Triumph/Entity/TriumphConfig.php(1 hunks)Api/src/Triumph/Entity/TriumphConfigCollection.php(1 hunks)Api/src/Triumph/Enum/TriumphEnum.php(1 hunks)Api/src/Triumph/Enum/TriumphScope.php(1 hunks)Api/src/Triumph/Enum/TriumphTeam.php(1 hunks)Api/src/Triumph/Enum/TriumphVisibility.php(1 hunks)Api/src/Triumph/Event/TriumphChangedEvent.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventInterface.php(1 hunks)Api/src/Triumph/Event/TriumphSourceEventTrait.php(1 hunks)Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php(1 hunks)Api/src/Triumph/README.md(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepository.php(1 hunks)Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php(1 hunks)Api/src/Triumph/Service/ChangeTriumphFromEventService.php(1 hunks)Api/src/Triumph/config/services.yaml(1 hunks)Api/tests/functional/Player/Event/PlayerCycleEventCest.php(2 hunks)Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php(1 hunks)Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php(1 hunks)Api/tests/unit/Player/Service/PlayerServiceTest.php(0 hunks)Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php(1 hunks)Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php(1 hunks)Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php(1 hunks)Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php(1 hunks)Api/translations/fr/triumph+intl-icu.fr.xlf(1 hunks)App/src/components/Game/BannerPanel.vue(1 hunks)
💤 Files with no reviewable changes (9)
- Api/config/routes/annotations.yaml
- Api/src/Game/Repository/TriumphConfigRepository.php
- Api/src/Player/Service/PlayerService.php
- Api/src/Game/ConfigData/TriumphConfigData.php
- Api/src/Game/Entity/TriumphConfig.php
- Api/tests/unit/Player/Service/PlayerServiceTest.php
- Api/src/Game/DataFixtures/TriumphConfigFixtures.php
- Api/src/Game/ConfigData/TriumphConfigDataLoader.php
- Api/src/Game/Enum/TriumphEnum.php
✅ Files skipped from review due to trivial changes (2)
- Api/src/Triumph/Enum/TriumphVisibility.php
- Api/src/Triumph/Repository/TriumphConfigRepositoryInterface.php
🚧 Files skipped from review as they are similar to previous changes (43)
- Api/src/Game/Service/ConfigDataLoaderService.php
- Api/composer.json
- Api/config/services.yaml
- Api/src/Player/Entity/PlayerVariables.php
- Api/config/api_platform/resources/game_config.yaml
- App/src/components/Game/BannerPanel.vue
- Api/config/api_platform/resources/triumph_config.yaml
- Api/src/Game/Enum/EventPriorityEnum.php
- Api/src/Game/ConfigData/GameConfigDataLoader.php
- Api/src/Game/Entity/GameConfig.php
- Api/src/Triumph/Enum/TriumphTeam.php
- Api/config/packages/doctrine.yaml
- Api/translations/fr/triumph+intl-icu.fr.xlf
- Api/src/Daedalus/Factory/DaedalusFactory.php
- Api/src/Triumph/Repository/TriumphConfigRepository.php
- Api/src/Triumph/ConfigData/TriumphConfigDataLoader.php
- Api/tests/functional/Player/Event/PlayerCycleEventCest.php
- Api/tests/unit/Triumph/TestDoubles/Repository/InMemoryTriumphConfigRepository.php
- Api/src/Triumph/Event/TriumphSourceEventTrait.php
- Api/src/Player/Entity/Collection/PlayerCollection.php
- Api/src/Triumph/Enum/TriumphScope.php
- Api/src/Daedalus/Event/DaedalusCycleEvent.php
- Api/src/Triumph/Event/TriumphSourceEventInterface.php
- Api/tests/unit/Triumph/Service/ChangeTriumphFromEventServiceTest.php
- Api/src/Triumph/Service/ChangeTriumphFromEventService.php
- Api/src/Game/ConfigData/GameConfigData.php
- Api/src/RoomLog/Listener/TriumphEventSubscriber.php
- Api/src/Triumph/Event/TriumphChangedEvent.php
- Api/config/api_platform/serialization/triumph_config.yaml
- Api/tests/unit/Triumph/Event/DaedalusFullEventTest.php
- Api/src/Triumph/Dto/TriumphConfigDto.php
- Api/tests/unit/Triumph/Event/FinishDaedalusEventTest.php
- Api/src/Triumph/Listener/TriumphSourceEventSubscriber.php
- Api/migrations/Version20250501184525.php
- Api/src/Triumph/Enum/TriumphEnum.php
- Api/src/Daedalus/Entity/Daedalus.php
- Api/tests/functional/Triumph/Event/TriumphSourceEventCest.php
- Api/tests/functional/Triumph/Service/ChangeTriumphFromEventServiceCest.php
- Api/src/Triumph/ConfigData/TriumphConfigData.php
- Api/src/Triumph/Entity/TriumphConfig.php
- Api/config/api_platform/validation/triumph_config.yaml
- Api/src/Triumph/Entity/TriumphConfigCollection.php
- Api/src/Triumph/DataFixtures/TriumphConfigFixtures.php
🧰 Additional context used
🪛 LanguageTool
Api/src/Triumph/README.md
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...following relevant attributes: - name: triumph name (from [TriumphEnum](./En...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~20-~20: It looks like you are using the wrong form of the noun or the verb. Did you mean “gain has” or “gains have”?
Context: ...r`: Determines after how many gains the gains has 2x less chance to be earned (TODO) ## ...
(NOUN_PLURAL_HAS)
[uncategorized] ~26-~26: Loose punctuation mark.
Context: ...eceive a triumph: - ALL_ACTIVE_HUMANS: All human players who are currently act...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 YAMLlint (1.37.1)
Api/src/Triumph/config/services.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
Api/src/Triumph/config/services.yaml (2)
1-5: Defaults for autowiring/autoconfiguration look good
Settingautowire: trueandautoconfigure: trueunder_defaultscorrectly ensures that all Triumph services are automatically wired and configured.
6-12: Service resource and exclusions are correctly defined
TheMush\Triumph\:resource pointing to../combined with explicit exclusions forDependencyInjection/,Entity/,Kernel.php, andTests/will register only the intended service classes.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
Api/src/Triumph/README.md (1)
46-58: Concrete example looks clear and accurate
The PHP snippet demonstrates how to configure a personal triumph correctly and aligns with the new DTO-based approach.
cmnemoi
force-pushed
the
feat/triumph-service
branch
from
edfc087 to
da08c40
Compare
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests
Chores