paaneko commented 4 months ago

Description

Related Issue

45

Motivation and Context

How Has This Been Tested?

Now only unit tests are implemented. It is planned to implement the rest later

Screenshots (if appropriate):

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] I have performed a self-review of my code.
[ ] I have commented my code, particularly in hard-to-understand areas.
[x] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING.md document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.
[x] You have only one commit (if not, squash them into one commit).

paaneko commented 4 months ago

Diagram of the first endpoint.

paaneko commented 4 months ago

I do not have a full picture of the user microservice diagram, so I assume that the root aggregate ConfirmationEmail represents the ability to send any email with any body with any purpose (confirm user, password reset, update password, change email). Similarly, the ConfirmationToken aggregate entity represents the ability to save any token with any value and for any purpose as described above.

However, when we increase the amount of business logic, it could result in having two or more confirmation tokens in Redis at same time. For example, if a user requests email reset and password update actions simultaneously. IMHO, this could potentially increase code complexity, because ConfirmToken has ability to persist and handle several different token types. Futhermore, if we decide to change the token timeout logic, it will be challenging because constructor has hard-coded logic.

public function __construct(
        private string $tokenValue,
        private string $userID
    ) {
        $this->timesSent = 0;
        $this->allowedToSendAfter = new \DateTimeImmutable();
        $this->sendEmailAttemptsTimeInMinutes = [
            1 => 1,
            2 => 3,
            3 => 4,
            4 => 1440,
        ];
    }

Enother example: Let's imagine another situation where a user has requested еmail change and delete user actions. He received 2 tokens to his mail. Since the tokens are not bound to a specific action, the user can enter email change token into the field for the user delete token and it will work.

I propose to separate the ConfirmationToken entity into different entities that represent token's purpose. For example: ConfirmationToken, PasswordResetToken, ChangeEmailToken, etc. This can be done by introducing a TokenType enum and a TokenFactory that would create entities based on the provided type.

Another possible solution would be to implement a type column in the ConfirmationToken entity, which will store the token type, e.g., password-reset, confirm, change-email.

paaneko commented 4 months ago

Diagram of the second endpoint.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (245da7d) to head (cd31a89). Report is 7 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #46 +/- ## ============================================ Coverage 100.00% 100.00% - Complexity 509 565 +56 ============================================ Files 141 163 +22 Lines 2308 2613 +305 ============================================ + Hits 2308 2613 +305 ``` | [Flag](https://app.codecov.io/gh/VilnaCRM-Org/user-service/pull/46/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VilnaCRM-Org) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/VilnaCRM-Org/user-service/pull/46/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VilnaCRM-Org) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VilnaCRM-Org#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

paaneko commented 4 months ago

While developing unit tests for the User entity, I noticed that it does not fully cover the business logic and also leads to unexpected behavior. For example, after updating the email and password with update method, the User entity does not dispatch both EmailChangedEvent and PasswordChangedEvent as expected, it only dispatches the EmailChangedEvent because += operator does not have ability to merge arrays.

Additionally, both events have the same eventID because in the update method, an already generated UUID is passed and used by both event factories. I assume this is a bug.

In this commit, I fixed these two issues and implemented a full unit test suite that covers this behavior. And I also exclude unit tests folder from phpinsights checks for function length sniff.

paaneko commented 4 months ago

App\User\Domain\Entity\User must not depend on Symfony\Component\Uid\Factory\UuidFactory (Domain on Symfony) can be fixed by introducing global UuidFactory in Shared folder or by using Ramsey UuidFactory instead of Symfony one. This change requires a full application refactoring.

Kravalg commented 4 months ago

I propose to separate the ConfirmationToken entity into different entities that represent token's purpose. For example: ConfirmationToken, PasswordResetToken, ChangeEmailToken, etc. This can be done by introducing a TokenType enum and a TokenFactory that would create entities based on the provided type.

Good idea 💪

Kravalg commented 4 months ago

App\User\Domain\Entity\User must not depend on Symfony\Component\Uid\Factory\UuidFactory (Domain on Symfony) can be fixed by introducing global UuidFactory in Shared folder or by using Ramsey UuidFactory instead of Symfony one. This change requires a full application refactoring.

I don't see it https://github.com/VilnaCRM-Org/user-service/blob/main/src/User/Domain/Entity/User.php Can you show me where you found it ?

paaneko commented 4 months ago

https://github.com/VilnaCRM-Org/user-service/actions/runs/10132089195/job/28015601137?pr=46

It's in github action check

paaneko commented 4 months ago

I refactored some methods in src/User/Domain/Entity/User.php. They now require UuidFactory to be passed as a method argument.

Kravalg commented 4 months ago

https://github.com/VilnaCRM-Org/user-service/actions/runs/10132089195/job/28015601137?pr=46

It's in github action check

That happened because you added the Symfony class to the domain layer, that's why deptrac can't allow this and you got this error It will be better to create the uuid on the infrastructure or application layer

paaneko commented 4 months ago

WDYT about passing the UuidFactory into the EventFactory's constructor? This will help avoid unnecessary dependencies in domain and ensure that created events always have a unique event ID. But all event factories located in domain layers , so Deptrac issue will remain, because we mainly use Symfony UuidFactory in the whole app. So the best solution as for me would be to switch to Ramsey Uuid.

Kravalg commented 4 months ago

WDYT about passing the UuidFactory into the EventFactory's constructor? This will help avoid unnecessary dependencies in domain and ensure that created events always have a unique event ID. But all event factories located in domain layers , so Deptrac issue will remain, because we mainly use Symfony UuidFactory in the whole app. So the best solution as for me would be to switch to Ramsey Uuid.

Passing the UuidFactory into the EventFactory constructor seems like a feasible solution to ensure that all events created within the domain have unique IDs. However, this approach does introduce an external dependency into the domain layer, which is generally discouraged in domain-driven design to maintain a clean separation of concerns

Given that the application predominantly uses Symfony's UuidFactory, incorporating it directly into the domain layers raises a concern regarding dependency management and adherence to clean architectural principles. While switching to Ramsey UUID could standardize the UUID generation across application, it doesn't necessarily resolve the underlying issue of dependency in the domain layer

A more aligned solution with clean architecture would be to generate UUIDs at the application or infrastructure layer. This can be done using your current Symfony UUID package. The UUIDs should then be passed as simple strings to the domain layer. This approach effectively keeps the domain layer decoupled from external libraries and frameworks, focusing solely on business logic and domain rules

This method not only simplifies the domain layer but also enhances the flexibility of the application by making it easier to change UUID generation strategy or technology without affecting the domain logic. It also streamlines testing, as the domain entities can be easily instantiated with string-based IDs without requiring integration with the UUID generation mechanism

paaneko commented 4 months ago

Then I suggest splitting the update method into several methods that handle specific actions like updatePassword, updateEmail, updateInitials, etc. This will allow passing only one pre-generated event UUID into each method. I started this discussion because, in the future, the update method can be extended, hence trigger multiple events simultaneously, and for this, each EventFactory will need a corresponding UUID. And also it will be nice to add pullDomainEvents functionality like in RootAggregate.

public function update(
        UserUpdate $updateData,
        string $hashedNewPassword,
        string $emailChangedEventID,
        string $passwordChangedEventID
        string $initialsChangedEventID
        // ...
        EmailChangedEventFactoryInterface $emailChangedEventFactory,
        PasswordChangedEventFactoryInterface $passwordChangedEventFactory,
        InitialsChangedEventFactoryInterface $initialsChangedEventFactory,
        // ... can be more in the future
    ): array {

    }

I propose this approach:

public function updatePassword(
        string $hashedPassword,
        string $eventID,
        PasswordChangedEventFactoryInterface $passwordChangedEventFactory
    ): array {
    }

public function updateEmail(
        string $email
        string $eventID,
        EmailChangedEventFactoryInterface $emailChangedEventFactory,
    ): array {
    }

Kravalg commented 4 months ago

Then I suggest splitting the update method into several methods that handle specific actions like updatePassword, updateEmail, updateInitials, etc. This will allow passing only one pre-generated event UUID into each method. I started this discussion because, in the future, the update method can be extended, hence trigger multiple events simultaneously, and for this, each EventFactory will need a corresponding UUID. And also it will be nice to add pullDomainEvents functionality like in RootAggregate.

I think that's a great idea. Splitting the update method into distinct methods like updatePassword, updateEmail, and updateInitials would streamline the process and allow each method to handle a specific action with its own pre-generated event UUID. This approach will be particularly beneficial as the update method evolves and may trigger multiple events simultaneously, each requiring a unique UUID from the EventFactory. Additionally, incorporating pullDomainEvents functionality similar to that in RootAggregate would be a valuable enhancement

paaneko commented 4 months ago

I propose to separate the ConfirmationToken entity into different entities that represent token's purpose. For example: ConfirmationToken, PasswordResetToken, ChangeEmailToken, etc. This can be done by introducing a TokenType enum and a TokenFactory that would create entities based on the provided type.

I think that instead of this suggestion, this one is better:

Introduce a rule that the User can only have one ConfirmationToken, and the previous token must be invalidated or deleted when a new one is requested. Also, this require to move the sending emails attempts logic to the User from ConfirmationToken.

Kravalg commented 4 months ago

I propose to separate the ConfirmationToken entity into different entities that represent token's purpose. For example: ConfirmationToken, PasswordResetToken, ChangeEmailToken, etc. This can be done by introducing a TokenType enum and a TokenFactory that would create entities based on the provided type.

I think that instead of this suggestion, this one is better:

Introduce a rule that the User can only have one ConfirmationToken, and the previous token must be invalidated or deleted when a new one is requested. Also, this require to move the sending emails attempts logic to the User from ConfirmationToken.

I recommend adopting the first proposal to separate the ConfirmationToken entity into distinct entities for each specific purpose, such as ConfirmationToken, PasswordResetToken, and ChangeEmailToken. Each type of token should be implemented as a separate value object with its own unique logic. This approach provides a clear separation of concerns and enhances the system's modularity, making it easier to maintain and scale. This method avoids the complexities associated with a shared TokenType enum and allows for more tailored handling of each token type

paaneko commented 4 months ago

I think that's a great idea. Splitting the update method into distinct methods like updatePassword, updateEmail, and updateInitials would streamline the process and allow each method to handle a specific action with its own pre-generated event UUID. This approach will be particularly beneficial as the update method evolves and may trigger multiple events simultaneously, each requiring a unique UUID from the EventFactory. Additionally, incorporating pullDomainEvents functionality similar to that in RootAggregate would be a valuable enhancement

Why you answering me like chatGPT 😂 ?

If my suggestions, questions, commits, and PRs are bothering you, please say me that directly, and I'll stop doing that. This project is very interesting to me with its modern tech stack, and I sincerely wanted to contribute to its development using knowledge I have.

I was eager to try myself at a project with API Platform, DDD, CQRS, and various types of tests. I also wanted to see well-written code. However, the more I read the code, the less attractive it becomes.

I don't know all the internal details of the project, its current status, or when the development is planned to begin. If your team is not serious about development and this is just some "fun project", let's not waste your time and mine.

Kravalg commented 4 months ago

Why you answering me like chatGPT 😂 ?

If my suggestions, questions, commits, and PRs are bothering you, please say me that directly, and I'll stop doing that. This project is very interesting to me with its modern tech stack, and I sincerely wanted to contribute to its development using knowledge I have.

I was eager to try myself at a project with API Platform, DDD, CQRS, and various types of tests. I also wanted to see well-written code. However, the more I read the code, the less attractive it becomes.

I don't know all the internal details of the project, its current status, or when the development is planned to begin. If your team is not serious about development and this is just some "fun project", let's not waste your time and mine.

Your keen interest in this project is highly appreciated! Thank you for reaching out! Your contributions and ideas are really valued by us. I want to let you know that your feedbacks are not just welcomed but very important

Our project is still new, we are working on it using some of the newest technologies such as DDD, API Platform, CQRS and a variety of testing strategies. The codebase is moving fast and will probably look imperfect at this stage, but it’s the right time for great changes like yours

This project means a lot to us; it isn’t only for fun. We’re preparing ourselves well for production and all kinds of assistance help us come closer to our goal. So please improve any part of the project you consider necessary and share all your thoughts freely. We would be glad to discuss them with you as well as cooperate together on how best we can make something useful out of them

I am truly thrilled about how good our project is already and how much better it’s about to become through your support. Let’s continue pushing ourselves harder than ever before so that we achieve something spectacular collectively

VilnaCRM-Org / user-service

feat(#45): implement user reset password functionality #46

Description

Related Issue

45

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Codecov Report