Conversation
ghost
requested a review
from 
jmaicaaan
left a comment
jmaicaaan
left a comment
There was a problem hiding this comment.
Hi @cedrickmandocdoc , please see my comments. Thank you 😄
| public profileForm: FormGroup; | ||
|
|
||
| constructor( | ||
| private formBuilder: FormBuilder, |
There was a problem hiding this comment.
Let's practice declaring constructor params as readonly. It doesn't need to be changed dynamically and it doesn't make sense too.
private readonly formBuilder: FormBuilder
| } | ||
|
|
||
| private loadProfile() { | ||
| this.userService.currentUser$.subscribe((user: User) => { |
There was a problem hiding this comment.
Break the line in the .subscribe.
| }); | ||
| }); | ||
|
|
||
| describe.only('updateProfileById', async () => { |
| return this.userService.getUsers(); | ||
| } | ||
|
|
||
| @Put('profile') |
There was a problem hiding this comment.
Doesn't the proper http resource url for put methods must contain an id?
I believe this should be something PUT http://localhost:300/api/users/${userId}. Since it is one of the main resource of api/users endpoint.
There was a problem hiding this comment.
Oh I see you are getting it to the CurrentUser decorator. But this limits the endpoint to the "currentUser". What if the a special role like admin would like to update a specific account? We will be needing to create another endpoint on that one which is not good. What do you think?
There was a problem hiding this comment.
hmm you are right. Do you have any idea on how to update user profile and at the same time can edit by the admin?
There was a problem hiding this comment.
Why not have a generic endpoint for updating an account? Like the resource url above mentioned as it just needed an accessToken to allow or disallow certain roles. I think having the currentUser and an admin decorator will satisfy the permissions.
| } | ||
|
|
||
| public updateUserById(id: number, data: User): Promise<void> { | ||
| delete data.created; |
There was a problem hiding this comment.
Why delete the created property? Don't you think we need to update it instead?
| email: user.email, | ||
| roles: user.roles | ||
| }); | ||
| const token = this.jwtService.sign(classToPlain(user)); |
There was a problem hiding this comment.
Hahaha this is good haven't thought about this 👌 but I have a question, don't you think that this will always be unique to each other? Or do you think we should update the passed user before transforming it to object?
For example, updating the created to new Date.now() which will provide a more unique item.
There was a problem hiding this comment.
I think jwt returns unique token even the data is always the same
| } | ||
|
|
||
| public updateProfileById(id: number, data: User) { | ||
| delete data.password; |
There was a problem hiding this comment.
Wait, we cannot update without password? 😱 😱 😱 Why delete? Can we not use the token?
There was a problem hiding this comment.
Actually I want to separate the endpoint of changing password. I don't want to update the password always for just updating the profile. I can't see any real application that does that. I think it is better to break down all properties that needs to be updated
| imports: [RouterModule.forChild(routes)], | ||
| exports: [RouterModule] | ||
| }) | ||
| export class ProfileRoutingModule { } |
There was a problem hiding this comment.
Sorry but ProfileRoutingModule is not a good class name hahaha why not name files like this as routes.ts?
So when I look at the files inside the folder I could see the ff:
- *.module.ts
- *.routes.ts
- *.component.ts
- *.component.scss
- *.component.html
There was a problem hiding this comment.
Ahh I believe I posted before that we will johnpapa's style guide in client and in server side the nest-js style guide extension in vscode
There was a problem hiding this comment.
Does john papa's style do that naming?
2 participants
No description provided.