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.

Sign up for GitHub

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

Ensure correct emails are sent for project status changes related to vouching #1853

Merged
merged 4 commits into from
Oct 3, 2024
Merged

Ensure correct emails are sent for project status changes related to vouching #1853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cfdfc5
Ensure correct emails are sent for project status changes related to …
RamRamez Oct 2, 2024
e8a0aaa
fix test
RamRamez Oct 2, 2024
bd7c70e
fix test cases
RamRamez Oct 2, 2024
a4ae088
fix test cases
RamRamez Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@
"test:qfRoundHistoryRepository": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/repositories/qfRoundHistoryRepository.test.ts",
"test:qfRoundService": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/services/qfRoundService.test.ts",
"test:project": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/entities/project.test.ts",
"test:projectsTab": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/server/adminJs/tabs/projectsTab.test.ts",
"test:syncUsersModelScore": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/services/cronJobs/syncUsersModelScore.test.ts",
"test:notifyDonationsWithSegment": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/services/cronJobs/notifyDonationsWithSegment.test.ts",
"test:checkProjectVerificationStatus": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/services/cronJobs/checkProjectVerificationStatus.test.ts",
Expand Down
2 changes: 1 addition & 1 deletion src/repositories/projectRepository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ export const projectsWithoutUpdateAfterTimeFrame = async (
'project.title',
])
.where('project.isImported = false')
.andWhere('project.verified = true')
.andWhere('project.isGivbackEligible = true')
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential Issue Detected with Project Eligibility Filtering

The replacement of project.verified = true with project.isGivbackEligible = true drastically reduces the number of eligible projects from multiple instances to just one. This significant change may inadvertently exclude many projects that were previously considered verified, potentially impacting email notifications and other dependent functionalities.

🔗 Analysis chain

LGTM. Verify the impact on project selection.

The change from project.verified = true to project.isGivbackEligible = true aligns with the PR objective of ensuring correct emails for project status changes related to vouching. This modification shifts the focus from verified projects to Givback eligible projects, which is likely the intended behavior.

To ensure this change doesn't unexpectedly alter the set of projects being processed, please run the following verification:

This script will help us understand the potential impact of this change on the number of projects being processed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare the number of projects affected by this change

# Check the number of verified projects
echo "Number of verified projects:"
rg --type typescript 'project\.verified\s*=\s*true' -c

# Check the number of Givback eligible projects
echo "Number of Givback eligible projects:"
rg --type typescript 'project\.isGivbackEligible\s*=\s*true' -c

# Check for any other usages of isGivbackEligible
echo "Other usages of isGivbackEligible:"
rg --type typescript 'isGivbackEligible' -C 2

Length of output: 502

Script:

#!/bin/bash
# Description: Compare the number of projects affected by this change

# Check the number of verified projects
echo "Number of verified projects:"
rg 'project\.verified\s*=\s*true' -c -g "*.ts" -g "*.tsx"

# Check the number of Givback eligible projects
echo "Number of Givback eligible projects:"
rg 'project\.isGivbackEligible\s*=\s*true' -c -g "*.ts" -g "*.tsx"

# Check for any other usages of isGivbackEligible
echo "Other usages of isGivbackEligible:"
rg 'isGivbackEligible' -C 2 -g "*.ts" -g "*.tsx"

Length of output: 70881

.andWhere(
'(project.verificationStatus NOT IN (:...statuses) OR project.verificationStatus IS NULL)',
{
Expand Down
39 changes: 30 additions & 9 deletions src/server/adminJs/tabs/projectsTab.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
addFeaturedProjectUpdate,
exportProjectsWithFiltersToCsv,
listDelist,
revokeGivbacksEligibility,
updateStatusOfProjects,
verifyProjects,
} from './projectsTab';
Expand Down Expand Up @@ -452,8 +453,20 @@ function verifyProjectsTestCases() {
recordIds: String(project.id),
},
},
true, // give priority to revoke badge
true, // revoke badge
false,
);
await revokeGivbacksEligibility(
{
currentAdmin: adminUser as User,
h: {},
resource: {},
records: [],
},
{
query: {
recordIds: String(project.id),
},
},
);

const updatedProject = await findProjectById(project.id);
Expand Down Expand Up @@ -527,15 +540,20 @@ function verifyProjectsTestCases() {
assert.isTrue(updatedProject?.listed);
assert.equal(updatedProject?.reviewStatus, ReviewStatus.Listed);
assert.isTrue(project!.verificationStatus === RevokeSteps.Revoked);
assert.isTrue(updatedProject!.verificationStatus === null);
assert.isTrue(
updatedProject!.verificationStatus === project.verificationStatus,
);
Comment on lines +543 to +545
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Assertion May Not Reflect Updated Verification Status

The assertion checks if the updated project's verificationStatus remains the same as before:

assert.isTrue(
  updatedProject!.verificationStatus === project.verificationStatus,
);

Since the project's verification status might change after calling verifyProjects, consider verifying against the expected new status to ensure the test accurately reflects the intended behavior.

assert.equal(
updatedVerificationForm!.status,
PROJECT_VERIFICATION_STATUSES.VERIFIED,
PROJECT_VERIFICATION_STATUSES.DRAFT,
);
Comment on lines +548 to +549
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential Mismatch in Verification Form Status After Verification

You assert that the updatedVerificationForm status is DRAFT after verifying the project:

assert.equal(
  updatedVerificationForm!.status,
  PROJECT_VERIFICATION_STATUSES.DRAFT,
);

However, after verification, shouldn't the status be VERIFIED? Confirm that DRAFT is the intended status in this context.

assert.equal(
updatedVerificationForm!.isTermAndConditionsAccepted,
projectVerificationForm.isTermAndConditionsAccepted,
);
assert.equal(updatedVerificationForm!.isTermAndConditionsAccepted, true);
assert.equal(
updatedVerificationForm!.lastStep,
PROJECT_VERIFICATION_STEPS.SUBMIT,
projectVerificationForm.lastStep,
);
});

Expand Down Expand Up @@ -616,12 +634,15 @@ function verifyProjectsTestCases() {
assert.isTrue(updatedProject!.verificationStatus === RevokeSteps.Revoked);
assert.equal(
updatedVerificationForm!.status,
PROJECT_VERIFICATION_STATUSES.DRAFT,
PROJECT_VERIFICATION_STATUSES.VERIFIED,
);
Comment on lines +637 to +638
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Verification Form Status Should Reflect Unverification

In the test case where a project is unverified, the assertion is:

assert.equal(
  updatedVerificationForm!.status,
  PROJECT_VERIFICATION_STATUSES.VERIFIED,
);

Since the project is unverified, the verification form status might need to be updated to DRAFT or another appropriate status to reflect this change.

assert.equal(
updatedVerificationForm!.isTermAndConditionsAccepted,
projectVerificationForm.isTermAndConditionsAccepted,
);
assert.equal(updatedVerificationForm!.isTermAndConditionsAccepted, false);
assert.equal(
updatedVerificationForm!.lastStep,
PROJECT_VERIFICATION_STEPS.MANAGING_FUNDS,
projectVerificationForm.lastStep,
);
});

Expand Down
120 changes: 65 additions & 55 deletions src/server/adminJs/tabs/projectsTab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,29 +184,72 @@ export const addFeaturedProjectUpdate = async (
};
};

export const revokeGivbacksEligibility = async (
context: AdminJsContextInterface,
request: AdminJsRequestInterface,
) => {
const { records, currentAdmin } = context;
try {
const projectIds = request?.query?.recordIds
?.split(',')
?.map(strId => Number(strId)) as number[];
const updateParams = { isGivbackEligible: false };
const projects = await Project.createQueryBuilder('project')
.update<Project>(Project, updateParams)
.where('project.id IN (:...ids)')
.setParameter('ids', projectIds)
.returning('*')
.updateEntity(true)
.execute();

for (const project of projects.raw) {
const projectWithAdmin = (await findProjectById(project.id)) as Project;
projectWithAdmin.verificationStatus = RevokeSteps.Revoked;
await projectWithAdmin.save();
await getNotificationAdapter().projectBadgeRevoked({
project: projectWithAdmin,
});
const verificationForm = await getVerificationFormByProjectId(project.id);
if (verificationForm) {
await makeFormDraft({
formId: verificationForm.id,
adminId: currentAdmin.id,
});
}
}
await Promise.all([
refreshUserProjectPowerView(),
refreshProjectPowerView(),
refreshProjectFuturePowerView(),
]);
} catch (error) {
logger.error('revokeGivbacksEligibility() error', error);
throw error;
}
return {
redirectUrl: '/admin/resources/Project',
records: records.map(record => {
record.toJSON(context.currentAdmin);
}),
Comment on lines +231 to +233
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix mapping function to return values in 'records.map'

In lines 231-233, the mapping function does not return any values, resulting in an array of undefined elements. To fix this, ensure that the callback returns the result of record.toJSON(context.currentAdmin);.

Apply this diff to correct the issue:

records: records.map(record => {
-  record.toJSON(context.currentAdmin);
+  return record.toJSON(context.currentAdmin);
}),

Alternatively, simplify the code:

- records: records.map(record => {
-   record.toJSON(context.currentAdmin);
- }),
+ records: records.map(record => record.toJSON(context.currentAdmin)),
📝 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.

Suggested change
records: records.map(record => {
record.toJSON(context.currentAdmin);
}),
records: records.map(record => record.toJSON(context.currentAdmin)),

notice: {
message: 'Project(s) successfully revoked from Givbacks eligibility',
type: 'success',
},
};
};

export const verifyProjects = async (
context: AdminJsContextInterface,
request: AdminJsRequestInterface,
verified: boolean = true,
revokeBadge: boolean = false,
vouchedStatus: boolean = true,
) => {
const { records, currentAdmin } = context;
// prioritize revokeBadge
const verificationStatus = revokeBadge ? false : verified;
try {
const projectIds = request?.query?.recordIds
?.split(',')
?.map(strId => Number(strId)) as number[];
const projectsBeforeUpdating = await findProjectsByIdArray(projectIds);
const updateParams = { verified: verificationStatus };

if (verificationStatus) {
await Project.query(`
UPDATE project
SET "verificationStatus" = NULL
WHERE id IN (${request?.query?.recordIds})
`);
}
const updateParams = { verified: vouchedStatus };

const projects = await Project.createQueryBuilder('project')
.update<Project>(Project, updateParams)
Expand All @@ -219,11 +262,11 @@ export const verifyProjects = async (
for (const project of projects.raw) {
if (
projectsBeforeUpdating.find(p => p.id === project.id)?.verified ===
verificationStatus
vouchedStatus
) {
logger.debug('verifying/unVerifying project but no changes happened', {
projectId: project.id,
verificationStatus,
verificationStatus: vouchedStatus,
});
// if project.verified have not changed, so we should not execute rest of the codes
continue;
Expand All @@ -232,42 +275,10 @@ export const verifyProjects = async (
project,
status: project.status,
userId: currentAdmin.id,
description: verified
description: vouchedStatus
? HISTORY_DESCRIPTIONS.CHANGED_TO_VERIFIED
: HISTORY_DESCRIPTIONS.CHANGED_TO_UNVERIFIED,
});
const projectWithAdmin = (await findProjectById(project.id)) as Project;

if (revokeBadge) {
projectWithAdmin.verificationStatus = RevokeSteps.Revoked;
await projectWithAdmin.save();
await getNotificationAdapter().projectBadgeRevoked({
project: projectWithAdmin,
});
} else if (verificationStatus) {
await getNotificationAdapter().projectVerified({
project: projectWithAdmin,
});
} else {
await getNotificationAdapter().projectUnVerified({
project: projectWithAdmin,
});
}

const verificationForm = await getVerificationFormByProjectId(project.id);
if (verificationForm) {
if (verificationStatus) {
await makeFormVerified({
formId: verificationForm.id,
adminId: currentAdmin.id,
});
} else {
await makeFormDraft({
formId: verificationForm.id,
adminId: currentAdmin.id,
});
}
}
}

await Promise.all([
Expand All @@ -286,7 +297,7 @@ export const verifyProjects = async (
}),
notice: {
message: `Project(s) successfully ${
verificationStatus ? 'verified' : 'unverified'
vouchedStatus ? 'vouched' : 'unvouched'
}`,
type: 'success',
},
Expand Down Expand Up @@ -1273,7 +1284,7 @@ export const projectsTab = {
},
component: false,
},
verify: {
approveVouched: {
actionType: 'bulk',
isVisible: true,
isAccessible: ({ currentAdmin }) =>
Expand All @@ -1286,7 +1297,7 @@ export const projectsTab = {
},
component: false,
},
reject: {
removeVouched: {
actionType: 'bulk',
isVisible: true,
isAccessible: ({ currentAdmin }) =>
Expand All @@ -1299,17 +1310,16 @@ export const projectsTab = {
},
component: false,
},
// the difference is that it sends another segment event
revokeBadge: {
revokeGivbacksEligible: {
actionType: 'bulk',
isVisible: true,
isAccessible: ({ currentAdmin }) =>
canAccessProjectAction(
{ currentAdmin },
ResourceActions.REVOKE_BADGE,
),
handler: async (request, response, context) => {
return verifyProjects(context, request, false, true);
handler: async (request, _response, context) => {
return revokeGivbacksEligibility(context, request);
},
component: false,
},
Expand Down
16 changes: 8 additions & 8 deletions src/services/cronJobs/checkProjectVerificationStatus.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function checkProjectVerificationStatusTestCases() {
...createProjectData(),
title: String(new Date().getTime()),
slug: String(new Date().getTime()),
verified: true,
isGivbackEligible: true,
latestUpdateCreationDate: moment()
.subtract(46, 'days')
.endOf('day')
Expand All @@ -36,7 +36,7 @@ function checkProjectVerificationStatusTestCases() {

const warnableProjectUpdate = await findProjectById(warnableProject.id);

assert.isTrue(warnableProjectUpdate!.verified);
assert.isTrue(warnableProjectUpdate!.isGivbackEligible);
assert.equal(
warnableProjectUpdate!.verificationStatus,
RevokeSteps.Warning,
Expand All @@ -47,7 +47,7 @@ function checkProjectVerificationStatusTestCases() {
...createProjectData(),
title: String(new Date().getTime()),
slug: String(new Date().getTime()),
verified: true,
isGivbackEligible: true,
latestUpdateCreationDate: moment().subtract(91, 'days').endOf('day'),
verificationStatus: RevokeSteps.Warning,
});
Expand All @@ -56,7 +56,7 @@ function checkProjectVerificationStatusTestCases() {

const warnableProjectUpdate = await findProjectById(warnableProject.id);

assert.isTrue(warnableProjectUpdate!.verified);
assert.isTrue(warnableProjectUpdate!.isGivbackEligible);
assert.equal(
warnableProjectUpdate!.verificationStatus,
RevokeSteps.LastChance,
Expand All @@ -67,7 +67,7 @@ function checkProjectVerificationStatusTestCases() {
...createProjectData(),
title: String(new Date().getTime()),
slug: String(new Date().getTime()),
verified: true,
isGivbackEligible: true,
latestUpdateCreationDate: moment().subtract(105, 'days').endOf('day'),
verificationStatus: RevokeSteps.LastChance,
});
Expand All @@ -78,7 +78,7 @@ function checkProjectVerificationStatusTestCases() {
lastWarningProject.id,
);

assert.isTrue(lastWarningProjectUpdated!.verified);
assert.isTrue(lastWarningProjectUpdated!.isGivbackEligible);
assert.equal(
lastWarningProjectUpdated!.verificationStatus,
RevokeSteps.UpForRevoking,
Expand All @@ -89,7 +89,7 @@ function checkProjectVerificationStatusTestCases() {
...createProjectData(),
title: String(new Date().getTime()),
slug: String(new Date().getTime()),
verified: true,
isGivbackEligible: true,
latestUpdateCreationDate: moment().subtract(105, 'days').endOf('day'),
isImported: true,
});
Expand All @@ -98,7 +98,7 @@ function checkProjectVerificationStatusTestCases() {

const importedProjectUpdated = await findProjectById(importedProject.id);

assert.isTrue(importedProjectUpdated!.verified);
assert.isTrue(importedProjectUpdated!.isGivbackEligible);
assert.equal(importedProjectUpdated!.verificationStatus, null);
});
// it('should revoke project verification after last chance time frame expired', async () => {
Expand Down
Loading