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

feat: Control plane #6

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mateo-gomez
Copy link
Contributor

Creo el pullrequest por motivos didácticos por si alguien está interesado en ver mi solución al reto de crear el control plane, por eso lo pongo todo en el mismo pullrequest.

La verdad apenas estoy aprendiendo de la arquitectura hexagonal y puede que tenga errores, sin embargo estoy abierto a correcciones.

new changes allows to create and get permissions
Modify the Auth interface contract to update the signature of the getPermissions method. Now it accepts only the userId parameter instead of (email, password) for more secure and efficient authentication handling.

BREAKING CHANGE: The getPermissions method signature has been updated to accept only the userId parameter instead of (email, password).
Copy link
Contributor

@Alan-TheGentleman Alan-TheGentleman left a comment

Choose a reason for hiding this comment

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

Me gustan las ideas que has querido implementar !

Solamente algunos comentarios para tener en cuenta. Y en cuanto a la implementación del userId, realmente el email ya debería de ser diferente ya que no debería de repetirse en la base de datos

import { Permissions } from "../../app/schemas/auth";
import { ForRepoQuerying } from "../../ports/drivens";

export class RepoQuerierStubAdapter implements ForRepoQuerying {
Copy link
Contributor

Choose a reason for hiding this comment

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

Si es un stub no debería tener utilización real de los proxy u otros adapters. Debería de retornar un mock

Copy link
Contributor

Choose a reason for hiding this comment

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

Creo que te has equivocado con el nombre del archivo y por eso sigue diciendo que es un stub, porque es una copia del RepoQuerierStubAdapter pero ha faltado modificar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corregido!

) {}

async getAuthDetails(email: string, password: string): Promise<AuthDetails> {
const user = await this.repoQuerier.getUser(email);
Copy link
Contributor

Choose a reason for hiding this comment

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

realmente esta comprobacion de si existe el user o no, se podría hacer en el repoQuerier y esto englobarlo con un try catch, cosa que si entra por el catch envie el error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tenés razón, ya lo corregí para que haga el catch del error.También vi que el logger era innecesario porque ya estaba en el repository asi que lo quité

@@ -37,4 +45,43 @@ export class Repository implements ForManagingUser {
email: newUser.email,
};
}

async getInternalUser(email: string): Promise<RepoUser> {
const user = this.userList.find((user) => user.email === email);
Copy link
Contributor

Choose a reason for hiding this comment

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

Agregar separación entre unidades lógicas

Suggested change
const user = this.userList.find((user) => user.email === email);
const user = this.userList.find((user) => user.email === email);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no sabía muy como interpretar lo de la separación entre unidades lógicas, asumí que te referías a que en el repository estaba combinando consultas de permissions y user, puede que sea por ahí?

Copy link
Contributor

Choose a reason for hiding this comment

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

Es solo el agregado de un espacio entre unidades. Aquí tienes un video como guía:

👨‍💻💻 Programando con estilo: cómo escribir código limpio y elegante como un verdadero 😎crack💪
https://youtu.be/041fWH35X-4

permissions: Permissions
): Promise<UserPermission> {
const newUserPermission: UserPermission = {
id: String(this.userList.length + 1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ojo con estos agregados utilizando la longitud, porque si eliminas un usuario diferente al ultimo y vuelves a agregar otro se repetiria el ultimo usuario en cuanto al id

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