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 not handled properly, as it may lead to SQL injection vulnerabilities. Ensure that the input is sanitized and that this operation is safe in the context it is used.

}
}

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 and handle any errors that might occur during disconnection. Promise.allSettled will suppress errors, which might not be desirable in this context.

}
}

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 defining a more specific type to ensure that only the necessary properties are mocked and to catch potential errors at compile time.

// 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]
Resetting the mock implementation of memberUpdateMock to resolve undefined in every beforeEach might hide potential issues if the mock is expected to return a specific value. Ensure that the mock's behavior aligns with the test's expectations.


// 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({
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 checks if memberUpdateMock is called with a specific object. Ensure that the userId variable is defined and correctly scoped within the test to avoid potential reference errors.

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 () => {
Copy link

Choose a reason for hiding this comment

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

[⚠️ design]
The test logs an error if members.member update fails but continues execution. Consider whether this behavior is appropriate, as it might lead to inconsistent states if the update is critical. Evaluate if the test should assert on the final state or outcome.

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.

[❗❗ correctness]
The updateStatus method now includes a statusComment parameter. Ensure that all usages of this method, including in production code, handle this new parameter correctly to avoid unexpected behavior.

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 could prevent potential issues with invalid email addresses being processed.

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) {
Copy link

Choose a reason for hiding this comment

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

[⚠️ correctness]
The emailChanged flag is used to determine if the email update should be propagated to the members.member table. Ensure that this flag is correctly set only when the email update is successful to avoid inconsistent states.

try {
await this.memberPrisma.member.update({
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.

[💡 maintainability]
The change to use CommonUtils.getAppDomain(this.configService) improves maintainability by centralizing domain retrieval logic. Ensure that CommonUtils.getAppDomain handles all edge cases, such as missing or malformed configuration values.

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