Unit tests for check method in PermissionsService #596
Conversation
|
@rafgpereira is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request updates the project configuration by modifying the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GitHub as GitHub Actions
participant NPM as npm
participant Jest as Jest Engine
Dev->>GitHub: Push changes
GitHub->>NPM: Run workflow
NPM->>Jest: Execute "jest --coverage"
Jest->>Dev: Return test results and coverage report
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/backend/src/services/auth/permissions/permissions.service.test.ts (4)
31-65: Consider adding validation for base test data.The base subscription and options objects are well-defined, but consider adding validation to ensure the test data remains valid if the underlying models change.
const baseSubscription = { id: 'mock-id', organizationId: 'mock-org-id', subscriptionTier: 'PRO' as SubscriptionTier, + // Add zod schema validation + const subscriptionSchema = z.object({ + id: z.string(), + organizationId: z.string(), + subscriptionTier: z.nativeEnum(SubscriptionTier), + // ... other fields + }); + subscriptionSchema.parse(baseSubscription);
102-178: Expand bypass verification test coverage.The bypass tests are good but could be enhanced:
- Test case for undefined requestedPermissions
- Test case for null requestedPermissions
- Test case for invalid permission combinations
180-317: Add edge cases for channel permission tests.Consider adding these test cases:
- Test with negative channel limits
- Test with undefined channel limits
- Test with non-numeric channel limits
318-401: Enhance monthly posts permission test coverage.The monthly posts tests could be improved with:
- Test case for subscription creation date edge cases
- Test case for date boundary conditions
- Test case for invalid post counts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/backend/src/services/auth/permissions/permissions.service.test.ts(1 hunks)package.json(2 hunks)
🔇 Additional comments (4)
apps/backend/src/services/auth/permissions/permissions.service.test.ts (2)
1-8: LGTM! Well-structured imports and mock setup.The test file follows good practices with clear organization of imports and mock setup.
1-404: Add missing test coverage for other sections.The test suite focuses on channel and monthly posts permissions but is missing coverage for:
- Team members permissions
- Webhook permissions
- AI feature permissions
Would you like me to generate additional test cases for these sections?
package.json (2)
32-33: LGTM! Test script with coverage reporting.The addition of the test script with coverage reporting aligns well with the PR objectives.
216-216: LGTM! Jest mock extension dependency.The addition of jest-mock-extended is appropriate for the mocking needs in the test suite.
|
Hey, so if I understand this correctly, this just makes Postiz more secure or what exactly does this do? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/backend/tsconfig.spec.json (1)
13-13: Clarify Redundant Include Entry.The explicit addition of "src/services/auth/permissions/check-permissions.service.spec.ts" appears redundant because the glob pattern "src/**/*.spec.ts" already covers all spec files under src—including this one. If there is a specific reason for explicitly including this file (e.g., special compilation settings or to override an exclusion elsewhere), please add a clarifying comment. Otherwise, consider removing the duplicate entry.
apps/backend/src/services/auth/permissions/remaining-permissions.service.test.ts (2)
31-48: Consider extracting test fixtures into a separate file.The test file contains large test fixtures that could be reused across different test files. Consider moving
baseSubscription,baseOptions, andbaseIntegrationinto a shared test fixtures file.+// test-fixtures/permissions.fixtures.ts +export const baseSubscription = { + id: 'mock-id', + // ... rest of the subscription object +}; + +export const baseOptions = { + channel: 10, + // ... rest of the options object +}; + +export const baseIntegration = { + id: 'mock-integration-id', + // ... rest of the integration object +};Also applies to: 50-65, 67-99
111-123: Consider adding edge case tests for channel quota calculation.While the current test verifies basic functionality, consider adding tests for:
- When subscription.totalChannels is undefined
- When all integrations need refresh
- When the limit is negative (as seen in pricing[tier].channel for non-FREE tiers)
apps/backend/src/services/auth/permissions/check-permissions.service.spec.ts (2)
180-317: Consider adding error case tests for channel permissions.While the happy path tests are comprehensive, consider adding tests for error cases:
- When getIntegrationsList throws an error
- When getPackageOptions throws an error
318-401: Consider using test.each for monthly posts permission tests.The monthly posts permission tests have similar structure with different data. Consider using test.each to make the tests more maintainable.
describe('Monthly Posts Permission', () => { test.each([ ['Posts Within Limit', 50, true], ['Posts Exceed Limit', 150, false], ])('%s', async (name, postCount, expectedResult) => { jest.spyOn(service, 'getPackageOptions').mockResolvedValue({ subscription: baseSubscription, options: { ...baseOptions, posts_per_month: 100 }, }); jest.spyOn(mockSubscriptionService, 'getSubscription').mockResolvedValue({ ...baseSubscription, createdAt: new Date(), }); jest.spyOn(mockPostsService, 'countPostsFromDay').mockResolvedValue(postCount); const result = await service.check( 'mock-org-id', new Date(), 'USER', [[AuthorizationActions.Create, Sections.POSTS_PER_MONTH]] ); expect(result.can(AuthorizationActions.Create, Sections.POSTS_PER_MONTH)).toBe(expectedResult); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/backend/src/services/auth/permissions/check-permissions.service.spec.ts(1 hunks)apps/backend/src/services/auth/permissions/permissions.service.ts(1 hunks)apps/backend/src/services/auth/permissions/remaining-permissions.service.test.ts(1 hunks)apps/backend/tsconfig.spec.json(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (3)
apps/backend/src/services/auth/permissions/remaining-permissions.service.test.ts (2)
102-109: LGTM! Test case for unsupported section is well-defined.The test case properly verifies that unsupported sections return 0 as expected.
124-132: LGTM! Webhook and posts quota tests are comprehensive.The test cases properly verify the remaining quota calculations for webhooks and posts per month.
Also applies to: 133-146
apps/backend/src/services/auth/permissions/check-permissions.service.spec.ts (1)
102-178: LGTM! Verification bypass tests are comprehensive.The test cases properly verify the bypass conditions for empty permission lists and missing Stripe key.
| async getRemainingQuota(orgId: string, section: Sections): Promise<number> { | ||
| if (section === Sections.CHANNEL) { | ||
| const { subscription, options } = await this.getPackageOptions(orgId); | ||
| const integrations = await this._integrationService.getIntegrationsList(orgId); | ||
| const validChannels = integrations.filter(integration => !integration.refreshNeeded).length; | ||
| let limit = options.channel; | ||
| if (subscription && subscription.totalChannels > limit) { | ||
| limit = subscription.totalChannels; | ||
| } | ||
| return limit - validChannels; | ||
| } else if (section === Sections.WEBHOOKS) { | ||
| const { options } = await this.getPackageOptions(orgId); | ||
| const totalWebhooks = await this._webhooksService.getTotal(orgId); | ||
| return options.webhooks - totalWebhooks; | ||
| } else if (section === Sections.POSTS_PER_MONTH) { | ||
| const { options } = await this.getPackageOptions(orgId); | ||
| const subscriptionInfo = await this._subscriptionService.getSubscription(orgId); | ||
| const createdAt = subscriptionInfo?.createdAt || new Date(); | ||
| const totalMonthsPassed = Math.abs(dayjs(createdAt).diff(dayjs(), 'month')); | ||
| const checkFrom = dayjs(createdAt).add(totalMonthsPassed, 'month'); | ||
| const count = await this._postsService.countPostsFromDay(orgId, checkFrom.toDate()); | ||
| return options.posts_per_month - count; | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicate logic between check() and getRemainingQuota().
The quota calculation logic is duplicated between check() and getRemainingQuota(). Consider extracting the common logic into private methods to improve maintainability.
+ private async getChannelQuota(orgId: string): Promise<{limit: number, used: number}> {
+ const { subscription, options } = await this.getPackageOptions(orgId);
+ const integrations = await this._integrationService.getIntegrationsList(orgId);
+ const validChannels = integrations.filter(integration => !integration.refreshNeeded).length;
+ let limit = options.channel;
+ if (subscription && subscription.totalChannels > limit) {
+ limit = subscription.totalChannels;
+ }
+ return { limit, used: validChannels };
+ }
+
+ private async getWebhooksQuota(orgId: string): Promise<{limit: number, used: number}> {
+ const { options } = await this.getPackageOptions(orgId);
+ const totalWebhooks = await this._webhooksService.getTotal(orgId);
+ return { limit: options.webhooks, used: totalWebhooks };
+ }
+
+ private async getPostsQuota(orgId: string, created_at: Date): Promise<{limit: number, used: number}> {
+ const { options } = await this.getPackageOptions(orgId);
+ const subscriptionInfo = await this._subscriptionService.getSubscription(orgId);
+ const createdAt = subscriptionInfo?.createdAt || created_at;
+ const totalMonthsPassed = Math.abs(dayjs(createdAt).diff(dayjs(), 'month'));
+ const checkFrom = dayjs(createdAt).add(totalMonthsPassed, 'month');
+ const count = await this._postsService.countPostsFromDay(orgId, checkFrom.toDate());
+ return { limit: options.posts_per_month, used: count };
+ }
async getRemainingQuota(orgId: string, section: Sections): Promise<number> {
if (section === Sections.CHANNEL) {
- const { subscription, options } = await this.getPackageOptions(orgId);
- const integrations = await this._integrationService.getIntegrationsList(orgId);
- const validChannels = integrations.filter(integration => !integration.refreshNeeded).length;
- let limit = options.channel;
- if (subscription && subscription.totalChannels > limit) {
- limit = subscription.totalChannels;
- }
- return limit - validChannels;
+ const { limit, used } = await this.getChannelQuota(orgId);
+ return limit - used;
} else if (section === Sections.WEBHOOKS) {
- const { options } = await this.getPackageOptions(orgId);
- const totalWebhooks = await this._webhooksService.getTotal(orgId);
- return options.webhooks - totalWebhooks;
+ const { limit, used } = await this.getWebhooksQuota(orgId);
+ return limit - used;
} else if (section === Sections.POSTS_PER_MONTH) {
- const { options } = await this.getPackageOptions(orgId);
- const subscriptionInfo = await this._subscriptionService.getSubscription(orgId);
- const createdAt = subscriptionInfo?.createdAt || new Date();
- const totalMonthsPassed = Math.abs(dayjs(createdAt).diff(dayjs(), 'month'));
- const checkFrom = dayjs(createdAt).add(totalMonthsPassed, 'month');
- const count = await this._postsService.countPostsFromDay(orgId, checkFrom.toDate());
- return options.posts_per_month - count;
+ const { limit, used } = await this.getPostsQuota(orgId, new Date());
+ return limit - used;
}
return 0;
}There was a problem hiding this comment.
This code has been removed from the PR
There was a problem hiding this comment.
🧩 Analysis chain
@rafgpereira I can still see the getRemainingQuota method in the file. Could you please clarify if you're referring to a different code change that was removed, or if the removal is still in progress? This will help me provide more accurate assistance.
🏁 Script executed:
#!/bin/bash
# Check if the getRemainingQuota method is still present in the file
ast-grep --pattern 'async getRemainingQuota($_, $_) { $$$ }'
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Search for getRemainingQuota method in the codebase
rg "getRemainingQuota" -A 5
Length of output: 27
🏁 Script executed:
#!/bin/bash
# View the current content of the permissions service file
cat apps/backend/src/services/auth/permissions/permissions.service.ts
Length of output: 5233
@rafgpereira You're absolutely right - I can now see that the getRemainingQuota method has been removed from the file. Thank you for the clarification!
There was a problem hiding this comment.
Hey @rafgpereira , please also update the GitHub Actions Build Workflow so it can be auto tested.
|
@nevo-david Can you please review this? |
Sure! I'll do it today. |
|
Or could you please make a new Workflow for this so it's easier for the PR Authors to see what went wrong? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/unit-test.yaml (2)
8-19: Consider expanding path filters for comprehensive coverage.The path filters should include test files and package-lock.json to ensure tests run when dependencies or test files change.
paths: - package.json + - package-lock.json - apps/** - '!apps/docs/**' - libraries/** + - '**/*.test.ts' + - '**/*.test.tsx' + - '**/*.spec.ts' + - '**/*.spec.tsx'
25-28: Consider enhancing the test matrix strategy.To improve test reliability and coverage across different Node.js environments:
- Add
fail-fast: falseto allow all matrix jobs to complete- Consider testing against multiple Node.js LTS versions
strategy: + fail-fast: false matrix: node-version: ['20.17.0'] + # Add more LTS versions for better compatibility testing + node-version: ['18.x', '20.x']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/unit-test.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run eslint scanning (frontend)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Run eslint scanning (backend)
- GitHub Check: build (20.17.0)
| restore-keys: | | ||
| ${{ runner.os }}-nextjs-${{ hashFiles('**/package-lock.json') }}- | ||
|
|
||
| - run: npm run test |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test execution and reporting.
The test execution step needs improvements:
- Add npm install step
- Configure test coverage reporting
- Add test results reporting
+ - run: npm ci
- run: npm run test
+ - name: Upload coverage reports
+ uses: codecov/codecov-action@v3
+ with:
+ token: ${{ secrets.CODECOV_TOKEN }}
+ - name: Test Report
+ uses: dorny/test-reporter@v1
+ if: success() || failure()
+ with:
+ name: Jest Tests
+ path: junit.xml
+ reporter: jest-junitAlso, update your package.json to generate the necessary reports:
{
"scripts": {
"test": "jest --coverage --ci --reporters=default --reporters=jest-junit"
}
}
|
Hmm, it doesnt seem to exit it. |
What kind of change does this PR introduce?
This PR introduces unit tests for the "check" method of the PermissionsService class, increasing test coverage and robustness of the authorization logic. Additionally, it resolves issue #595.
Why was this change needed?
The "check" method is essential for the authorization system, applying complex validation rules—such as channel limits, webhooks, posts per month and specific permissions (ADMIN, TEAM_MEMBERS, etc.)—which, until now, have not been comprehensively tested. The inclusion of these unit tests prevents regressions and ensures that future changes do not compromise the integrity of the system.
The tests were designed following the modified condition/decision coverage (MC/DC) standards, an agile approach that reduces the number of tests required (generally n+1 for n conditions) without compromising the robustness and high coverage of logical validation.
Jest has also been configured to run unit tests and generate coverage reports, which will facilitate maintenance and ongoing code quality. The library was already installed in the app, just scripts were added to make it functional.
Script implemented to enable the execution of tests:
Other information:
This change resolves issue #595, focusing exclusively on improving test coverage and authorization reliability. In the future, we plan to expand testing to other PermissionsService methods and related services. Then, a print of the test execution will be added to demonstrate the coverage achieved.
Checklist:
Summary by CodeRabbit