[TM-1311] Implement password reset #46
Conversation
apps/user-service/src/auth/dto/reset-password-response-operation.dto.ts
Outdated
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit 746b750.
☁️ Nx Cloud last updated this comment at |
| this.transporter = nodemailer.createTransport({ | ||
| host: this.configService.get<string>('MAIL_HOST'), | ||
| port: this.configService.get<number>('MAIL_PORT'), | ||
| secure: false, // true for 465, false for other ports |
There was a problem hiding this comment.
This comment is a little odd. What if MAIL_PORT where configured to be 465? What will it be in AWS?
There was a problem hiding this comment.
This is just for the actual configuration, we can take the config value from the configservice in case that is needed, but for now I think is ok, or do you want me to take this value from env?
There was a problem hiding this comment.
I was mainly reacting to the comment noting that for some port values this should be true, but the port value is configured externally. I would do this line as
secure: this.configService.get<number>('MAIN_PORT') === 465,
| async requestReset(@Body() { emailAddress, callbackUrl }: ResetPasswordRequest): Promise<JsonApiDocument> { | ||
| const { email, userId } = await this.resetPasswordService.sendResetPasswordEmail(emailAddress, callbackUrl); | ||
| return buildJsonApi() | ||
| .addData(`${userId}`, new ResetPasswordResponseDto({ emailAddress: email })) |
There was a problem hiding this comment.
Please use UUID here (and update the type on the DTO decorator). We should be favoring UUIDs for all API surface areas as much as possible.
I realize the login controller is using numerical ID. It doesn't actually matter in that case, and I should probably change (and the user find endpoint; while only used for me right now, it does accept a numerical ID for future admin use that I want to switch to UUID). The recently added user update endpoint does use UUID though, and all other endpoints that exist today and refer to a real resource use UUID.
| import { ConfigService } from "@nestjs/config"; | ||
|
|
||
| @Injectable() | ||
| export class TranslationService { |
There was a problem hiding this comment.
If you feel strongly about keeping them separate, you can. If it were me, I would just fold the stuff from Localization service into this service and just have the one service.
| @@ -21,19 +13,18 @@ export class i18nItem extends Model<i18nItem> { | |||
| status: string | null; | |||
|
|
|||
| @AllowNull | |||
| @Column(STRING) | |||
| @Column(DataTypes.STRING(255)) | |||
There was a problem hiding this comment.
varchar(255) is the default for STRING, so adding the 255 here is not needed.
There was a problem hiding this comment.
I haven't been as strict about getting things perfect as I was originally thinking I would. If it's useful, one tool I've used to check is once you've run some unit tests with the updated schema in place, you can use describe table on both the PHP database, and the testing database used for this service. That'll show you easily what differences there are.
However, I think that when we are ready to make this repo be the owner of the schema (maybe by summer? not sure), we'll probably have to do a big pass over all the tables, use some tooling to find any differences in schema and address each one, so right now it's just about limiting the number of column defs we're going to have to tweak at that time, so I just generally try to get them close and worry about it being perfect later. One thing in particular that this codebase isn't great at right now is keeping up with index definitions. I have a few of them specified, but we'll both need to make sure this codebase is aware of all the ones created by PHP when we switch and also do an audit for places where an index would speed things up and doesn't yet exist.
| export * from "./framework-user.entity"; | ||
| export * from "./framework.entity"; |
There was a problem hiding this comment.
Not that it's a big deal, but why these changes and the the others here around files that aren't part of this PR? framework should come before framework-user in an alphabetical sort
https://gfw.atlassian.net/browse/TM-1311