Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Dojo: Replicate Action Refactor #304

Open
wants to merge 23 commits into
base: development
Choose a base branch
from

Conversation

nshandra
Copy link
Contributor

@nshandra nshandra commented Nov 19, 2024

📌 References

📝 Implementation

Refactor replicate functions:

  • Replicate user from template (A final form version is available and will probably be the one to integrate into UEApp)
  • Replicate user from table (Updated the new ImportTable for this function)

@nshandra nshandra requested a review from xurxodev November 19, 2024 20:18
@nshandra nshandra self-assigned this Nov 19, 2024
@nshandra nshandra marked this pull request as ready for review November 20, 2024 12:29
@nshandra
Copy link
Contributor Author

nshandra commented Nov 28, 2024

@xurxodev putting on draft to remove the demo part from the PR

@nshandra nshandra marked this pull request as draft November 28, 2024 08:26
@nshandra nshandra marked this pull request as ready for review December 17, 2024 07:14
Copy link

@xurxodev xurxodev left a comment

Choose a reason for hiding this comment

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

thanks @nshandra

I've found issues to review:

  • Non related specially to this PR but UserLogic is really the entity and User is the props. I would expect the names as UserLogic -> User and User -> UserProps
  • In the new code affected by this PR that to use UserRepository, we should change to return the entity from the repository and not the props.

I've added rest of the issues in comments in the code.

@@ -152,6 +152,22 @@ export class UserD2ApiRepository implements UserRepository {
).map(({ objects }) => objects.map(user => user.id));
}

public listAllUsernames(options: ListOptions): FutureData<string[]> {

Choose a reason for hiding this comment

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

A repository should never return primitive types, this is a code smell that we saw in the coding dojo

In Clean Architecture, a repository should always return entities.

The presentation layer should map user list to get userNames if required.

In this case the entity is UserLogic (can we change to User and User to UserProps?)

@@ -0,0 +1,11 @@
import { UseCase } from "../../CompositionRoot";

Choose a reason for hiding this comment

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

A use case should never return primitive types, this is a code smell that we saw in the coding dojo

In Clean Architecture, a use case should always return entities.

The presentation layer should map user list to get userNames if required.

In this case the entity is UserLogic (can we change to User and User to UserProps?)

@@ -597,14 +630,23 @@ const useValidations = (
return {
validation: (value: string) => {
if (!value) return i18n.t("Please provide a username");
if (/^[._@-]|[._@-]$/.test(value)) {

Choose a reason for hiding this comment

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

These new validations are business rules validations and should be realized in the user domain entity

}

export const ReplicateUserFromTable: React.FC<ReplicateUserFromTableProps> = props => {
const { compositionRoot } = useAppContext();

Choose a reason for hiding this comment

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

The component should only render. (render logic)

The rest of the presentation logic (manage state, actions, invoke use cases, etc.) should be in a custom hook: useReplicateUserFromTable.

In this custom hook, we should use the user entity, not the user props

onRequestClose: () => void;
}

export const ReplicateUserFromTemplate: React.FC<ReplicateUserFromTemplateProps> = props => {

Choose a reason for hiding this comment

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

The rest of the presentation logic (manage state, actions, invoke use cases, etc.) should be in a custom hook: useReplicateUserFromTemplate.

In this custom hook, we should use the user entity, not the user props

</>
);
};

Choose a reason for hiding this comment

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

These validations are business rules validations and should be realized in the user domain entity.

To validate you can to try to create a domain entity, call internally to validate a return, for example an Either with the entity or the errors.

You can see these examples:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants