Deepika test - #14
Deepika test#14app-dev-198 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Automated Testing:
- Lint: passes
- test: fails
- coverage: fails
- build: pass
Manual Review
- Has Angular animations
- Good use of services
- Has auth guards
- No readme describing changes
- Good use of forms (except password field with type=text)
- Lazy loading modules
- Broken route in AuthGuard
- Async looks good
- Use of local storage for a JWT is a security vulnerability.
- Not great directory structure (all components to go directly into their respective module?)
- Would like to see more typescript features (privacy modifiers / return types / type setting)
| @Injectable() | ||
| export class AuthService { | ||
|
|
||
| loginUrl: any = 'https://dev-test-service.madebywiser.com/login'; |
There was a problem hiding this comment.
Doesn't need to be type: any
Should have a privacy modifier
Could be getting this value from the environment file
|
|
||
| loginUrl: any = 'https://dev-test-service.madebywiser.com/login'; | ||
|
|
||
| authenticationUrl: any = 'https://dev-test-service.madebywiser.com/me'; |
There was a problem hiding this comment.
Doesn't need to be type: any
Should have a privacy modifier
Could be getting this value from the environment file
| public profile: BehaviorSubject < object > = new BehaviorSubject < object > ({}); | ||
| constructor(private http: HttpClient) {} | ||
|
|
||
| login(user) { |
There was a problem hiding this comment.
Would be very nice to have privacy modifiers and return types on these methods
| it('should delete token', inject([AuthService], (service: AuthService) => { | ||
| service.setToken('xyxxxx'); | ||
| const token = service.deleteToken(); | ||
| expect(token).toBeUndefined(); |
| }); | ||
|
|
||
| it('should return true', inject([AuthGuardService], (guard: AuthGuardService) => { | ||
| expect(guard.canActivate()).toBeFalsy(); |
There was a problem hiding this comment.
'should return true'
expect to be falsy?
|
|
||
| // Component transition animations | ||
| export const slideInDownAnimation = | ||
| trigger('routeAnimation', [ |
| } | ||
|
|
||
| setToken(token) { | ||
| localStorage.setItem('token', token); |
There was a problem hiding this comment.
localStorage for an access token? This could be seen as a security vulnerability.
| component: ErrorComponent | ||
| }, | ||
| { | ||
| path: 'login-error', |
There was a problem hiding this comment.
Should be in the AuthModule
| styleUrls: ['./login-error.component.css'], | ||
| animations: [ slideInDownAnimation ] | ||
| }) | ||
| export class LoginErrorComponent implements OnInit { |
There was a problem hiding this comment.
Should be in the AuthModule
| @@ -0,0 +1,44 @@ | |||
| import { Component, HostBinding, OnInit } from '@angular/core'; | |||
There was a problem hiding this comment.
Components (pages/partials) should probably be organized (grouped) into a common child directory of their respective module
3 participants
No description provided.