feat: userController [ISSUE-1,9,65] #66

ttkmw · 2019-06-12T16:20:28Z

Related Issue

resolve: #1, #9 , #65

Description

user controller & e2e test

Checklist

Thank you for your contribution.
Before submitting this PR, please make sure:

Test case
End of Work

junbeomlee · 2019-06-16T15:04:50Z

server/src/web/user/user.controller.spec.ts

+import {User} from '../../domain/user/user.entity';
+import {UserDto} from '../../app/user/dto/user.dto';
+import {BadRequestException} from '@nestjs/common';
+


we should specify error(ex InvalidParameterException, ValidationException... ) such as BadRequestException

junbeomlee · 2019-06-16T15:05:31Z

server/src/web/user/user.controller.ts

+    async delete(@Param('id') id: number): Promise<DeleteResult> {
+        return await this.service.delete(id);
+    }
+    @Put(':id/increase-level')


let's talk about url naming!

i was not sure http method and url is good.

i referenced: github-api
PUT /repos/:owner/:repo/pulls/:pull_number/update-branch

it was using verb in url

How about it changes to @Put(':id/level') and amount has positive amount or negative amount?

I think using verb is better

level is not entity or object.

And if verb doesn’t exist, maybe client think create level with just method and parameter.

Then, how about @Put(':id?level=amount')?

that's good but
maybe client will think like "set level with the amount".

like i wanted to say increase level 2 up but client will think like level = 2.

and there is another issue that one controller should route the url only with the parameter.
i mean one controller should cover 'increase-level', 'increase-point' like that. i think that is not goood

about the comment @Put(':id/level'):

i think only resources should be used as 'noun'.
seeing reference,
it says resource is like object in oop.

A resource is an object with a type, associated data, relationships to other resources, and a set of methods that operate on it.

resources has have data associated with them,

so i think resource is like 'entity'.

but level is just value.

junbeomlee · 2019-06-16T15:08:38Z

server/test/user/user.e2e-spec.ts

+// todo: implement ExceptionHandler, apply custom Error
+
+describe('UserController', () => {
+    let app: INestApplication;


plz read nestjs/nest#1843 this to rollback transactions

i used dropSchema option at config file.

it's a little bit different from the reference, but i wanted to gather all the configuration in config file which is not applied at the reference

junbeomlee · 2019-06-16T15:09:28Z

server/test/user/user.e2e-spec.ts

+    const initUserForDecreasePoint = {id: 3, address: 'thirdAddress', name: 'thirdName', point: 10, level: 0};
+    const initUserForDelete = {id: 4, address: 'fourthAddress', name: 'fourthName', point: 0, level: 0};
+
+    beforeAll(async () => {


in e2e test cases we don't need to restart whole app per a test case!!

didn't understand. it's 'beforeAll', not 'beforeEach'

junbeomlee · 2019-06-16T15:10:32Z

server/test/user/user.e2e-spec.ts

+import {getConnection} from 'typeorm';
+import * as assert from 'assert';
+
+// todo: implement ExceptionHandler, apply custom Error


It seems to be possible to perform the test with one user data.

i minimized the default data, but needed 2 data. That's because i needed 2 state: user with 10 point, user with 0 point

junbeomlee

Except the comment!
LGTM

hihiboss · 2019-06-19T17:21:00Z

server/src/port/persistence/repository/repository.config.service.ts

+import {TypeOrmModuleOptions, TypeOrmOptionsFactory} from '@nestjs/typeorm';
+
+@Injectable()
+export class RepositoryConfigService implements TypeOrmOptionsFactory {


If you implement TypeOrmOptionsFactory, should you name RepositoryConfigFactory?

Why does RepositoryConfigService implement TypeOrmOptionsFactory?

it make sense ur comment but I think name repositoryconfihservice is better

We are using "service" at all the other place. i used service for consistency.

other references are using name "service" at the same usage like ConfigService.

Oh, I see!

I understand your mind. Thank you :)

hihiboss · 2019-06-19T17:25:15Z

server/src/app/user/user.service.impl.ts

-        errors = user.canDecreasePoint(amount);
-        if (errors.length !== 0) {
-            throw new Error('can not decrease point');
+            throw new NotFoundException('user with the id is not found');


In line 17, you throw BadRequestException, but in line 54, you throw NotFoundException.

Why did you separated "finding undefined user" to BadRequestException and NotFoundException?

thanks!! very nice

hihiboss · 2019-06-19T17:28:29Z

server/src/web/user/user.controller.ts

+    async delete(@Param('id') id: number): Promise<DeleteResult> {
+        return await this.service.delete(id);
+    }
+    @Put(':id/increase-level')


How about it changes to @Put(':id/level') and amount has positive amount or negative amount?

hihiboss · 2019-06-19T17:29:54Z

server/src/web/user/user.controller.ts

+    async increaseLevel(@Param('id') id: number, @Query('amount', new ParseIntPipe()) amount): Promise<User> {
+        return await this.service.increaseLevel(id, amount);
+    }
+    @Put(':id/increase-point')


Also, I suggest that it changes to @Put(':id/point') and amount has positive amount or negative amount.

point is not object. that's why i didn't use noun for point at the url

How about @Put(':id?point=amount')?

hihiboss · 2019-06-19T17:36:53Z

I've already known it's too tiny but, commit message format is [type]: [msg], not [type] : [msg]. Just whitespace... :)

ttkmw · 2019-06-19T22:59:42Z

i referenced example at the contributing guide

example:
[type] : [description]
docs : add more description about PBFT [ISSUE-1]
fix : change type int to unit [ISSUE-20]

all the samples has whitespace

@hihiboss

hihiboss · 2019-06-20T08:10:38Z

@inDlife

Oh, really sorry to commit message!

In our commit logs, messages have [type]: [message] style.
But, I checked that our contribution guide says [type] : [message].

Let's follow contribution guide or modify contribution guide!

ttkmw · 2019-06-20T12:35:36Z

@hihiboss
it's okay. respect ur precise, details(꼼꼼함ㅎㅎ)

junbeomlee

LGTM

junbeomlee · 2019-06-19T10:25:54Z

server/test/user/user.e2e-spec.ts

+    const initUserForDecreasePoint = {id: 3, address: 'thirdAddress', name: 'thirdName', point: 10, level: 0};
+    const initUserForDelete = {id: 4, address: 'fourthAddress', name: 'fourthName', point: 0, level: 0};
+
+    beforeAll(async () => {


ttkmw force-pushed the feature/userController branch 2 times, most recently from a866ea9 to a4bae7d Compare June 16, 2019 13:34

junbeomlee reviewed Jun 16, 2019

View reviewed changes

ttkmw force-pushed the feature/userController branch from a4bae7d to 3eb4c3e Compare June 18, 2019 14:37

ttkmw changed the title ~~[WIP]Feature/userController & e2e test~~ feat: userController & e2e test Jun 18, 2019

ttkmw force-pushed the feature/userController branch 4 times, most recently from b9c1512 to c358994 Compare June 19, 2019 12:49

ttkmw added 3 commits June 19, 2019 23:58

feat : set database config and connect database [ISSUE-1]

da673a1

feat : implement UserController [ISSUE-9]

5c572bf

test : add unit test of UserController [ISSUE-9]

ebce797

ttkmw force-pushed the feature/userController branch from c358994 to 5642f25 Compare June 19, 2019 15:11

ttkmw changed the title ~~feat: userController & e2e test~~ feat: userController [ISSUE-1,9,65] Jun 19, 2019

hihiboss reviewed Jun 19, 2019

View reviewed changes

ttkmw force-pushed the feature/userController branch from 5642f25 to 9c64854 Compare June 20, 2019 11:12

test : add e2e test of user domain [ISSUE-65]

cac0e25

ttkmw force-pushed the feature/userController branch from 9c64854 to cac0e25 Compare June 20, 2019 14:19

junbeomlee approved these changes Jun 24, 2019

View reviewed changes

junbeomlee merged commit 723dfbb into DE-labtory:develop Jun 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: userController [ISSUE-1,9,65] #66

feat: userController [ISSUE-1,9,65] #66

ttkmw commented Jun 12, 2019 •

edited

Loading

junbeomlee Jun 16, 2019

junbeomlee Jun 16, 2019

ttkmw Jun 17, 2019

hihiboss Jun 19, 2019

ttkmw Jun 19, 2019 •

edited

Loading

hihiboss Jun 20, 2019 •

edited

Loading

ttkmw Jun 20, 2019 •

edited

Loading

ttkmw Jun 20, 2019

junbeomlee Jun 16, 2019

ttkmw Jun 18, 2019 •

edited

Loading

junbeomlee Jun 16, 2019 •

edited

Loading

ttkmw Jun 18, 2019

junbeomlee Jun 19, 2019

junbeomlee Jun 16, 2019

ttkmw Jun 18, 2019

junbeomlee left a comment

hihiboss Jun 19, 2019

ttkmw Jun 19, 2019

hihiboss Jun 20, 2019

hihiboss Jun 19, 2019

ttkmw Jun 20, 2019

hihiboss Jun 19, 2019

hihiboss Jun 19, 2019

ttkmw Jun 19, 2019

hihiboss Jun 20, 2019

hihiboss commented Jun 19, 2019 •

edited

Loading

ttkmw commented Jun 19, 2019

hihiboss commented Jun 20, 2019

ttkmw commented Jun 20, 2019

junbeomlee left a comment

junbeomlee Jun 19, 2019

feat: userController [ISSUE-1,9,65] #66

feat: userController [ISSUE-1,9,65] #66

Conversation

ttkmw commented Jun 12, 2019 • edited Loading

Related Issue

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttkmw Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

hihiboss Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

ttkmw Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttkmw Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

junbeomlee Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junbeomlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hihiboss commented Jun 19, 2019 • edited Loading

ttkmw commented Jun 19, 2019

hihiboss commented Jun 20, 2019

ttkmw commented Jun 20, 2019

junbeomlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttkmw commented Jun 12, 2019 •

edited

Loading

ttkmw Jun 19, 2019 •

edited

Loading

hihiboss Jun 20, 2019 •

edited

Loading

ttkmw Jun 20, 2019 •

edited

Loading

ttkmw Jun 18, 2019 •

edited

Loading

junbeomlee Jun 16, 2019 •

edited

Loading

hihiboss commented Jun 19, 2019 •

edited

Loading