Fix getUseIpAddressForGeolocation always returning true #316
Conversation
|
@jaredmixpanel Can you get someone to review this PR? It seems like a serious bug -- library consumers think they're turning off IP tracking, but they're not, possibly breaking their regulatory commitments. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the getUseIpAddressForGeolocation method where it always returned true, even when explicitly set to false. The issue stemmed from using || true as the fallback, which caused the logical OR to always return true when the value was false. The fix uses the in operator to explicitly check if the property exists before accessing it, correctly handling the case where the value is intentionally set to false.
Key Changes:
- Fixed boolean logic in
getUseIpAddressForGeolocationto properly returnfalsewhen explicitly configured - Minor code style improvement to import statement spacing
| getUseIpAddressForGeolocation(token) { | ||
| return ( | ||
| (this._config[token] && this._config[token].useIpAddressForGeolocation) || | ||
| true | ||
| ); | ||
| if ( | ||
| this._config[token] && | ||
| "useIpAddressForGeolocation" in this._config[token] | ||
| ) { | ||
| return this._config[token].useIpAddressForGeolocation; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The fix correctly addresses the bug where getUseIpAddressForGeolocation always returned true due to the || true fallback. However, this fix lacks test coverage. Consider adding a test case to verify that when useIpAddressForGeolocation is explicitly set to false, the getter returns false rather than true. This would prevent regression of this bug.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
Sorry this took so long, thanks for the fix. Released with |
2 participants
Fixes #315.
This is part 1 of 3 in a stack made with GitButler: