-
Notifications
You must be signed in to change notification settings - Fork 0
Feat url #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat-throttler
Are you sure you want to change the base?
Feat url #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laxman-aqw Changes to Auth seems to be out of place for this PR. Any reason for the changes here?
src/email/email.controller.ts
Outdated
| @@ -0,0 +1,29 @@ | |||
| // import { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to delete the file itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
| CREATE TABLE "urls" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| "user_id" uuid NOT NULL, | ||
| "encrypted_url" varchar (2048) NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need encrypted_url ? Can it be reconstructed using short_code if needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the short_code is just a random identifier so i think we cannot.
| @@ -0,0 +1,9 @@ | |||
| import { IsNotEmpty } from 'class-validator'; | |||
|
|
|||
| export class CreateUrlRequestData { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request also needs the expiry time that the user can set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/url/url.controller.ts
Outdated
| @Req() request: RequestWithUser, | ||
| ) { | ||
| const userData = request.decodedData; | ||
| if (!userData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be part of validation pipeline, instead of manual check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/analytics/types.ts
Outdated
| family: string; | ||
| major: string; | ||
| minor: string; | ||
| patch: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of repeating this can't we make a seperate interface for this and use it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/auth/auth.service.ts
Outdated
| const existingUser = await this.userRepository.findOne({ | ||
| where: [ | ||
| { username: signUpUserDto.username }, | ||
| { email: signUpUserDto.email }, | ||
| ], | ||
| }); | ||
|
|
||
| if (userByUsername) { | ||
| throw new BadRequestException('Username already taken'); | ||
| if (existingUser) { | ||
| if (existingUser.username === signUpUserDto.username) { | ||
| throw new BadRequestException('Username already taken'); | ||
| } | ||
|
|
||
| if (existingUser.email === signUpUserDto.email) { | ||
| throw new BadRequestException('Email already taken'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User creation shouldn't be within user's module ?
async createUser(payload: SignupRequestData){
// here should be sign up logic
}
Should be called in auth service as follows :
private readonly userService:userService
this.userService.createUser()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/cron/check-url-expiry.service.ts
Outdated
| const user = await this.userRepository.findOneByOrFail({ | ||
| id: url.userId, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than calling injection repostiory for user here, better create a findUserById method in user service and call user service from here
src/url/dto/get-urls-request-data.ts
Outdated
| export class GetUrlRequestData { | ||
| title: string; | ||
| shortCode: string; | ||
| expiresAt: Date; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does get ` method have request data ? Is payload allowed in get ? or is it response data ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it to response
src/url/url.controller.ts
Outdated
| @Req() req: RequestWithUser, | ||
| ) { | ||
| const { longCode } = await this.urlService.getLongUrl(shortCode, req); | ||
| return { url: longCode, statusCode: 302 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a decorator to set http status in nest js , better use that above the method call. this is not a good approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
| @Column({ type: 'uuid', name: 'user_id' }) | ||
| readonly userId: string; | ||
|
|
||
| @Column({ type: 'varchar', length: 2048, name: 'encrypted_url' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are unsure regarding the length you can use text type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments added . Focus on seperation of concerns
src/url/url.service.ts
Outdated
| const urls = (await this.urlRepository.find({ | ||
| where: { userId: userId }, | ||
| select: ['title', 'shortCode', 'expiresAt'], | ||
| })) as GetUrlRequestData[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the type name . Also can't it be urls:GetUrlRequestData[]=
Another best way might be
const urls: Pick<Url,'title'|'shortCode'|'expiresAt'>
instead a creating a new type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/url/url.service.ts
Outdated
| if (deletedUrl.affected === 0) { | ||
| throw new NotFoundException('URL not found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have checked existence of url above and you are again checking if url is there or not here. Doesn't seem logical .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/user/user.service.ts
Outdated
| if (value === undefined || value === null) { | ||
| throw new BadRequestException(`Invalid value for field: ${field}`); | ||
| } | ||
| if (value === undefined || value === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(!value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Time stamp on file and timestamp on migration class name doesn't match ????
src/cron/check-url-expiry.service.ts
Outdated
| where: { expiresAt: LessThan(new Date()), expiryAlertedAt: IsNull() }, | ||
| }); | ||
| for (const url of expiredUrls) { | ||
| const user = await this.userRepository.findOneByOrFail({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findOneByOrFail throws different error in local and in production . So better use findOne and then check validation if(!user)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments added
…up logic and user service to handle user creation logic
Added URL module setup.
Implemented features to add and edit URLs.
Added a new column original_url in the urls table to:
Store the original (hashed) URL.
Prevent creation of duplicate URLs.
Integrated guards to ensure authenticated access for URL CRUD operations.
removed console
added errorhandler method to handle error
updated auth to allow user login only after verification
set expiry time in createurl
updated authguard to check userid