feat: public opportunity apply

[capJavert]

No description provided.

[capJavert]

@claude review

[claude]

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

---

Code Review

• Read and analyze changed files
• Check for bugs, security issues, and best practices
• Verify test coverage and patterns
• Post comprehensive review feedback

---

Summary

This PR adds a public opportunity apply feature that allows authenticated users to apply to LIVE opportunities. The implementation includes:

• New opportunityApply mutation
• Access control logic to prevent duplicate applications
• New CandidateApplied status
• Comprehensive test coverage
• Anonymous user access to LIVE opportunities in opportunityById query

✅ What's Good

1. Excellent test coverage - The test suite is thorough with 6 test cases covering authentication, LIVE state checks, duplicate application prevention, non-existent opportunities, and invalid UUID validation.

2. Proper UUID validation - Using z.uuid() for input validation follows Zod 4.x best practices.

3. Good separation of concerns - Access control logic is properly extracted to the accessControl.ts module.

4. Consistent error handling - Uses appropriate error types (AuthenticationError, NotFoundError, ConflictError).

5. Anonymous access to LIVE opportunities - The existing opportunityById query already supports anonymous users viewing LIVE opportunities (lines 1390-1402 in opportunity.ts), and tests confirm this behavior.

🔍 Issues & Recommendations

1. Potential N+1 Query Issue (Minor)

Location: src/schema/opportunity.ts:3060-3068

The final GraphORM query uses .andWhere({ userId: ctx.userId }) which should work fine, but there's a subtle inefficiency: you're querying after the insert when you could return the created entity directly.

Current pattern:

await ctx.con.getRepository(OpportunityMatch).save({
  opportunityId,
  userId: ctx.userId,
  status: OpportunityMatchStatus.CandidateApplied,
  // ...
});

return await graphorm.queryOneOrFail<GQLOpportunityMatch>(
  ctx,
  info,
  (builder) => {
    builder.queryBuilder
      .where({ opportunityId })
      .andWhere({ userId: ctx.userId });
    return builder;
  },
);

Suggestion: Capture the saved entity and use it:

const savedMatch = await ctx.con.getRepository(OpportunityMatch).save({
  opportunityId,
  userId: ctx.userId,
  status: OpportunityMatchStatus.CandidateApplied,
  description: {},
  screening: [],
  feedback: [],
  applicationRank: {},
});

return await graphorm.queryOneOrFail<GQLOpportunityMatch>(
  ctx,
  info,
  (builder) => {
    builder.queryBuilder
      .where({ opportunityId: savedMatch.opportunityId })
      .andWhere({ userId: savedMatch.userId });
    return builder;
  },
);

However, this is a minor optimization and the current pattern is acceptable.

2. Inconsistent Query Pattern (Medium)

Location: src/schema/opportunity.ts:3037-3040

You're using ctx.con.getRepository(OpportunityJob).findOne() for the opportunity existence check, but CLAUDE.md recommends using queryReadReplica for read-only queries when eventual consistency is acceptable.

Current:

const opportunity = await ctx.con.getRepository(OpportunityJob).findOne({
  where: { id: opportunityId },
  select: ['id', 'state'],
});

However, in this case you actually SHOULD use the primary connection (not read replica) because:

1. This is part of a mutation operation
2. You need immediate consistency to ensure the state is correct before creating the match
3. The check happens right before a write operation

So the current implementation is actually correct! But it's worth adding a brief comment to clarify why we're not using the read replica here:

// Use primary connection for immediate consistency before write operation
const opportunity = await ctx.con.getRepository(OpportunityJob).findOne({
  where: { id: opportunityId },
  select: ['id', 'state'],
});

3. Missing Transaction (Medium Priority)

Location: src/schema/opportunity.ts:3018-3068

The mutation performs multiple database operations sequentially:

1. Check for existing match (in ensureOpportunityPermissions)
2. Verify opportunity exists and is LIVE
3. Create new match

According to CLAUDE.md best practices:

Wrap multiple sequential database updates in a transaction to ensure atomicity - if any operation fails, all changes are rolled back.

While the read operations don't need a transaction, having them outside provides better separation. However, if there were multiple writes, they should be wrapped. In this specific case, there's only one write operation (the save), so a transaction is not strictly necessary. But if you were to add more writes (e.g., updating user stats, sending notifications synchronously), you'd want to wrap them.

Current implementation is acceptable for a single write, but worth keeping in mind for future changes.

4. Function Style Preference (Style)

Location: src/schema/opportunity.ts:3019-3023

According to CLAUDE.md:

Prefer const arrow functions over function declarations

The mutation uses a traditional async function declaration, but given the context (it's within the resolvers object), this is the standard GraphQL pattern and is fine.

5. Test Assertion Style (Minor)

Location: __tests__/schema/opportunity.ts:2891-2895

The test uses separate assertions for each field:

expect(res.data.opportunityApply).toEqual({
  opportunityId: '550e8400-e29b-41d4-a716-446655440002',
  userId: '3',
  status: OpportunityMatchStatus.CandidateApplied,
});

Then separately verifies the database:

expect(match).toMatchObject({
  status: OpportunityMatchStatus.CandidateApplied,
  description: {},
  screening: [],
  feedback: [],
  applicationRank: {},
});

According to CLAUDE.md:

When evaluating response objects (GraphQL, API), prefer toEqual and toMatchObject over multiple expect().toBe() lines

This is already following best practices! ✅

6. Opportunity State Constants (Suggestion)

When checking for LIVE state, consider if there will be other valid states in the future. The current check opportunity.state !== OpportunityState.LIVE is explicit and correct, but per CLAUDE.md:

Prefer negative checks over listing states when checking for "non-draft" or similar conditions.

In this case, you're doing the opposite (checking for a specific state), which is correct for this business logic. The opportunity must be LIVE to apply.

🎯 Action Items

1. Optional: Add a comment explaining why the primary connection is used for the opportunity lookup (see chore: add helm chart #2 above) - Fix this →

2. Optional: Minor optimization to avoid re-querying the match (see chore: add cloudbuild support #1 above), though current pattern is acceptable

📊 Test Coverage Analysis

The test suite covers:

• ✅ Authentication requirement
• ✅ Successful application to LIVE opportunity
• ✅ Database verification of created match
• ✅ Non-LIVE opportunity rejection
• ✅ Duplicate application prevention
• ✅ Non-existent opportunity handling
• ✅ Invalid UUID validation
• ✅ Anonymous user access to LIVE opportunities
• ✅ Anonymous user blocked from non-LIVE opportunities

Excellent coverage! No gaps identified.

🔒 Security Assessment

• ✅ Authentication required via @auth directive
• ✅ Duplicate application check via access control
• ✅ State validation (only LIVE opportunities)
• ✅ UUID validation to prevent injection
• ✅ Proper error messages that don't leak sensitive information

📝 Overall Assessment

Status: ✅ Approved with minor suggestions

This is a solid implementation that follows most best practices. The code is well-tested, properly structured, and secure. The suggestions above are minor optimizations and documentation improvements that would make the code even better, but they're not blockers for merging.

The most important suggestion is adding the clarifying comment about using the primary connection (see #2), but even that is optional since the implementation is correct.

---

[pulumi]

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

Resource Changes

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

[rebelchris]

Looks good?