Skip to content
This repository has been archived by the owner on Jun 21, 2019. It is now read-only.

Feature/#31 cms profile page #43

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/client/app/cms/cms-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ const routes: Routes = [
path: '',
component: CmsComponent,
canActivate: [CurrentUserGuard],
children: []
children: [
{
path: 'profile',
loadChildren: '../profile/profile.module#ProfileModule'
}
]
}
];

Expand Down
1 change: 1 addition & 0 deletions src/client/app/cms/cms-sidenav/cms-sidenav.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
</section>
<mat-list role="list">
<a mat-list-item routerLink="/cms" routerLinkActive="active" [routerLinkActiveOptions]="{exact: true}">Home</a>
<a mat-list-item routerLink="/cms/profile" routerLinkActive="active">Profile</a>
<a mat-list-item (click)="onClickLogout()">Logout</a>
</mat-list>
8 changes: 8 additions & 0 deletions src/client/app/core/services/user.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,12 @@ export class UserService {
this.apiService.setAccessToken(null);
});
}

public updateProfile(data: User): Observable<User> {
return this.apiService.put(`/users/profile`, data)
.do((user: User) => {
console.log('updated current user', user);
this.currentUserSource.next(user);
});
}
}
16 changes: 16 additions & 0 deletions src/client/app/profile/profile-routing.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { NgModule } from '@angular/core';
import { RouterModule, Routes } from '@angular/router';

import { ProfileComponent } from './profile.component';

const routes: Routes = [
{ path: '', component: ProfileComponent }
];

@NgModule({
imports: [RouterModule.forChild(routes)],
exports: [RouterModule]
})
export class ProfileRoutingModule { }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Does john papa's style do that naming?


export const routedComponents = [ProfileComponent];
7 changes: 7 additions & 0 deletions src/client/app/profile/profile.component.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<h3>Profile</h3>
<form [formGroup]="profileForm">
<mat-form-field>
<input matInput type="text" placeholder="Email" formControlName="email">
</mat-form-field>
<button mat-raised-button type="button" (click)="onClickUpdate()">Update</button>
</form>
Empty file.
65 changes: 65 additions & 0 deletions src/client/app/profile/profile.component.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { Component, OnInit } from '@angular/core';
import { FormBuilder, FormControl, FormGroup, Validators } from '@angular/forms';

import { Role, User } from '../../../common/entities';

import { UserService } from '../core';

@Component({
selector: 'profile',
templateUrl: 'profile.component.html',
styleUrls: ['profile.component.scss']
})

export class ProfileComponent implements OnInit {

public user: User;
public profileForm: FormGroup;

constructor(
private formBuilder: FormBuilder,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Alright

private userService: UserService
) { }

// interface methods
public ngOnInit() {
this.loadProfile();
this.buildProfileForm();
this.patchProfileFormValue(this.user);
}

// event methods
public onClickUpdate() {
const data = this.profileForm.getRawValue();
this.updateProfile(data);
}

private loadProfile() {
this.userService.currentUser$.subscribe((user: User) => {
Copy link
Member

Choose a reason for hiding this comment

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

Break the line in the .subscribe.

this.user = user;
});
}

private buildProfileForm() {
this.profileForm = this.formBuilder.group({
email: ['', Validators.required]
});
}

private patchProfileFormValue(user: User) {
this.profileForm.patchValue({
email: user.email
});
}

private updateProfile(data: User) {
this.userService
.updateProfile(data)
.subscribe((user: User) => {
this.user = user;
this.patchProfileFormValue(this.user);
}, (error: Error) => {
console.log(error);
});
}
}
18 changes: 18 additions & 0 deletions src/client/app/profile/profile.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { NgModule } from '@angular/core';

import { SharedModule } from '../shared';

import { ProfileRoutingModule, routedComponents } from './profile-routing.module';

@NgModule({
imports: [
SharedModule,
ProfileRoutingModule
],
exports: [],
declarations: [
routedComponents
],
providers: []
})
export class ProfileModule { }
5 changes: 5 additions & 0 deletions src/client/app/shared/shared.module.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { FormsModule, ReactiveFormsModule } from '@angular/forms';

import { MaterialModule } from '../material';

Expand All @@ -12,10 +13,14 @@ import { ListComponent } from './list/list.component';
@NgModule({
imports: [
CommonModule,
FormsModule,
ReactiveFormsModule,
MaterialModule
],
exports: [
CommonModule,
FormsModule,
ReactiveFormsModule,
MaterialModule,
ListComponent
],
Expand Down
16 changes: 16 additions & 0 deletions src/server/app/users/user.controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,20 @@ describe('UserController', () => {
.expect(200);
});
});

describe.only('updateProfileById', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove .only here

it('should update current user, /api/users/profile', async () => {
const data = await userService.login({
email: `admin@test.com`,
password: `test`
});
const response = await server
.put('/api/users/profile')
.set('x-access-token', data.token)
.send({
email: 'updated-admin@test.com'
});
expect(response.body).to.have.property('email', 'updated-admin@test.com');
});
});
});
12 changes: 11 additions & 1 deletion src/server/app/users/user.controller.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Body, Controller, Get, Post, UseGuards } from '@nestjs/common';
import { Body, Controller, Get, Post, UseGuards, Put } from '@nestjs/common';

import { AccessTokenGuard, CurrentUser } from '../core';

Expand Down Expand Up @@ -30,4 +30,14 @@ export class UserController {
public getUsers() {
return this.userService.getUsers();
}

@Put('profile')
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

maybe a different endpoint?

Copy link
Member

Choose a reason for hiding this comment

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

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.

@UseGuards(AccessTokenGuard)
public async updateProfile(
@CurrentUser() currentUser: User,
@Body() user: User
): Promise<User> {
await this.userService.updateProfileById(currentUser.id, user);
return this.userService.getUserById(currentUser.id);
}
}
8 changes: 7 additions & 1 deletion src/server/app/users/user.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ export class UserRepository extends Repository<User> {

public async getUserByEmail(email: string): Promise<User> {
return this.findOne({
where: { email }
where: { email },
relations: ['roles']
});
}

public updateUserById(id: number, data: User): Promise<void> {
delete data.created;
Copy link
Member

Choose a reason for hiding this comment

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

Why delete the created property? Don't you think we need to update it instead?

return this.updateById(id, data);
}

}
12 changes: 7 additions & 5 deletions src/server/app/users/user.service.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component, Inject, UnauthorizedException } from '@nestjs/common';
import { classToPlain } from 'class-transformer';

import { BcryptService, JsonWebTokenService } from '../core';
import { User } from './user.entity';
Expand All @@ -22,11 +23,7 @@ export class UserService {
if (!isValidPassword) {
throw new UnauthorizedException();
}
const token = this.jwtService.sign({
id: user.id,
email: user.email,
roles: user.roles
});
const token = this.jwtService.sign(classToPlain(user));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I think jwt returns unique token even the data is always the same

return { user, token };
}

Expand All @@ -39,4 +36,9 @@ export class UserService {
public async getUsers() {
return this.userRepository.getUsers({});
}

public updateProfileById(id: number, data: User) {
delete data.password;
Copy link
Member

Choose a reason for hiding this comment

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

Wait, we cannot update without password? 😱 😱 😱 Why delete? Can we not use the token?

Copy link
Author

Choose a reason for hiding this comment

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

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

return this.userRepository.updateUserById(id, data);
}
}