[TM-1311] Implement password reset #46
Conversation
| @ApiProperty({ | ||
| description: | ||
| 'User email', | ||
| example: '[email protected]', |
There was a problem hiding this comment.
I haven't seen how this is used yet, but this description / example doesn't seem right for a property named message.
| @@ -0,0 +1,3 @@ | |||
| export class ResetPasswordDto { | |||
| newPassword: string; | |||
| } | |||
There was a problem hiding this comment.
This should have some validation on it. See login-request.dto.ts for an example.
| import { NoBearerAuth } from '@terramatch-microservices/common/guards'; | ||
| import { ResetPasswordResponseOperationDto } from "./dto/reset-password-response-operation.dto"; | ||
|
|
||
| @Controller('auth/v3/reset-password') |
There was a problem hiding this comment.
For resource named endpoints in this codebase, they should be camel case and plural. I would probably call this passwordResets.
| export class ResetPasswordController { | ||
| constructor(private readonly resetPasswordService: ResetPasswordService) {} | ||
|
|
||
| @Post('request') |
There was a problem hiding this comment.
When creating a resource (in the case of this controller, a password reset), it's more typical in REST to post directly to the root resource path. So, this would be simply Post(), and the client will then issue a POST to auth/v3/passwordResets as a create action.
| .document.serialize(); | ||
| } | ||
|
|
||
| @Post('reset/:token') |
There was a problem hiding this comment.
I think it would follow REST conventions better if this were @Put(':token'). Symantically, that would mean that you're updating the given passwordReset resource, which aligns a little better with REST for this kind of weird virtual resource.
|
|
||
|
|
||
| @Table({ tableName: "localization_keys", underscored: true, paranoid: false }) | ||
| export class LocalizationKeys extends Model<LocalizationKeys> { |
There was a problem hiding this comment.
The convention in laravel is to have the database table name be pluralized, but in this codebase all entities are singular: LocalizationKey
|
|
||
|
|
||
|
|
||
| @Table({ tableName: "localization_keys", underscored: true, paranoid: false }) |
There was a problem hiding this comment.
paranoid: false is the default and may be left out of this config.
| @Column(STRING) | ||
| key: string | null; | ||
|
|
||
| @AllowNull |
There was a problem hiding this comment.
According to the current DB schema, key and value are not allowed to be null. Please try to follow the current schema as closely as possible when defining columns in this codebase.
| key: string | null; | ||
|
|
||
| @AllowNull | ||
| @Column(STRING) |
There was a problem hiding this comment.
This column appears to be a TEXT type, not STRING in the DB (text vs varchar(255))
| value: string | null; | ||
|
|
||
| @AllowNull | ||
| @Unique |
There was a problem hiding this comment.
I'm not sure if these value ids are required to be unique or not, but I do see that the current DB schema doesn't set a uniqueness constraint on this column, so we shouldn't here either.
https://gfw.atlassian.net/browse/TM-1311