[NO-REF] - add quiz trigger tracker events

[vladconstructor]

No description provided.

[claude]

PR Review: Add Quiz Trigger Tracker Events

Thank you for this contribution! I've reviewed the changes and have several findings to share:

---

🐛 Critical Bugs

1. Logic Error in trackQuizTriggerClick Validation (Line ~2595)

Location: src/modules/tracker.js:2595

if (typeof matchedFacet !== 'string' || typeof matchedQuery !== 'string') {
  return new Error('"Either matchedFacet or matchedQuery" is a required parameter of type string');
}

Issue: Using || (OR) instead of && (AND) means this validation will always fail if either parameter is undefined. This logic error makes it impossible to use just one of these parameters.

Fix: Should be:

if (typeof matchedFacet !== 'string' && typeof matchedQuery !== 'string') {
  return new Error('"Either matchedFacet or matchedQuery" is a required parameter of type string');
}

Impact: This bug breaks the entire trackQuizTriggerClick function - it cannot be called successfully with the current code.

---

2. Missing Parameter in trackQuizTriggerClick Body

Location: src/modules/tracker.js:2600

bodyParams.quiz_id = quizId;
bodyParams.matched_facet = matchedFacet;
bodyParams.matchedQuery = matchedQuery;  // ❌ Should be matched_query

Issue: Inconsistent snake_case conversion. Should be matched_query not matchedQuery.

Also missing: The searchQuery parameter is validated as required but never added to bodyParams.

Fix:

bodyParams.quiz_id = quizId;
bodyParams.search_query = searchQuery;  // Add this line
bodyParams.matched_facet = matchedFacet;
bodyParams.matched_query = matchedQuery;  // Fix casing

---

⚠️ Quality & Best Practices Issues

3. Tests Left in .only Mode

Location: spec/src/modules/tracker.js:8503, 8924

Both test suites have it.only on the backwards compatibility tests:

it.only('Backwards Compatibility - Should respond with a valid response...')

Issue: This will skip all other tests in the suite during test runs. This was likely left in during development.

Fix: Remove .only from both tests.

---

4. Inconsistent JSDoc Documentation

Location: src/modules/tracker.js:2540, 2530

The JSDoc for trackQuizTriggerClick has the wrong description:

* @description User typed a search query and it caused a quiz trigger to pop up

Issue: This is the same description as trackQuizTriggerShow. It should describe the click action.

Suggestion:

* @description User clicked on a quiz trigger that appeared after typing a search query

---

5. Test Validation Logic Issue

Location: spec/src/modules/tracker.js:8726, 9144

it('Should throw an error when no url is provided', () => {
  const { tracker } = new ConstructorIO({ apiKey: testApiKey });
  const { url: _, ...rest } = requiredParameters;
  expect(tracker.trackQuizTriggerShow(rest)).to.be.an('error');
});

Issue: The requiredParameters object doesn't have a url property, so this test will always pass even though it's testing nothing useful. The destructuring will just create _ as undefined.

Impact: This test provides false confidence - it's not actually validating URL handling.

---

✅ Positive Observations

1. Comprehensive test coverage - Good variety of test cases including edge cases, encoding, special characters, and network timeouts
2. Backwards compatibility - Nice addition of snake_case parameter support for API consistency
3. Type validation - Proper parameter type checking with clear error messages
4. Consistent patterns - Follows existing code structure from other tracking methods
5. TypeScript definitions - Properly added to tracker.d.ts

---

🎯 Recommendations

High Priority

1. Fix the critical logic bug in trackQuizTriggerClick validation (Bug Feature/documentation via jsdoc #1)
2. Add missing searchQuery to request body and fix matchedQuery casing in trackQuizTriggerClick (Bug Document class and module methods via JSDoc. #2)
3. Remove it.only from tests (Issue Feature/tracking module #3)

Medium Priority

4. Update JSDoc description for trackQuizTriggerClick (Issue Feature/require over import #4)
5. Remove or fix the invalid URL test case (Issue Update constructorio-identity package version. #5)

Low Priority

6. Consider adding input sanitization for the matched parameters if they come from user input
7. Consider extracting the validation logic into a shared helper since both methods have identical validation

---

📊 Security & Performance

• ✅ Security: No major security concerns. Parameters are properly validated and the code follows existing patterns.
• ✅ Performance: No performance concerns. The implementation follows the established pattern with appropriate use of the request queue.

---

🧪 Test Coverage Assessment

Coverage: Excellent overall coverage with tests for:

• ✅ Required/optional parameters
• ✅ Type validation
• ✅ Backwards compatibility (snake_case)
• ✅ User segments, testCells, userId
• ✅ Network timeouts
• ✅ Parameter encoding
• ✅ URL truncation

Missing:

• ❌ Tests won't catch Bug Feature/documentation via jsdoc #1 because the backward compatibility test is .only and other tests are being skipped
• ❌ Tests won't catch Bug Document class and module methods via JSDoc. #2 without inspecting actual request body structure

---

Please address the critical bugs before merging. Happy to re-review once these are fixed! 🚀

[claude]

Code Review: PR #412 - Add Quiz Trigger Tracker Events

Thank you for this contribution! This PR adds two new tracker methods for quiz triggers. The test coverage is comprehensive (44 test cases added), and the code follows many existing patterns well. However, I've identified several critical bugs that need to be addressed before merging.

🚨 Critical Issues (Must Fix)

1. Logic Error in trackQuizTriggerClick Parameter Validation

Location: src/modules/tracker.js:2577 (approximately)

Bug:

if (typeof matchedFacet !== 'string' || typeof matchedQuery !== 'string') {
  return new Error('"Either matchedFacet or matchedQuery" is a required parameter of type string');
}

Problem: Uses OR (||) instead of AND (&&). This requires BOTH parameters to be strings, contradicting the "either/or" error message and making the method unusable.

Fix:

if (typeof matchedFacet !== 'string' && typeof matchedQuery !== 'string') {
  return new Error('"Either matchedFacet or matchedQuery" is a required parameter of type string');
}

2. Wrong Parameter Name in trackQuizTriggerClick

Location: src/modules/tracker.js (bodyParams assignment)

Bug:

bodyParams.matchedQuery = matchedQuery;  // ❌ Wrong! Should be snake_case

Fix:

bodyParams.matched_query = matchedQuery;  // ✓ Consistent with API expectations

3. Missing searchQuery Assignment in trackQuizTriggerClick

Location: src/modules/tracker.js

Bug: The searchQuery parameter is validated but never assigned to bodyParams:

if (typeof searchQuery !== 'string') {
  return new Error('"searchQuery" is a required parameter of type string');
}
// Missing: bodyParams.search_query = searchQuery;

Fix: Add the assignment:

bodyParams.search_query = searchQuery;

Note: trackQuizTriggerShow correctly assigns all these parameters, so you can use it as reference.

---

⚠️ Major Issues

4. Test/Implementation Mismatch - URL Parameter

The tests include validation for a url parameter:

it('Should throw an error when no url is provided', () => {
  const { tracker } = new ConstructorIO({ apiKey: testApiKey });
  const { url: _, ...rest } = requiredParameters;
  expect(tracker.trackQuizTriggerShow(rest)).to.be.an('error');
});

However, the implementation doesn't validate or use a url parameter. Please clarify:

• Should url be a required/optional parameter?
• If yes, add validation and handling (see trackQuizResultsLoaded for URL truncation pattern)
• If no, remove the test cases that check for URL

5. TypeScript Definitions Don't Reflect "Either/Or" Requirement

Location: src/types/tracker.d.ts

Both matchedFacet and matchedQuery are marked as optional (?), but the implementation requires at least ONE. Consider using a union type:

trackQuizTriggerShow(
  parameters: {
    quizId: string;
    searchQuery: string;
    section?: string;
  } & (
    { matchedFacet: string; matchedQuery?: string } |
    { matchedFacet?: string; matchedQuery: string }
  ),
  networkParameters?: NetworkParameters
): true | Error;

This provides better type safety and developer experience.

---

📝 Code Quality Suggestions

6. Redundant Validation Checks

Both methods have these checks after the "either/or" validation:

if (matchedFacet && typeof matchedFacet !== 'string') {
  return new Error('If matchedFacet is present, it has to be a string');
}

These are redundant since the earlier validation already ensures at least one is a string. Consider removing them or adding a comment explaining why they're needed.

7. Error Message Clarity

Current:

return new Error('"Either matchedFacet or matchedQuery" is a required parameter of type string');

Suggested:

return new Error('At least one of "matchedFacet" or "matchedQuery" is required and must be a string');

8. Section Parameter Handling Inconsistency

These methods add section to both queryParams and bodyParams:

queryParams.section = section;
bodyParams.section = section;

However, similar methods like trackQuizResultClick only add it to queryParams. Is this intentional? If so, please add a comment explaining why.

9. Missing Complexity Comment

Similar methods like trackQuizResultsLoaded include // eslint-disable-next-line complexity. Consider adding this if needed.

---

✅ What's Good

• Excellent test coverage: 44 comprehensive test cases covering happy paths, error cases, edge cases, and backwards compatibility
• Pattern consistency: Follows existing patterns for parameter destructuring, snake_case fallback support, and request queueing
• Backwards compatibility: Supports both camelCase and snake_case parameters
• Documentation: JSDoc comments are clear and include examples
• Edge case testing: Tests for encoding, non-breaking spaces, URL truncation, timeout handling, etc.

---

🤔 Questions/Considerations

1. Empty string validation: Should empty strings be rejected for required parameters (e.g., quizId: '')?
2. Both parameters provided: What's the expected behavior when both matchedFacet AND matchedQuery are provided?
3. URL parameter: Clarify whether URL should be supported (test suggests yes, implementation says no)

---

Security & Performance

✅ No security concerns identified
✅ No performance issues - follows existing patterns for request handling
✅ Proper parameter encoding and sanitization

---

Recommendation

Cannot merge as-is due to three critical bugs in trackQuizTriggerClick that would cause runtime failures. Once the critical issues are fixed and the test/implementation mismatch is resolved, this will be good to merge.

Please fix the three critical bugs and address the test/implementation mismatch, then I'll be happy to review again!

[esezen]

Should this be && instead of || like the one above?

[esezen]

Is URL a supported parameter here?

[esezen]

Is URL a supported parameter here?

[esezen]

This needs a change