Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ The following table summarizes the environment variables used by the application
| `JWT_SECRET` | Secret key for signing/verifying internal JWTs (e.g., 2FA, one-time tokens). | `just-a-random-string` (example) |
| `LEGACY_BLOWFISH_KEY` | Base64 encoded Blowfish key for legacy password encryption/decryption. | `dGhpc2lzRGVmYXVmZlZhbHVl` (example) |

### Migrating legacy social login data

- Run `npx ts-node scripts/migrate-user-social-login.ts` to copy legacy `user_social_login` rows into `identity.user_social_login`.
- Set `SOURCE_IDENTITY_PG_URL` (legacy) and `IDENTITY_DB_URL` (target) before running; `USER_SOCIAL_LOGIN_BATCH_SIZE` tunes pagination.
- Flags available: `--dry-run` (log only), `--truncate` (clear target before load; ignored during dry-run), and `--insert-missing-only` (skip rows that already exist in the target).
- Ensure `identity.social_login_provider` is migrated first so foreign keys resolve during import.


**Downstream Usage**
--------------------
Expand Down
148 changes: 148 additions & 0 deletions scripts/migrate-user-social-login.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import { PrismaClient as TargetIdentityClient } from '@prisma/client';
import { PrismaClient as SourceIdentityClient } from '../legacy_migrate/generated/source-identity';

const DEFAULT_BATCH_SIZE = 1000;

const parseBoolFlag = (flag: string) => process.argv.includes(flag);

const requiredEnv = (key: string) => {
const value = process.env[key];
if (!value) {
throw new Error(`Missing required environment variable: ${key}`);
}
return value;
};

async function migrateUserSocialLogin() {
const dryRun = parseBoolFlag('--dry-run');
const truncate = parseBoolFlag('--truncate');
const batchSize = Number(process.env.USER_SOCIAL_LOGIN_BATCH_SIZE ?? DEFAULT_BATCH_SIZE);
const insertMissingOnly = parseBoolFlag('--insert-missing-only');

if (Number.isNaN(batchSize) || batchSize <= 0) {
throw new Error(`Invalid USER_SOCIAL_LOGIN_BATCH_SIZE: ${process.env.USER_SOCIAL_LOGIN_BATCH_SIZE}`);
}

if (insertMissingOnly && truncate) {
throw new Error('Cannot use --insert-missing-only together with --truncate');
}

const sourceDbUrl = requiredEnv('SOURCE_IDENTITY_PG_URL');
const targetDbUrl = requiredEnv('IDENTITY_DB_URL');

const sourceDb = new SourceIdentityClient({
datasources: {
db: { url: sourceDbUrl },
},
});

const targetDb = new TargetIdentityClient({
datasources: {
db: { url: targetDbUrl },
},
});

console.log(
`Starting user_social_login migration (dryRun=${dryRun}, truncate=${truncate}, insertMissingOnly=${insertMissingOnly}, batchSize=${batchSize})`,
);

try {
const totalRows = await sourceDb.user_social_login.count();
if (!totalRows) {
console.log('No rows found in source user_social_login. Nothing to migrate.');
return;
}

console.log(`Found ${totalRows} rows to migrate from source user_social_login`);

if (truncate) {
if (dryRun) {
console.log('[Dry Run] Would truncate target identity.user_social_login');
} else {
console.log('Truncating target identity.user_social_login before import...');
await targetDb.$executeRaw`TRUNCATE TABLE identity.user_social_login RESTART IDENTITY CASCADE`;
Copy link

Choose a reason for hiding this comment

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

[❗❗ security]
Using $executeRaw for SQL execution can be risky if any part of the query is constructed dynamically, as it may lead to SQL injection vulnerabilities. Ensure that the query is static or properly sanitized.

}
}

for (let offset = 0; offset < totalRows; offset += batchSize) {
const batch = await sourceDb.user_social_login.findMany({
orderBy: [{ user_id: 'asc' }, { social_login_provider_id: 'asc' }],
skip: offset,
take: batchSize,
});

if (!batch.length) {
break;
}

if (dryRun) {
console.log(`[Dry Run] Would migrate ${batch.length} rows (offset ${offset})`);
continue;
}

const recordsToInsert = batch.map((record) => ({
social_user_id: record.social_user_id ?? null,
user_id: Number(record.user_id),
social_login_provider_id: Number(record.social_login_provider_id),
social_user_name: record.social_user_name,
social_email: record.social_email ?? null,
social_email_verified: record.social_email_verified ?? null,
create_date: record.create_date ?? undefined,
modify_date: record.modify_date ?? undefined,
}));

if (insertMissingOnly) {
const existing = await targetDb.user_social_login.findMany({
where: {
OR: recordsToInsert.map((rec) => ({
user_id: rec.user_id,
social_login_provider_id: rec.social_login_provider_id,
})),
},
select: {
user_id: true,
social_login_provider_id: true,
},
});

const existingKey = new Set(existing.map((row) => `${row.user_id}-${row.social_login_provider_id}`));
const missing = recordsToInsert.filter(
(rec) => !existingKey.has(`${rec.user_id}-${rec.social_login_provider_id}`),
);

if (!missing.length) {
console.log(`Batch at offset ${offset} skipped (all ${recordsToInsert.length} rows already present)`);
continue;
}

await targetDb.user_social_login.createMany({
data: missing,
skipDuplicates: true,
});

console.log(
`Migrated ${Math.min(offset + batch.length, totalRows)} / ${totalRows} rows (inserted ${missing.length}, skipped ${
recordsToInsert.length - missing.length
})`,
);
continue;
}

await targetDb.user_social_login.createMany({
data: recordsToInsert,
skipDuplicates: true,
});

console.log(`Migrated ${Math.min(offset + batch.length, totalRows)} / ${totalRows} rows`);
}

console.log('user_social_login migration completed');
} finally {
await Promise.allSettled([sourceDb.$disconnect(), targetDb.$disconnect()]);
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider using Promise.all instead of Promise.allSettled if you want to ensure that both database connections are properly closed. Promise.allSettled will not throw an error if one of the promises is rejected, which might mask issues with disconnecting.

}
}

migrateUserSocialLogin().catch((error) => {
console.error('user_social_login migration failed:', error);
process.exit(1);
});
65 changes: 59 additions & 6 deletions src/api/user/user.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,13 @@ const mockEventService: jest.Mocked<Partial<EventService>> = {
postDirectBusMessage: jest.fn(),
};

const mockMemberPrisma: Partial<MemberPrismaService> = {
const memberUpdateMock = jest.fn();
const mockMemberPrisma: any = {
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Using any for mockMemberPrisma can lead to type safety issues. Consider using a more specific type or interface to ensure the mock object aligns with expected structure.

// Only the parts used by UserService need to be mocked
member: {
create: jest.fn(),
} as any,
update: memberUpdateMock,
},
};

const mockConfigService = {
Expand Down Expand Up @@ -1562,6 +1564,8 @@ describe('UserService', () => {
beforeEach(() => {
jest.clearAllMocks();

memberUpdateMock.mockResolvedValue(undefined);
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The test case should log an error if members.member update fails but continue does not verify the continuation of the process after logging the error. Consider adding assertions to ensure the process continues as expected.


// Mock checkEmailAvailabilityForUser
mockCheckEmail = jest
.spyOn(service, 'checkEmailAvailabilityForUser')
Expand Down Expand Up @@ -1652,6 +1656,10 @@ describe('UserService', () => {
{ userId: 1, handle: 'testuser' },
);
expect(result).toEqual(mockUser);
expect(memberUpdateMock).toHaveBeenCalledWith({
where: { userId },
data: { email: newEmail.toLowerCase() },
});
});

it('should throw BadRequestException for invalid user ID format', async () => {
Expand Down Expand Up @@ -1863,6 +1871,48 @@ describe('UserService', () => {
);
});

it('should log an error if members.member update fails but continue', async () => {
const txMock = {
user: {
findUnique: jest.fn().mockResolvedValue(mockUser),
update: jest.fn().mockResolvedValue(mockUser),
},
email: {
findFirst: jest
.fn()
.mockResolvedValueOnce(mockCurrentEmailRecord)
.mockResolvedValueOnce(null),
update: jest.fn().mockResolvedValue(mockUpdatedEmailRecord),
},
};
mockPrismaOltp.$transaction.mockImplementation(
<T>(callback): Promise<T> => {
const result = callback(txMock);
return result instanceof Promise ? result : Promise.resolve(result);
},
);

memberUpdateMock.mockRejectedValueOnce(
new Error('Member update failed'),
);

const result = await service.updatePrimaryEmail(
userIdString,
newEmail,
mockAuthUser,
);

expect(result).toEqual(mockUser);
expect(memberUpdateMock).toHaveBeenCalledWith({
where: { userId },
data: { email: newEmail.toLowerCase() },
});
expect(loggerErrorSpy).toHaveBeenCalledWith(
expect.stringContaining('Failed to update members.member email'),
expect.any(String),
);
});

it('should handle case when updated email record is not found after transaction', async () => {
// Set up transaction mock
const txMock = {
Expand Down Expand Up @@ -1965,6 +2015,7 @@ describe('UserService', () => {
const userIdString = '1';
const oldStatus = 'U';
const newStatus = 'A'; // Unverified to Active
const statusComment = 'Unit test comment';
Copy link

Choose a reason for hiding this comment

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

[💡 design]
The addition of statusComment in the updateStatus method calls should be verified for its intended use. Ensure that the comment is being utilized correctly within the method logic.

const mockExistingUser = createMockUserModel({
user_id: new Decimal(userId),
status: oldStatus,
Expand Down Expand Up @@ -1992,6 +2043,7 @@ describe('UserService', () => {
userIdString,
newStatus,
mockUser,
statusComment,
);

expect(prismaOltp.user.update).toHaveBeenCalledWith({
Expand Down Expand Up @@ -2023,7 +2075,7 @@ describe('UserService', () => {
prismaOltp.user.findUnique.mockResolvedValue(fromActiveUser);
prismaOltp.user.update.mockResolvedValue(toInactiveUser);

await service.updateStatus(userIdString, 'I', mockUser);
await service.updateStatus(userIdString, 'I', mockUser, statusComment);
expect(mockEventService.postEnvelopedNotification).toHaveBeenCalledWith(
'event.user.deactivated',
service.toCamelCase(toInactiveUser),
Expand All @@ -2037,17 +2089,17 @@ describe('UserService', () => {

it('should throw BadRequest for invalid user ID or status code', async () => {
await expect(
service.updateStatus('abc', newStatus, mockUser),
service.updateStatus('abc', newStatus, mockUser, statusComment),
).rejects.toThrow(BadRequestException);
await expect(
service.updateStatus(userIdString, 'X', mockUser),
service.updateStatus(userIdString, 'X', mockUser, statusComment),
).rejects.toThrow(BadRequestException);
});

it('should throw NotFoundException if user not found', async () => {
prismaOltp.user.findUnique.mockResolvedValue(null);
await expect(
service.updateStatus(userIdString, newStatus, mockUser),
service.updateStatus(userIdString, newStatus, mockUser, statusComment),
).rejects.toThrow(NotFoundException);
});

Expand All @@ -2056,6 +2108,7 @@ describe('UserService', () => {
userIdString,
oldStatus,
mockUser,
statusComment,
);
expect(prismaOltp.user.update).not.toHaveBeenCalled();
expect(result).toEqual(mockExistingUser);
Expand Down
31 changes: 24 additions & 7 deletions src/api/user/user.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1451,6 +1451,8 @@ export class UserService {
if (isNaN(userId)) {
throw new BadRequestException('Invalid user ID format.');
}
const normalizedEmail = newEmail.toLowerCase();
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider validating newEmail before converting it to lowercase to ensure it is a valid email format. This can prevent unnecessary operations if the email is invalid.

let emailChanged = false;

this.logger.log(
`Attempting to update primary email for user ID: ${userId} to ${newEmail} by admin ${authUser.userId}`,
Expand Down Expand Up @@ -1512,10 +1514,11 @@ export class UserService {
await tx.email.update({
where: { email_id: currentPrimaryEmailRecord.email_id },
data: {
address: newEmail.toLowerCase(),
address: normalizedEmail,
modify_date: new Date(),
},
});
emailChanged = true;

this.logger.log(
`Updated existing primary email record ${currentPrimaryEmailRecord.email_id.toNumber()} from ${oldEmail} to ${newEmail} for user ${userId}`,
Expand Down Expand Up @@ -1554,6 +1557,23 @@ export class UserService {
);
}

if (emailChanged) {
try {
await this.memberPrisma.member.update({
Copy link

Choose a reason for hiding this comment

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

[❗❗ maintainability]
The update operation on members.member should be part of the transaction to ensure consistency. If the transaction fails after updating the email in the email table but before updating members.member, the data could become inconsistent.

where: { userId },
data: { email: normalizedEmail },
});
this.logger.log(
`Updated members.member email to ${normalizedEmail} for user ${userId}`,
);
} catch (error) {
this.logger.error(
`Failed to update members.member email for user ${userId}: ${error.message}`,
error.stack,
);
}
}

return updatedUserInTx;
}

Expand Down Expand Up @@ -2172,8 +2192,7 @@ export class UserService {

// Send Welcome Email directly, matching legacy Java behavior
if (emailAddress && user?.handle) {
const domain =
this.configService.get<string>('APP_DOMAIN') || 'topcoder-dev.com';
const domain = CommonUtils.getAppDomain(this.configService);
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
Ensure that CommonUtils.getAppDomain(this.configService) returns a valid domain. If the configuration is incorrect, it could lead to malformed email addresses.

const fromEmail = `Topcoder <noreply@${domain}>`;
let welcomeTemplateId = this.configService.get<string>(
'SENDGRID_WELCOME_EMAIL_TEMPLATE_ID',
Expand Down Expand Up @@ -2289,8 +2308,7 @@ export class UserService {
email: string,
regSource?: string,
) {
const domain =
this.configService.get<string>('APP_DOMAIN') || 'topcoder-dev.com';
const domain = CommonUtils.getAppDomain(this.configService);
const fromEmail = `Topcoder <noreply@${domain}>`;
const sendGridTemplateId = this.configService.get<string>(
'SENDGRID_RESEND_ACTIVATION_EMAIL_TEMPLATE_ID',
Expand Down Expand Up @@ -2349,8 +2367,7 @@ export class UserService {
async resendActivationEmailEvent(userOtp: UserOtpDto, primaryEmail: string) {
try {
// For activation email (resend), use postDirectBusMessage to match legacy Java structure
const domain =
this.configService.get<string>('APP_DOMAIN') || 'topcoder-dev.com';
const domain = CommonUtils.getAppDomain(this.configService);
const fromEmail = `Topcoder <noreply@${domain}>`;
// Use the specific template ID for resending activation emails
const sendgridTemplateId = this.configService.get<string>(
Expand Down