feat: use aws for get reference number

[AvivAbachi]

No description provided.

[github-actions]

Test Results: Open Results!

[AvivAbachi]

@claude
Please review this pull request with a focus on:

Code quality and best practices
Potential bugs or issues
Security implications
Performance considerations

[claude]

Claude finished @AvivAbachi's task in 1m 23s —— View job

---

Code Review Complete ✓

I've reviewed this PR which migrates from web scraping the Israeli government forms site to using an AWS API Gateway endpoint for obtaining reference numbers. All CI checks pass. Here's my comprehensive review:

---

Summary

Overall Assessment: Good refactoring that simplifies the codebase and improves reliability. The migration from Cheerio-based HTML scraping to a proper API endpoint is a positive change.

Key Changes:

• Replaced cheerio dependency with dotenv (net reduction of 291 lines in package-lock.json)
• Migrated from scraping HTML forms to calling AWS API Gateway
• Removed null check handling in complaints controller
• Enhanced test coverage with proper error scenarios

---

Code Quality & Best Practices

✅ Strengths

1. Better Architecture: Moving from HTML scraping to an API is more maintainable and less fragile
2. Good Error Handling: Proper error messages for missing credentials, HTTP failures, and JSON parsing errors
3. Comprehensive Testing: Tests now cover 4 scenarios including credential validation, HTTP errors, and JSON parsing failures
4. Reduced Dependencies: Removing Cheerio and its transitive dependencies reduces attack surface
5. Proper Documentation: JSDoc comments explain the function behavior and error conditions

⚠️ Issues & Concerns

1. Inconsistent Error Handling Pattern (src/utils/getReferenceNumber.js:19-29)

The function checks response.ok but ky automatically throws on HTTP errors by default. This check is redundant:

if (!response.ok) {
  throw new Error('Failed to get Reference Number');
}

ky will already throw a HTTPError for non-2xx responses, so this is unreachable code.

Fix this →

2. Returning null in Error Path (src/utils/getReferenceNumber.js:23-24)

The function returns null when ref or guid are missing, but this is inside a try-catch that wraps all errors. This creates inconsistent behavior:

• Missing credentials → throws Error
• HTTP failure → throws Error
• Missing ref/guid → returns null

Consider throwing an error for consistency:

if (!data.ref || !data.guid) {
  throw new Error('Invalid response: missing ref or guid');
}

Fix this →

3. Null Check Removed but Return Type Still Allows Null (src/controllers/complaints.controller.js:22)

The null check was removed from the controller, but getReferenceNumber() can still return null (line 24 in getReferenceNumber.js). This could cause Cannot read property 'ref' of null errors at runtime.

4. Missing Test Coverage for Null Return (tests/getReferenceNumber.test.js)

There's no test for the case where data.ref or data.guid are missing, even though the code has logic to return null in that case.

---

Security Implications

✅ Positive Changes

1. No More Web Scraping: Eliminates risks from parsing untrusted HTML
2. Credential Validation: Checks for required environment variables before making requests
3. Reduced Dependencies: Removing Cheerio and its 10+ transitive dependencies reduces supply chain risk

⚠️ Concerns

1. API Key in Environment Variables (src/utils/getReferenceNumber.js:9-11)

Using environment variables for secrets is standard practice, but ensure:

• .env files are in .gitignore
• Production secrets are managed securely (AWS Secrets Manager, etc.)
• API keys have minimal required permissions

2. Hardcoded AWS Region (src/utils/getReferenceNumber.js:14)

The region us-east-1 is hardcoded. Consider using an environment variable for flexibility:

const AWS_REGION = process.env.AWS_REGION || 'us-east-1';
const url = `https://${process.env.AWS_APP_ID}.execute-api.${AWS_REGION}.amazonaws.com/prod`;

3. No Rate Limiting or Retry Logic

The AWS API might have rate limits. Consider adding retry logic with exponential backoff, especially since this is called for every complaint submission.

---

Performance Considerations

✅ Improvements

1. Faster Response: Direct API call is faster than fetching and parsing HTML
2. Smaller Bundle: Removing Cheerio reduces application size
3. Reasonable Timeout: 30-second timeout is appropriate for API calls

⚠️ Concerns

1. No Caching Strategy

Each complaint requires a new reference number API call. If reference numbers could be reused within a time window, implementing a short-lived cache could improve performance and reduce AWS API costs.

2. Sequential Blocking Call (src/controllers/complaints.controller.js:22)

Every complaint submission waits for the AWS API. If the AWS endpoint is slow, it affects user experience. Consider:

• Monitoring API latency
• Setting up alerts for slow responses
• Documenting expected SLA from the AWS endpoint

---

Potential Bugs

Critical Bug: The null return path (getReferenceNumber.js:24) combined with the removed null check (complaints.controller.js:23) creates a potential runtime error:

// getReferenceNumber.js line 23-24
if (!data.ref || !data.guid) {
  return null;  // ← Can return null
}

// complaints.controller.js line 22
const clientData = isDebug ? { ref: '1234567', guid: 'test' } : await getReferenceNumber();
// ← No null check anymore!

// Line 24 will crash if clientData is null
const xml = templateBuilder(request.body, clientData.ref);

Recommended Fix: Make error handling consistent by always throwing errors, never returning null.

---

Additional Recommendations

1. Add logging for AWS API calls to track latency and failures
2. Consider metrics for monitoring AWS API availability
3. Document the AWS API endpoint in README/documentation
4. Add integration test that validates the actual AWS endpoint behavior (if feasible in CI)

---

Test Results

All CI checks passed:

• ✅ Tests (linting + mocha + coverage)
• ✅ Docker Test
• ✅ Lint PR

---