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.

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

add cluster matching sync timestamp and logs #1913

Merged
merged 2 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
25 changes: 25 additions & 0 deletions migration/1737544105947-addClusterMatchingSyncAtTimestamp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { MigrationInterface, QueryRunner, TableColumn } from 'typeorm';

export class addClusterMatchingSyncAtTimestamp1737544105947
implements MigrationInterface
{
public async up(queryRunner: QueryRunner): Promise<void> {
const table = await queryRunner.getTable('qf_round');
const columnExists = table?.findColumnByName('clusterMatchingSyncAt');

if (!columnExists) {
await queryRunner.addColumn(
'qf_round',
new TableColumn({
name: 'clusterMatchingSyncAt',
type: 'timestamp',
isNullable: true,
}),
);
}
}

public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.dropColumn('qf_round', 'clusterMatchingSyncAt');
}
}
4 changes: 4 additions & 0 deletions src/entities/qfRound.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ export class QfRound extends BaseEntity {
@Column({ default: false })
isDataAnalysisDone: boolean;

@Field(_type => Date)
@Column({ nullable: true })
clusterMatchingSyncAt: Date;
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

Align GraphQL and database column nullability.

The field is nullable in the database but required in GraphQL. This mismatch could cause runtime errors when the field is null.

Update the field definition to match nullability:

-  @Field(_type => Date)
+  @Field(_type => Date, { nullable: true })
   @Column({ nullable: true })
   clusterMatchingSyncAt: Date;
📝 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
@Field(_type => Date)
@Column({ nullable: true })
clusterMatchingSyncAt: Date;
@Field(_type => Date, { nullable: true })
@Column({ nullable: true })
clusterMatchingSyncAt: Date;


@UpdateDateColumn()
updatedAt: Date;

Expand Down
4 changes: 4 additions & 0 deletions src/services/cronJobs/syncEstimatedClusterMatchingJob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ export const fetchAndUpdateClusterEstimatedMatching = async () => {
activeQfRound.id,
matchingData,
);

// Update latest job ran succesfully
activeQfRound.clusterMatchingSyncAt = new Date();
await activeQfRound.save();
} catch (e) {
logger.error('fetchAndUpdateClusterEstimatedMatching error', e);
} finally {
Expand Down
12 changes: 10 additions & 2 deletions src/workers/cocm/estimatedClusterMatchingWorker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,20 @@ export type EstimatedClusterMatchingWorker =

const worker: EstimatedClusterMatchingWorker = {
async fetchEstimatedClusterMatching(matchingDataInput: any) {
return await getClusterMatchingAdapter().fetchEstimatedClusterMatchings(
matchingDataInput,
logger.debug('fetchEstimatedClusterMatching() has been called');
const matchingData =
await getClusterMatchingAdapter().fetchEstimatedClusterMatchings(
matchingDataInput,
);
logger.debug(
'fetchEstimatedClusterMatching() has worked with params',
String(matchingData),
);
return matchingData;
Comment on lines +17 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve logging security for matchingData.

Converting matchingData directly to string could expose sensitive information in logs. Consider logging only necessary metadata.

Suggested improvement:

-    logger.debug(
-      'fetchEstimatedClusterMatching() has worked with params',
-      String(matchingData),
-    );
+    logger.debug(
+      'fetchEstimatedClusterMatching() completed',
+      {
+        recordCount: matchingData?.length || 0,
+        timestamp: new Date().toISOString()
+      }
+    );
📝 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
logger.debug('fetchEstimatedClusterMatching() has been called');
const matchingData =
await getClusterMatchingAdapter().fetchEstimatedClusterMatchings(
matchingDataInput,
);
logger.debug(
'fetchEstimatedClusterMatching() has worked with params',
String(matchingData),
);
return matchingData;
logger.debug('fetchEstimatedClusterMatching() has been called');
const matchingData =
await getClusterMatchingAdapter().fetchEstimatedClusterMatchings(
matchingDataInput,
);
logger.debug(
'fetchEstimatedClusterMatching() completed',
{
recordCount: matchingData?.length || 0,
timestamp: new Date().toISOString()
}
);
return matchingData;

},

async updateEstimatedClusterMatching(qfRoundId: number, matchingData: any) {
logger.debug('updateEstimatedClusterMatching() has been called');
try {
const params: any[] = [];
const values = matchingData
Expand Down
Loading