Skip to content
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

fix: organization apis are accessible through api key #4619

Merged
merged 7 commits into from Oct 25, 2023
Merged

Conversation

jainpawan21
Copy link
Member

@jainpawan21 jainpawan21 commented Oct 23, 2023

What change does this PR introduce?

  1. Remove @ApiExcludeController() and add @ExternalApiAccessible() decorator
  2. Add OrganizationResponseDto

Fixes NV-1920

Why was this change needed?

So that organizations' APIs can be accessed using API Key

Other information (Screenshots)

@linear
Copy link

linear bot commented Oct 23, 2023

NV-3033 make organization-related apis public

Organisations APIs are excluded from swagger and can't be accessed using API key

@linear
Copy link

linear bot commented Oct 23, 2023

NV-1920 remove @ApiExcludeController from organisation controller

Remove ApiExcludeController from organisation controller so that organisation apis are shown in api documentation.

Tasks:-

  1. Remove ApiExcludeController
  2. Make sure all routes have request and response dtos
  3. Request and Response payloads are correctly showing in api-docs
  4. All apis are accessible using api key

Copy link
Contributor

@djabarovgeorge djabarovgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look like awesome work :)
I left a couple of comments for your review.


@IsString()
@IsOptional()
logo?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add here @ApiPropertyOptional()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ✅

@@ -81,6 +93,11 @@ export class OrganizationController {
}

@Get('/me')
@ExternalApiAccessible()
@ApiResponse(OrganizationResponseDto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add?

Suggested change
@ApiResponse(OrganizationResponseDto)
@ApiResponse(OrganizationResponseDto, 200)

@ApiOperation({
summary: 'Update a member role to admin',
})
@ApiParam({ name: 'memberId', type: String, required: true })
async updateMemberRoles(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add here?
@ApiResponse(MemberDto, 200)

@ExternalApiAccessible()
@ApiOperation({
summary: 'Fetch all members of current organizations',
})
async getMember(@UserSession() user: IJwtPayload) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add here?
@ApiResponse(MemberDto, 200, true)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added MemberDto ✅

@@ -130,19 +161,12 @@ export class OrganizationController {
);
}

@Post('/members/invite')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what this endpoint was responsible of but i guess we don't need @post('/members/invite') that returns members. 👏

@Put('/branding')
@ExternalApiAccessible()
@ApiResponse(OrganizationBrandingResponseDto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be with 200?

Suggested change
@ApiResponse(OrganizationBrandingResponseDto)
@ApiResponse(OrganizationBrandingResponseDto, 200)

@Roles(MemberRoleEnum.ADMIN)
@ApiResponse(RenameOrganizationDto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be with 200?

Suggested change
@ApiResponse(RenameOrganizationDto)
@ApiResponse(RenameOrganizationDto, 200)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ApiResponse() has 200 as default status code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, good to know :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@jainpawan21
Copy link
Member Author

@djabarovgeorge Please look into this PR again 🙂

Copy link
Contributor

@djabarovgeorge djabarovgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah looks good to me, left a couple of comments related to class-validator and nestjs/swagger decorators.
To be honest, I don't even know if we need class-validator decorators in the DTO because we don't validate them, it could be a good idea to add validation IMO but from what i know we still don't.

invitationDate: Date;

@ApiProperty()
@IsDate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@IsDate()
@IsDate()
@ApiPropertyOptional()

answerDate?: Date;

@ApiProperty()
@IsDate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@IsDate()
@IsString()

enum: MemberRoleEnum,
isArray: true,
})
@IsArray()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@IsArray()
@IsEnum(MemberRoleEnum)

@ApiPropertyOptional({
enum: { ...MemberStatusEnum },
})
@IsString()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@IsString()
@IsEnum(MemberStatusEnum)

import { UpdateBrandingDetailsDto } from './update-branding-details.dto';

export class IPartnerConfigurationResponseDto {
@ApiPropertyOptional()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@ApiPropertyOptional()
@ApiPropertyOptional()
@IsArray()
@IsString({ each: true })

Comment on lines 10 to 17
@ApiProperty()
accessToken: string;

@ApiProperty()
configurationId: string;

@ApiPropertyOptional()
teamId: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IsString()

enum: { ...PartnerTypeEnum },
description: 'Partner Type Enum',
})
@IsString()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@IsString()
@IsEnum(PartnerTypeEnum)

@IsObject()
branding: OrganizationBrandingResponseDto;

@ApiPropertyOptional()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@ApiPropertyOptional()
@ApiPropertyOptional()
@IsObject()

@jainpawan21 jainpawan21 merged commit 816b14a into next Oct 25, 2023
22 of 26 checks passed
@jainpawan21 jainpawan21 deleted the NV-3033 branch October 25, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants