Update verify approve conditions #1866
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the functionality and maintainability of project verification processes across multiple files. Key modifications include restructuring function parameters for clarity, improving error handling, and refining SQL logic. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Outside diff range and nitpick comments (1)
src/server/adminJs/tabs/projectVerificationTab.ts (1)
249-267: LGTM! Consider these improvements for the DRAFT status check.
The preliminary check for DRAFT status is a good addition. Consider these enhancements:
- Use i18n for the error message to maintain consistency with other messages in the codebase
- Optimize the query to use COUNT instead of fetching entire records
Here's how you could optimize the query:
- const draftProjects = await ProjectVerificationForm.createQueryBuilder(
- 'form',
- )
- .where('form.id IN (:...formIds)', { formIds })
- .andWhere('form.status = :status', {
- status: PROJECT_VERIFICATION_STATUSES.DRAFT,
- })
- .getMany();
-
- if (draftProjects.length > 0) {
+ const draftCount = await ProjectVerificationForm.createQueryBuilder('form')
+ .where('form.id IN (:...formIds)', { formIds })
+ .andWhere('form.status = :status', {
+ status: PROJECT_VERIFICATION_STATUSES.DRAFT,
+ })
+ .getCount();
+
+ if (draftCount > 0) {
throw new Error(
- `Cannot ${approved ? 'approve' : 'reject'} project${draftProjects.length > 1 ? 's' : ''} in DRAFT status.`,
+ i18n.__(translationErrorMessagesKeys.CANNOT_APPROVE_REJECT_DRAFT_PROJECTS, {
+ action: approved ? 'approve' : 'reject',
+ count: draftCount
+ }),
);
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/repositories/projectVerificationRepository.ts (1 hunks)
- src/server/adminJs/tabs/projectVerificationTab.ts (3 hunks)
- src/server/adminJs/tabs/projectsTab.ts (2 hunks)
🔇 Additional comments (3)
src/repositories/projectVerificationRepository.ts (2)
396-399: LGTM! Parameter destructuring improves readability.
The change from a single params object to destructured parameters makes the function signature clearer and more maintainable.
396-413: Verify consistency with project verification workflow.
Let's ensure these changes align with the broader verification process:
- The function should be used consistently with
approveProject - The verification status changes should match the expected workflow
✅ Verification successful
Based on the results, I can now generate the final response as we have enough information about the implementation and usage of both single and multiple project approval functions, as well as the verification status handling.
Changes to approveMultipleProjects are consistent with project verification workflow
The implementation is consistent with the broader verification process:
-
The function follows the same pattern as
approveProjectfor handling verification status:- Both reset
verificationStatusto null when approving - Both preserve
verificationStatuswhen not approving - Both update
isGivbackEligibleandverifiedfields similarly
- Both reset
-
The changes are used correctly in the admin interface (
projectVerificationTab.ts) where both single and bulk approval actions maintain consistent behavior. -
The verification status transitions are properly handled across the codebase, with clear status management in the verification workflow (reminder -> warning -> last chance -> up for revoking -> revoked).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage and consistency of project approval functions
# Test 1: Compare with single project approval implementation
echo "Comparing with single project approval implementation..."
rg -A 5 "approveProject|approveMultipleProjects" --type ts
# Test 2: Check verification status transitions
echo "Checking verification status transitions..."
ast-grep --pattern 'verificationStatus = $_' --type ts
Length of output: 7504
Script:
#!/bin/bash
# Let's verify the implementation consistency and verification status handling
# Check single project approval implementation
echo "Checking single project approval implementation..."
rg -B 5 -A 10 "export const approveProject" src/repositories/projectVerificationRepository.ts
# Check verification status field usage
echo "Checking verification status field usage..."
rg "verificationStatus" --type ts -B 2 -A 2
# Check Project entity definition
echo "Checking Project entity definition..."
rg "class Project" --type ts -A 20
Length of output: 128418
src/server/adminJs/tabs/projectVerificationTab.ts (1)
316-316: LGTM! Error message formatting improvement.
The addition of a colon improves the readability of the error message.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/repositories/projectVerificationRepository.ts (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/repositories/projectVerificationRepository.ts
🔇 Additional comments (1)
src/server/adminJs/tabs/projectsTab.ts (1)
248-267: LGTM: Error handling implementation is robust
The error handling implementation is well-structured:
- Proper error logging
- Consistent response format
- User-friendly error messages
- Maintains the same response structure for both success and error cases
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/server/adminJs/tabs/projectsTab.test.ts (2)
Line range hint 10-10: Consider standardizing sleep durations and handling race conditions.
The test file uses inconsistent sleep durations (10ms vs 50ms) for waiting on history creation. This could lead to flaky tests on slower systems.
Consider these improvements:
- await sleep(50);
+ const HISTORY_CREATION_WAIT_MS = 50;
+ await sleep(HISTORY_CREATION_WAIT_MS);
- await sleep(10);
+ await sleep(HISTORY_CREATION_WAIT_MS);Also, consider implementing a retry mechanism or polling for history creation instead of fixed delays:
async function waitForHistory(params: {
projectId: number;
userId: number;
maxAttempts = 5;
intervalMs = 10;
}): Promise<ProjectStatusHistory | null> {
for (let i = 0; i < params.maxAttempts; i++) {
const history = await findOneProjectStatusHistory({
projectId: params.projectId,
userId: params.userId,
});
if (history) return history;
await sleep(params.intervalMs);
}
return null;
}Also applies to: 50-50, 761-761, 824-824
Line range hint 425-824: Consider adding error case test coverage.
The test suite thoroughly covers the happy path scenarios but could benefit from additional test cases for error scenarios:
- Attempting to verify a non-existent project
- Handling database errors during verification
- Concurrent verification attempts
- Invalid verification status transitions
Consider adding test cases like:
it('should handle verification of non-existent project', async () => {
const nonExistentId = 999999;
const adminUser = await findUserById(SEED_DATA.ADMIN_USER.id);
const result = await verifyProjects(
{
currentAdmin: adminUser as User,
h: {},
resource: {},
records: [],
},
{
query: {
recordIds: String(nonExistentId),
},
},
true,
);
assert.equal(result.notice.type, 'danger');
assert.equal(result.notice.message, 'Project not found');
});📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/server/adminJs/tabs/projectsTab.test.ts (5 hunks)
🔇 Additional comments (2)
src/server/adminJs/tabs/projectsTab.test.ts (2)
Line range hint 425-493: LGTM! Well-structured test case for badge revocation.
The test case thoroughly validates that the verification status remains unchanged while the verification form transitions to draft state when a badge is revoked.
601-601: LGTM! Consistent test data setup.
The addition of isGivbackEligible: false property across test cases ensures consistent test data setup for verification scenarios.
Also applies to: 658-658, 729-729
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
src/server/adminJs/tabs/projectVerificationTab.test.ts (1)
22-151: Consider adding tests for edge cases and error scenarios.
The current test suite covers the main flows but misses important edge cases:
- Invalid/non-existent form ID
- Admin user without proper permissions
- Database/network error handling
Would you like me to help generate additional test cases for these scenarios?
package.json (1)
139-139: LGTM! Consider grouping admin tab test scripts.
The new test script follows the project's conventions and improves test coverage for the project verification functionality.
Consider grouping all admin tab test scripts under a single command for easier testing of related functionality. For example:
+"test:adminTabs": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/server/adminJs/tabs/**/*.test.ts",This would complement the existing test:adminJs script and provide more granular control over testing admin functionality.
src/server/adminJs/tabs/projectsTab.test.ts (3)
71-135: Consider using more reliable async handling.
The test cases are well structured, but using fixed sleep delays for async operations could lead to flaky tests. Consider using proper async/await patterns or event listeners instead.
- await sleep(50);
+ await Promise.all([
+ // Wait for specific async operations to complete
+ // e.g., database transactions, event emissions, etc.
+ ]);Line range hint 497-571: Consider splitting test cases for better maintainability.
The test case contains multiple assertions checking different aspects (verification status, form status, etc.). Consider splitting into smaller, focused test cases for better maintainability and easier debugging.
Example structure:
it('should maintain verification status when badge is revoked', async () => {
// Test only verification status
});
it('should set verification form as draft when badge is revoked', async () => {
// Test only form status
});674-674: Consider adding edge cases for givbacks eligibility.
While the basic cases are covered, consider adding test cases for edge scenarios:
- Projects transitioning between eligibility states
- Projects with pending givbacks
- Projects with historical givbacks but currently ineligible
Also applies to: 731-731, 802-802
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- package.json (1 hunks)
- src/server/adminJs/tabs/projectVerificationTab.test.ts (1 hunks)
- src/server/adminJs/tabs/projectsTab.test.ts (8 hunks)
🧰 Additional context used
🪛 GitHub Check: test
src/server/adminJs/tabs/projectVerificationTab.test.ts
[failure] 1-1:
Expected 2-3 arguments, but got 1.
| it('Should be able to reject Givback Eligibility Form for not draft form', async () => { | ||
| const project = await saveProjectDirectlyToDb({ | ||
| ...createProjectData(), | ||
| title: String(new Date().getTime()), | ||
| slug: String(new Date().getTime()), | ||
| verified: true, | ||
| listed: true, | ||
| isGivbackEligible: false, | ||
| }); | ||
|
|
||
| const projectVerificationForm = await createProjectVerificationForm({ | ||
| projectId: project.id, | ||
| userId: project.adminUserId, | ||
| }); | ||
| projectVerificationForm.status = PROJECT_VERIFICATION_STATUSES.SUBMITTED; | ||
| await projectVerificationForm.save(); | ||
|
|
||
| const adminUser = await findUserById(SEED_DATA.ADMIN_USER.id); | ||
|
|
||
| await approveVerificationForms( | ||
| { | ||
| currentAdmin: adminUser as User, | ||
| h: {}, | ||
| resource: {}, | ||
| records: [], | ||
| }, | ||
| { | ||
| query: { | ||
| recordIds: String(projectVerificationForm.id), | ||
| }, | ||
| }, | ||
| false, | ||
| ); | ||
|
|
||
| const updatedForm = await findProjectVerificationFormById( | ||
| projectVerificationForm.id, | ||
| ); | ||
| const updatedPorject = await findProjectById(project.id); | ||
|
|
||
| assert.isOk(updatedForm); | ||
| assert.equal(updatedForm?.status, PROJECT_VERIFICATION_STATUSES.REJECTED); | ||
| assert.isFalse(updatedPorject?.isGivbackEligible); | ||
| }); |
There was a problem hiding this comment.
Fix variable name typo and enhance rejection assertions.
The test has a typo in the variable name and could benefit from additional assertions for rejection details:
- const updatedPorject = await findProjectById(project.id);
+ const updatedProject = await findProjectById(project.id);
assert.isOk(updatedForm);
assert.equal(updatedForm?.status, PROJECT_VERIFICATION_STATUSES.REJECTED);
- assert.isFalse(updatedPorject?.isGivbackEligible);
+ assert.isFalse(updatedProject?.isGivbackEligible);
+ assert.equal(updatedForm?.adminUserId, adminUser.id, 'Should record the admin who rejected');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('Should be able to reject Givback Eligibility Form for not draft form', async () => { | |
| const project = await saveProjectDirectlyToDb({ | |
| ...createProjectData(), | |
| title: String(new Date().getTime()), | |
| slug: String(new Date().getTime()), | |
| verified: true, | |
| listed: true, | |
| isGivbackEligible: false, | |
| }); | |
| const projectVerificationForm = await createProjectVerificationForm({ | |
| projectId: project.id, | |
| userId: project.adminUserId, | |
| }); | |
| projectVerificationForm.status = PROJECT_VERIFICATION_STATUSES.SUBMITTED; | |
| await projectVerificationForm.save(); | |
| const adminUser = await findUserById(SEED_DATA.ADMIN_USER.id); | |
| await approveVerificationForms( | |
| { | |
| currentAdmin: adminUser as User, | |
| h: {}, | |
| resource: {}, | |
| records: [], | |
| }, | |
| { | |
| query: { | |
| recordIds: String(projectVerificationForm.id), | |
| }, | |
| }, | |
| false, | |
| ); | |
| const updatedForm = await findProjectVerificationFormById( | |
| projectVerificationForm.id, | |
| ); | |
| const updatedPorject = await findProjectById(project.id); | |
| assert.isOk(updatedForm); | |
| assert.equal(updatedForm?.status, PROJECT_VERIFICATION_STATUSES.REJECTED); | |
| assert.isFalse(updatedPorject?.isGivbackEligible); | |
| }); | |
| it('Should be able to reject Givback Eligibility Form for not draft form', async () => { | |
| const project = await saveProjectDirectlyToDb({ | |
| ...createProjectData(), | |
| title: String(new Date().getTime()), | |
| slug: String(new Date().getTime()), | |
| verified: true, | |
| listed: true, | |
| isGivbackEligible: false, | |
| }); | |
| const projectVerificationForm = await createProjectVerificationForm({ | |
| projectId: project.id, | |
| userId: project.adminUserId, | |
| }); | |
| projectVerificationForm.status = PROJECT_VERIFICATION_STATUSES.SUBMITTED; | |
| await projectVerificationForm.save(); | |
| const adminUser = await findUserById(SEED_DATA.ADMIN_USER.id); | |
| await approveVerificationForms( | |
| { | |
| currentAdmin: adminUser as User, | |
| h: {}, | |
| resource: {}, | |
| records: [], | |
| }, | |
| { | |
| query: { | |
| recordIds: String(projectVerificationForm.id), | |
| }, | |
| }, | |
| false, | |
| ); | |
| const updatedForm = await findProjectVerificationFormById( | |
| projectVerificationForm.id, | |
| ); | |
| const updatedProject = await findProjectById(project.id); | |
| assert.isOk(updatedForm); | |
| assert.equal(updatedForm?.status, PROJECT_VERIFICATION_STATUSES.REJECTED); | |
| assert.isFalse(updatedProject?.isGivbackEligible); | |
| assert.equal(updatedForm?.adminUserId, adminUser.id, 'Should record the admin who rejected'); | |
| }); |
| it('Should throw error if Givback Eligibility Form is on draft', async () => { | ||
| const project = await saveProjectDirectlyToDb({ | ||
| ...createProjectData(), | ||
| title: String(new Date().getTime()), | ||
| slug: String(new Date().getTime()), | ||
| verified: true, | ||
| listed: true, | ||
| }); | ||
|
|
||
| const projectVerificationForm = await createProjectVerificationForm({ | ||
| projectId: project.id, | ||
| userId: project.adminUserId, | ||
| }); | ||
|
|
||
| projectVerificationForm.status = PROJECT_VERIFICATION_STATUSES.DRAFT; | ||
| await projectVerificationForm.save(); | ||
|
|
||
| const adminUser = await findUserById(SEED_DATA.ADMIN_USER.id); | ||
|
|
||
| await approveVerificationForms( | ||
| { | ||
| currentAdmin: adminUser as User, | ||
| h: {}, | ||
| resource: {}, | ||
| records: [], | ||
| }, | ||
| { | ||
| query: { | ||
| recordIds: String(projectVerificationForm.id), | ||
| }, | ||
| }, | ||
| true, | ||
| ); | ||
|
|
||
| const updatedForm = await findProjectVerificationFormById( | ||
| projectVerificationForm.id, | ||
| ); | ||
| assert.isOk(updatedForm); | ||
| assert.equal(updatedForm?.status, PROJECT_VERIFICATION_STATUSES.DRAFT); | ||
| }); |
There was a problem hiding this comment.
Test description doesn't match implementation.
The test description states it "Should throw error" but there's no assertion to verify that an error was thrown. Consider using chai's expect().to.throw():
- await approveVerificationForms(
+ await expect(approveVerificationForms(
{
currentAdmin: adminUser as User,
h: {},
resource: {},
records: [],
},
{
query: {
recordIds: String(projectVerificationForm.id),
},
},
true,
- );
+ )).to.throw('Cannot approve/reject draft forms');Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor