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

feat(api-client): Added workspace controller #427

Merged
merged 2 commits into from
Sep 14, 2024

Conversation

rajdip-b
Copy link
Member

Description

Fixes #348
Added the controllers for workspace.

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug
The defaultWorkspace is being set for API key authentication, but not for JWT authentication. This could lead to inconsistent behavior.

Performance Issue
Multiple database queries are being made to check user existence and create users. This could be optimized to reduce database calls.

Code Smell
The test suite is creating and deleting workspaces in each test, which could be optimized to improve test performance.

Copy link
Contributor

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Use URLSearchParams for constructing query parameters in the URL

Consider using a more robust method for URL parameter construction. The current
approach of manually appending parameters could lead to errors or inconsistencies.
Use a dedicated URL building library or a utility function to construct the URL with
query parameters.

packages/api-client/tests/workspace.spec.ts [82-87]

-let url = `/api/workspace?`
-request.page && (url += `page=${request.page}&`)
-request.limit && (url += `limit=${request.limit}&`)
-request.sort && (url += `sort=${request.sort}&`)
-request.order && (url += `order=${request.order}&`)
-request.search && (url += `search=${request.search}&`)
+const params = new URLSearchParams();
+if (request.page) params.append('page', request.page.toString());
+if (request.limit) params.append('limit', request.limit.toString());
+if (request.sort) params.append('sort', request.sort);
+if (request.order) params.append('order', request.order);
+if (request.search) params.append('search', request.search);
+const url = `/api/workspace?${params.toString()}`;
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Simplify nested if-else structure using early returns

The code is using a nested if-else structure which can be simplified. Consider using
early returns to reduce nesting and improve readability.

apps/api/src/auth/guard/auth/auth.guard.ts [75-82]

-if (process.env.NODE_ENV === 'test') {
-  const email = request.headers[X_E2E_USER_EMAIL]
-  if (!email) {
-    throw new ForbiddenException()
-  }
+if (process.env.NODE_ENV !== 'test') {
+  const request = context.switchToHttp().getRequest()
+  // Rest of the non-test logic
+  return
+}
 
-  user = await getUserByEmailOrId(email, this.prisma)
-} else {
-  const request = context.switchToHttp().getRequest()
+const email = request.headers[X_E2E_USER_EMAIL]
+if (!email) {
+  throw new ForbiddenException()
+}
 
+user = await getUserByEmailOrId(email, this.prisma)
+
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Use the appropriate assertion method to match the expected behavior

Consider using toBeNull() instead of toBeUndefined() to match the expected behavior
of defaultWorkspace being null.

apps/api/src/user/user.e2e.spec.ts [156]

+expect(createAdminUserResponse.defaultWorkspace).toBeNull()
 
-
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Best practice
Use a constant for the error message in the BadRequestException

The error message in the BadRequestException could be more specific. Instead of
using a template literal, consider creating a constant for the error message to
improve maintainability and consistency across the codebase.

apps/api/src/workspace-membership/service/workspace-membership.service.ts [97-99]

-throw new BadRequestException(
-  `${otherUser.email} has not accepted the invitation to workspace ${workspace.name} (${workspace.slug})`
-)
+const INVITATION_NOT_ACCEPTED_ERROR = 'User has not accepted the invitation to the workspace';
+throw new BadRequestException(INVITATION_NOT_ACCEPTED_ERROR);
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Handle or log errors in the catch clause instead of ignoring them

The try-catch block is using an empty catch clause, which silently ignores any
errors. It's generally a bad practice to ignore errors without at least logging
them. Consider logging the error or handling it appropriately.

apps/api/src/auth/service/auth.service.ts [181-183]

 try {
   user = await getUserByEmailOrId(email, this.prisma)
-} catch (ignored) {}
+} catch (error) {
+  this.logger.warn(`Failed to get user by email: ${email}`, error);
+}
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Use consistent import paths for better maintainability

Consider using a relative import path for the WorkspaceController to maintain
consistency with other imports in the file.

packages/api-client/src/index.ts [7]

-import WorkspaceController from './controllers/workspace'
+import WorkspaceController from '@api-client/controllers/workspace'
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Possible issue
Make a property optional to handle cases where it might not be present

Consider making the defaultWorkspace property optional in the
AuthenticatedUserContext interface, as it might not always be present.

apps/api/src/auth/auth.types.ts [8-12]

 export type AuthenticatedUserContext = User & {
   isAuthViaApiKey?: boolean
   apiKeyAuthorities?: Set<Authority>
-  defaultWorkspace: Workspace
+  defaultWorkspace?: Workspace
 }
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Maintainability
Use a named constant for configuration values to improve maintainability

Consider using a constant or configuration value for the timeout duration instead of
hardcoding it, to make it easier to adjust in the future.

packages/api-client/tests/config/setup.ts [53-56]

+const API_LAUNCH_TIMEOUT = 10000;
 setTimeout(() => {
   console.log('API launched')
   resolve()
-}, 10000)
+}, API_LAUNCH_TIMEOUT)
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.50%. Comparing base (ce50743) to head (f8795de).
Report is 150 commits behind head on develop.

Files with missing lines Patch % Lines
apps/api/src/cache/cache.service.ts 33.33% 2 Missing ⚠️
apps/api/src/auth/guard/auth/auth.guard.ts 80.00% 1 Missing ⚠️
apps/api/src/common/util.ts 0.00% 1 Missing ⚠️
apps/api/src/user/service/user.service.ts 83.33% 1 Missing ⚠️
...membership/service/workspace-membership.service.ts 92.85% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #427      +/-   ##
===========================================
- Coverage    91.71%   87.50%   -4.22%     
===========================================
  Files          111      105       -6     
  Lines         2510     2672     +162     
  Branches       469      405      -64     
===========================================
+ Hits          2302     2338      +36     
- Misses         208      334     +126     
Flag Coverage Δ
api-e2e-tests 87.50% <87.50%> (-4.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rajdip-b rajdip-b merged commit 2f4edec into develop Sep 14, 2024
6 of 7 checks passed
@rajdip-b rajdip-b deleted the feat/api-client-workspace-controller branch September 14, 2024 06:14
rajdip-b pushed a commit that referenced this pull request Sep 16, 2024
## [2.5.0](v2.4.0...v2.5.0) (2024-09-16)

### 🚀 Features

* **api-client:** Added workspace controller ([#427](#427)) ([2f4edec](2f4edec))
* **api-client:** Added workspace role controller ([#430](#430)) ([b03ce8e](b03ce8e))
* **api-client:** Synced with latest API ([27f4309](27f4309))
* **api:** Add slug in entities ([#415](#415)) ([89e2fcc](89e2fcc))
* **api:** Included default workspace details in getSelf function ([#414](#414)) ([e67bbd6](e67bbd6))
* **platform:** Add loading skeleton in the [secure]s page ([#423](#423)) ([a97681e](a97681e))
* **schema:** Added a schema package ([01ea232](01ea232))
* **web:** Update about and careers page ([e167f53](e167f53))

### 🐛 Bug Fixes

* **api:** Error messages fixed in api-key service ([#418](#418)) ([edfbce0](edfbce0))

### 📚 Documentation

* Fixed minor typo in postman workspace link ([#411](#411)) ([ed23116](ed23116))
* Updated Postman links ([444bfb1](444bfb1))

### 🔧 Miscellaneous Chores

* **api:** Suppressed version check test in [secure] ([4688e8c](4688e8c))
* **api:** Update slug generation method ([#420](#420)) ([1f864df](1f864df))

### 🔨 Code Refactoring

* **API:** Refactor workspace-membership into a separate module ([#421](#421)) ([574170f](574170f))
* **platform:** added optional chaining due to strict null check ([#413](#413)) ([907e369](907e369))
@rajdip-b
Copy link
Member Author

🎉 This PR is included in version 2.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
## [2.5.0](keyshade-xyz/keyshade@v2.4.0...v2.5.0) (2024-09-16)

### 🚀 Features

* **api-client:** Added workspace controller ([keyshade-xyz#427](keyshade-xyz#427)) ([2f4edec](keyshade-xyz@2f4edec))
* **api-client:** Added workspace role controller ([keyshade-xyz#430](keyshade-xyz#430)) ([b03ce8e](keyshade-xyz@b03ce8e))
* **api-client:** Synced with latest API ([27f4309](keyshade-xyz@27f4309))
* **api:** Add slug in entities ([keyshade-xyz#415](keyshade-xyz#415)) ([89e2fcc](keyshade-xyz@89e2fcc))
* **api:** Included default workspace details in getSelf function ([keyshade-xyz#414](keyshade-xyz#414)) ([e67bbd6](keyshade-xyz@e67bbd6))
* **platform:** Add loading skeleton in the [secure]s page ([keyshade-xyz#423](keyshade-xyz#423)) ([a97681e](keyshade-xyz@a97681e))
* **schema:** Added a schema package ([01ea232](keyshade-xyz@01ea232))
* **web:** Update about and careers page ([e167f53](keyshade-xyz@e167f53))

### 🐛 Bug Fixes

* **api:** Error messages fixed in api-key service ([keyshade-xyz#418](keyshade-xyz#418)) ([edfbce0](keyshade-xyz@edfbce0))

### 📚 Documentation

* Fixed minor typo in postman workspace link ([keyshade-xyz#411](keyshade-xyz#411)) ([ed23116](keyshade-xyz@ed23116))
* Updated Postman links ([444bfb1](keyshade-xyz@444bfb1))

### 🔧 Miscellaneous Chores

* **api:** Suppressed version check test in [secure] ([4688e8c](keyshade-xyz@4688e8c))
* **api:** Update slug generation method ([keyshade-xyz#420](keyshade-xyz#420)) ([1f864df](keyshade-xyz@1f864df))

### 🔨 Code Refactoring

* **API:** Refactor workspace-membership into a separate module ([keyshade-xyz#421](keyshade-xyz#421)) ([574170f](keyshade-xyz@574170f))
* **platform:** added optional chaining due to strict null check ([keyshade-xyz#413](keyshade-xyz#413)) ([907e369](keyshade-xyz@907e369))
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.

API-CLIENT: Create controller for Workspace module
1 participant