fix string concatenation of number fields in advanced charts

[cairocoder01]

In Metrics -> Project -> Advanced Charts, if you selected a number field (e.g. Member Count on Groups), the bar chart would end up showing very high values. This was a result of a string value returned from the API not being parsed as an INT and thus causing string concatenation instead of integer addition.

This parses the needed field in the API response as an INT to fix the issue.

[github-actions]

Code Review

Summary

This PR fixes a critical bug in the advanced charts feature where number fields (e.g., Member Count on Groups) were displaying incorrectly inflated values due to string concatenation instead of integer addition.

Analysis

Root Cause:
The cumulative_offset value returned from the PHP backend (dt-metrics/counters/counter-post-stats.php:1129) is a numeric string. Without parsing, JavaScript performs string concatenation when adding it to other values in formatSimpleMonthData() (line 1392) and calculateCumulativeTotals() (line 1496).

The Fix:
Converting response.cumulative_offset to an integer using parseInt() ensures proper arithmetic operations throughout the charting logic.

Code Quality ✅

Strengths:

• Minimal, targeted fix that addresses the root cause
• Follows existing pattern (line 1391 already uses parseInt(monthData.count))
• Defensive coding with undefined check maintained
• No breaking changes to API or data structures

Observations:

• The fix is correctly placed at the data ingestion point, preventing downstream issues
• Alternative approaches (fixing the PHP backend to return an int, or fixing each usage site) would be more invasive

Testing Considerations ⚠️

Manual Testing Required:
Since there are no automated tests for this metrics functionality, please verify:

1. Number fields (e.g., "Member Count") display correct cumulative values
2. Bar charts show expected sums, not concatenated strings
3. Both monthly and yearly views work correctly
4. Edge cases: zero values, negative numbers (if applicable), undefined/null values

Example Test Case:

• Create groups with member counts: 5, 10, 15
• View advanced chart for "Member Count" field
• Verify cumulative values: 5, 15, 30 (not "5", "510", "51015")

Potential Issues 🔍

1. Type Coercion Edge Cases:
parseInt() behavior to consider:

• parseInt(undefined) returns NaN - mitigated by the ternary operator
• parseInt(null) returns NaN - should handle explicitly
• parseInt("") returns NaN - should handle explicitly
• parseInt("123.456") returns 123 - acceptable for counts

Recommendation: Consider using parseInt(response.cumulative_offset, 10) || 0 to handle edge cases more robustly.

2. Consistency Across Field Types:
The connection field type (line 1046) may have similar issues if it uses numeric offsets. Worth investigating processConnectionData() for consistency.

Security ✅

No security concerns. This is a client-side data processing fix with no injection risks.

Performance ✅

Negligible impact. parseInt() is a native operation with minimal overhead.

Best Practices Alignment ✅

• Follows WordPress/DT coding patterns
• Maintains existing code structure
• No unnecessary complexity added

Recommendations

Before Merge:

1. ✅ Manual testing with various number field scenarios
2. ⚠️ Consider adding , 10 radix parameter: parseInt(response.cumulative_offset, 10)
3. ⚠️ Consider fallback: parseInt(response.cumulative_offset, 10) || 0

Future Improvements:

1. Add unit tests for metrics data formatting functions
2. Consider type safety (TypeScript or JSDoc) for metrics data structures
3. Audit similar numeric fields in other chart types for consistency

Verdict: ✅ Approve with Minor Suggestions

This is a solid fix for a real bug. The core change is correct and well-targeted. The minor suggestions above would make it more robust, but the current implementation is acceptable and safe to merge.

Impact: High (fixes broken functionality)
Risk: Low (minimal, well-scoped change)
Complexity: Low (simple type conversion)

---

Review generated with assistance from Claude Code

[cairocoder01]

Concerning:

2. Consistency Across Field Types:
   The connection field type (line 1046) may have similar issues if it uses numeric offsets. Worth investigating processConnectionData() for consistency.

I tested the connection field type in the same chart and it didn't have the same issue from what I could see. The API returns values as integers instead of strings.