Skip to content

feat: opportunity rematch#3540

Open
capJavert wants to merge 5 commits intomainfrom
opportunity-rematch
Open

feat: opportunity rematch#3540
capJavert wants to merge 5 commits intomainfrom
opportunity-rematch

Conversation

@capJavert
Copy link
Contributor

@capJavert capJavert self-assigned this Feb 3, 2026
@capJavert capJavert requested a review from a team as a code owner February 3, 2026 19:48
@capJavert
Copy link
Contributor Author

@claude review

@claude
Copy link

claude bot commented Feb 3, 2026

Claude finished @capJavert's task in 1m 41s —— View job


Code Review Complete ✓

Todo List

  • Read and understand all changed files
  • Review migration structure and formatting
  • Review OpportunityMatch entity changes
  • Review worker implementations
  • Review notification logic
  • Review test coverage
  • Check adherence to CLAUDE.md guidelines
  • Provide comprehensive feedback

Summary

This PR implements an "opportunity rematch" feature that allows candidates who previously rejected a job opportunity to be re-matched when the job requirements change. The implementation adds history tracking to OpportunityMatch entities, introduces a new notification worker, and includes comprehensive test coverage.

Overall Assessment: The implementation is solid with good test coverage and proper use of transactions. I found a few issues that should be addressed:

Issues Found

1. 🔴 Missing Notification Settings for ReMatchedOpportunity

File: src/notifications/common.ts:77

The new NotificationType.ReMatchedOpportunity notification type is defined but not included in DEFAULT_NOTIFICATION_SETTINGS. This means users won't have default notification preferences for this notification type.

Fix: Add the notification type to DEFAULT_NOTIFICATION_SETTINGS around line 275:

[NotificationType.NewOpportunityMatch]: {
  email: NotificationPreferenceStatus.Subscribed,
  inApp: NotificationPreferenceStatus.Subscribed,
},
[NotificationType.ReMatchedOpportunity]: {
  email: NotificationPreferenceStatus.Subscribed,
  inApp: NotificationPreferenceStatus.Subscribed,
},

Fix this →


2. 🟡 TODO Comment for Email Template ID

File: src/workers/newNotificationV2Mail.ts:126

rematched_opportunity: '', // TODO opportunity-rematch

The email template ID is empty with a TODO comment. This should be tracked to ensure the email template is implemented.

Action: Ensure this is tracked in your project management system (ENG-579) and will be completed before release.


3. 🟡 Redundant User Query in reMatchedOpportunityNotification

File: src/workers/notifications/reMatchedOpportunityNotification.ts:33-37

const user = await con.getRepository(User).findOneBy({ id: userId });

if (!user) {
  return;
}

This user query is unnecessary because:

  1. The notification system will handle missing users downstream
  2. You're not using any user properties in this worker
  3. It adds an extra database query

Recommendation: Remove this user check to simplify the code and reduce database queries.


4. 🟢 Consider Adding Migration to /format-migration Skill

File: src/migration/1770145297391-OpportunityMatchHistory.ts

The migration is well-formed and follows best practices. However, according to CLAUDE.md:

After generating a migration, use the /format-migration skill to format the SQL code for readability and consistency.

Recommendation: Run /format-migration on this migration to ensure consistent formatting, even though the current formatting looks acceptable.


Positive Observations ✅

  1. Excellent Transaction Usage: The storeCandidateOpportunityMatch worker correctly wraps all database operations in a transaction (src/workers/opportunity/storeCandidateOpportunityMatch.ts:45-118), ensuring atomicity.

  2. Proper JSONB Update Pattern: The history update correctly uses query builder with setParameter for nested JSONB values (src/workers/opportunity/storeCandidateOpportunityMatch.ts:71-83), following the best practice documented in AGENTS.md.

  3. Comprehensive Test Coverage: Both new workers have excellent test coverage with edge cases:

    • Tests for missing fields
    • Tests for re-match vs new match scenarios
    • Tests for alert updates
    • Tests for history archiving
  4. Good Worker Separation: The separation between candidateOpportunityMatchNotification (new matches) and reMatchedOpportunityNotification (re-matches) is clean and follows single responsibility principle.

  5. Documentation Updates: The AGENTS.md updates add valuable guidance about type declarations, comments, and JSONB patterns.

  6. Proper Select Statement: The workers correctly use explicit .select() to fetch only needed fields (src/workers/notifications/reMatchedOpportunityNotification.ts:25).


Minor Observations

  1. Test Consistency: The test file updates for candidateOpportunityMatchNotification properly add fixtures for Organization, Opportunity, and DatasetLocation to ensure proper foreign key relationships.

  2. Infrastructure Setup: The new worker is properly registered in .infra/common.ts with the correct topic and subscription.

  3. Notification Generation: The notification title and generation logic in src/notifications/generate.ts is well-implemented and consistent with existing patterns.


Recommendations

Before merging:

  1. 🔴 Add ReMatchedOpportunity to DEFAULT_NOTIFICATION_SETTINGS (critical)
  2. 🟡 Remove redundant user query in reMatchedOpportunityNotification (nice-to-have)
  3. 🟡 Track the email template TODO (ENG-579)

Optional improvements:

  • Consider running /format-migration on the migration file for consistency

Files Reviewed

  • .infra/common.ts - Worker registration
  • AGENTS.md - Documentation updates
  • __tests__/workers/notifications/candidateOpportunityMatchNotification.ts - Test updates
  • __tests__/workers/notifications/reMatchedOpportunityNotification.ts - New tests
  • __tests__/workers/opportunity/storeCandidateOpportunityMatch.ts - Test updates
  • src/entity/OpportunityMatch.ts - Entity changes
  • src/migration/1770145297391-OpportunityMatchHistory.ts - Migration
  • src/notifications/common.ts - Notification type enum
  • src/notifications/generate.ts - Notification generation
  • src/workers/newNotificationV2Mail.ts - Email templates
  • src/workers/notifications/candidateOpportunityMatchNotification.ts - Worker updates
  • src/workers/notifications/index.ts - Worker exports
  • src/workers/notifications/reMatchedOpportunityNotification.ts - New worker
  • src/workers/opportunity/storeCandidateOpportunityMatch.ts - Worker updates

@pulumi
Copy link

pulumi bot commented Feb 3, 2026

🍹 The Update (preview) for dailydotdev/api/prod (at cf55cc7) was successful.

Resource Changes

    Name                                                       Type                                  Operation
~   vpc-native-update-tags-str-cron                            kubernetes:batch/v1:CronJob           update
~   vpc-native-validate-active-users-cron                      kubernetes:batch/v1:CronJob           update
~   vpc-native-user-profile-analytics-clickhouse-cron          kubernetes:batch/v1:CronJob           update
+   vpc-native-api-db-migration-da7036d5                       kubernetes:batch/v1:Job               create
~   vpc-native-clean-zombie-images-cron                        kubernetes:batch/v1:CronJob           update
~   vpc-native-hourly-notification-cron                        kubernetes:batch/v1:CronJob           update
~   vpc-native-update-current-streak-cron                      kubernetes:batch/v1:CronJob           update
~   vpc-native-user-profile-updated-sync-cron                  kubernetes:batch/v1:CronJob           update
~   vpc-native-personalized-digest-deployment                  kubernetes:apps/v1:Deployment         update
~   vpc-native-generic-referral-reminder-cron                  kubernetes:batch/v1:CronJob           update
~   vpc-native-generate-search-invites-cron                    kubernetes:batch/v1:CronJob           update
~   vpc-native-update-tag-recommendations-cron                 kubernetes:batch/v1:CronJob           update
~   vpc-native-update-source-public-threshold-cron             kubernetes:batch/v1:CronJob           update
~   vpc-native-sync-subscription-with-cio-cron                 kubernetes:batch/v1:CronJob           update
~   vpc-native-clean-zombie-users-cron                         kubernetes:batch/v1:CronJob           update
~   vpc-native-clean-gifted-plus-cron                          kubernetes:batch/v1:CronJob           update
~   vpc-native-bg-deployment                                   kubernetes:apps/v1:Deployment         update
~   vpc-native-post-analytics-history-day-clickhouse-cron      kubernetes:batch/v1:CronJob           update
-   vpc-native-api-clickhouse-migration-a677f13f               kubernetes:batch/v1:Job               delete
~   vpc-native-calculate-top-readers-cron                      kubernetes:batch/v1:CronJob           update
~   vpc-native-private-deployment                              kubernetes:apps/v1:Deployment         update
~   vpc-native-deployment                                      kubernetes:apps/v1:Deployment         update
~   vpc-native-temporal-deployment                             kubernetes:apps/v1:Deployment         update
~   vpc-native-personalized-digest-cron                        kubernetes:batch/v1:CronJob           update
+   api-sub-api.rematched-opportunity-notification             gcp:pubsub/subscription:Subscription  create
~   vpc-native-check-analytics-report-cron                     kubernetes:batch/v1:CronJob           update
~   vpc-native-clean-zombie-user-companies-cron                kubernetes:batch/v1:CronJob           update
~   vpc-native-clean-zombie-opportunities-cron                 kubernetes:batch/v1:CronJob           update
~   vpc-native-update-trending-cron                            kubernetes:batch/v1:CronJob           update
~   vpc-native-user-posts-analytics-refresh-cron               kubernetes:batch/v1:CronJob           update
~   vpc-native-update-highlighted-views-cron                   kubernetes:batch/v1:CronJob           update
+   vpc-native-api-clickhouse-migration-da7036d5               kubernetes:batch/v1:Job               create
~   vpc-native-expire-super-agent-trial-cron                   kubernetes:batch/v1:CronJob           update
~   vpc-native-clean-stale-user-transactions-cron              kubernetes:batch/v1:CronJob           update
~   vpc-native-ws-deployment                                   kubernetes:apps/v1:Deployment         update
~   vpc-native-daily-digest-cron                               kubernetes:batch/v1:CronJob           update
~   vpc-native-user-profile-analytics-history-clickhouse-cron  kubernetes:batch/v1:CronJob           update
~   vpc-native-update-source-tag-view-cron                     kubernetes:batch/v1:CronJob           update
-   vpc-native-api-db-migration-a677f13f                       kubernetes:batch/v1:Job               delete
~   vpc-native-update-views-cron                               kubernetes:batch/v1:CronJob           update
~   vpc-native-post-analytics-clickhouse-cron                  kubernetes:batch/v1:CronJob           update
... and 2 other changes

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Should we not only do this if you rejected for the reason if change?
I think its mote important to rematch on gondul with people it skipped?

@capJavert
Copy link
Contributor Author

@rebelchris for now only rejected, we will expand it further later

@capJavert capJavert force-pushed the opportunity-rematch branch from 4f5ef90 to 7ce564f Compare February 3, 2026 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants