Implement request blocking for specific account ID; add tests for blo… #483
Conversation
…cked account behavior
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughA blocked-account guard for account ID Changes
Sequence DiagramsequenceDiagram
participant Client
participant RequestMgr as RequestManager
participant Dispatcher as RequestDispatcher
participant Logger
rect rgb(240, 248, 255)
note over Client,Logger: Blocked Account Flow (8R5-4K5-466Z)
Client->>RequestMgr: post(url, body)
activate RequestMgr
RequestMgr->>RequestMgr: check account.id === '8R5-4K5-466Z'
RequestMgr->>Logger: debug("Account blocked")
RequestMgr-->>Client: Promise.reject(Error('Account blocked'))
deactivate RequestMgr
end
rect rgb(230, 245, 230)
note over Client,Logger: Non-Blocked Account Flow
Client->>RequestMgr: post(url, body)
activate RequestMgr
RequestMgr->>RequestMgr: account check passes
RequestMgr->>Dispatcher: fireRequest(url, ...)
activate Dispatcher
Dispatcher->>Dispatcher: check account.id === '8R5-4K5-466Z'
Dispatcher->>Dispatcher: account check passes
Dispatcher->>Dispatcher: add headers, compression, etc.
Dispatcher-->>RequestMgr: success
deactivate Dispatcher
RequestMgr-->>Client: Promise.resolve(result)
deactivate RequestMgr
end
rect rgb(255, 245, 230)
note over Client,Logger: New Token/Event Processing
Client->>RequestMgr: registerToken(payload) / processEvent(data)
activate RequestMgr
RequestMgr->>RequestMgr: augment with system data
RequestMgr->>RequestMgr: compress payload
RequestMgr->>Dispatcher: fireRequest(url, ...)
activate Dispatcher
Dispatcher-->>RequestMgr: success
deactivate Dispatcher
RequestMgr-->>Client: Promise.resolve()
deactivate RequestMgr
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.6)clevertap.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/util/requestDispatcher.js (1)
16-166: Restore lint-compliant spacing on method declarations.ESLint now fails with
space-before-function-parenon the updated declarations (#fireRequest,fireRequest,#dropRequestDueToOptOut,#addUseIPToRequest,#addARPToRequest,getDelayFrequency). Please restore the required space before each parameter list so CI passes.- static #fireRequest(url, tries, skipARP, sendOULFlag, evtName) { + static #fireRequest (url, tries, skipARP, sendOULFlag, evtName) { @@ - static fireRequest(url, skipARP, sendOULFlag, evtName) { + static fireRequest (url, skipARP, sendOULFlag, evtName) { @@ - static #dropRequestDueToOptOut() { + static #dropRequestDueToOptOut () { @@ - static #addUseIPToRequest(pageLoadUrl) { + static #addUseIPToRequest (pageLoadUrl) { @@ - static #addARPToRequest(url, skipResARP) { + static #addARPToRequest (url, skipResARP) { @@ - getDelayFrequency() { + getDelayFrequency () {src/modules/request.js (1)
22-304: Fix the space-before-function-paren lint violations.The updated class methods now violate our lint rule (
space-before-function-paren), causing ESLint to fail. Please restore the space before each parameter list on the affected declarations.- constructor({ logger, account, device, session, isPersonalisationActive }) { + constructor ({ logger, account, device, session, isPersonalisationActive }) { @@ - processBackupEvents(oulOnly = false) { + processBackupEvents (oulOnly = false) { @@ - addSystemDataToObject(dataObject, ignoreTrim) { + addSystemDataToObject (dataObject, ignoreTrim) { @@ - addFlags(data) { + addFlags (data) { @@ - saveAndFireRequest(url, override, sendOULFlag, evtName) { + saveAndFireRequest (url, override, sendOULFlag, evtName) { @@ - #getNextAvailableReqN() { + #getNextAvailableReqN () { @@ - unregisterTokenForGuid(givenGUID) { + unregisterTokenForGuid (givenGUID) { @@ - registerToken(payload) { + registerToken (payload) { @@ - processEvent(data) { + processEvent (data) { @@ - #addToLocalEventMap(evtName) { + #addToLocalEventMap (evtName) { @@ - post(url, body) { + post (url, body) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (5)
clevertap.js(2 hunks)src/modules/request.js(7 hunks)src/util/requestDispatcher.js(4 hunks)test/unit/modules/blockedAccountRequest.spec.js(1 hunks)test/unit/util/blockedAccount.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/unit/util/blockedAccount.spec.js (2)
src/util/requestDispatcher.js (1)
RequestDispatcher(8-168)src/modules/account.js (1)
Account(3-89)
src/modules/request.js (6)
clevertap.js (25)
data(8105-8105)data(8790-8790)data(9953-9953)data(10320-10320)data(10576-10576)data(16710-16710)data(18481-18481)data(18501-18501)url(9962-9962)url(13788-13788)url(14292-14292)sendOULFlag(10482-10482)LCOOKIE_NAME(167-167)$ct(7469-7516)$ct(11745-11750)PUSH_SUBSCRIPTION_DATA(195-195)isValueValid(295-301)obj(9036-9036)obj(9066-9066)compressData(8153-8156)addToURL(8516-8518)FIRE_PUSH_UNREGISTERED(194-194)WEBPUSH_LS_KEY(170-170)CAMP_COOKIE_NAME(156-156)EVT_PUSH(178-178)src/util/storage.js (3)
StorageManager(11-280)$ct(282-324)$ct(282-324)src/util/constants.js (12)
LCOOKIE_NAME(19-19)LCOOKIE_NAME(19-19)PUSH_SUBSCRIPTION_DATA(46-46)PUSH_SUBSCRIPTION_DATA(46-46)FIRE_PUSH_UNREGISTERED(45-45)FIRE_PUSH_UNREGISTERED(45-45)WEBPUSH_LS_KEY(25-25)WEBPUSH_LS_KEY(25-25)CAMP_COOKIE_NAME(10-10)CAMP_COOKIE_NAME(10-10)EVT_PUSH(33-33)EVT_PUSH(33-33)src/util/datatypes.js (2)
isValueValid(34-39)isValueValid(34-39)src/util/encoder.js (2)
compressData(16-19)compressData(16-19)src/util/url.js (2)
addToURL(35-37)addToURL(35-37)
test/unit/modules/blockedAccountRequest.spec.js (1)
src/modules/account.js (1)
Account(3-89)
src/util/requestDispatcher.js (2)
clevertap.js (12)
url(9962-9962)url(13788-13788)url(14292-14292)$ct(7469-7516)$ct(11745-11750)MAX_TRIES(182-182)isValueValid(295-301)isString(270-272)OPTOUT_COOKIE_ENDSWITH(173-173)useIP(16725-16725)USEIP_KEY(174-174)addToURL(8516-8518)src/util/storage.js (3)
$ct(282-324)$ct(282-324)StorageManager(11-280)
🪛 ESLint
src/modules/request.js
[error] 22-22: Missing space before function parentheses.
(space-before-function-paren)
[error] 38-38: Missing space before function parentheses.
(space-before-function-paren)
[error] 75-75: Missing space before function parentheses.
(space-before-function-paren)
[error] 109-109: Missing space before function parentheses.
(space-before-function-paren)
[error] 140-140: Missing space before function parentheses.
(space-before-function-paren)
[error] 178-178: Missing space before function parentheses.
(space-before-function-paren)
[error] 200-200: Missing space before function parentheses.
(space-before-function-paren)
[error] 227-227: Missing space before function parentheses.
(space-before-function-paren)
[error] 240-240: Missing space before function parentheses.
(space-before-function-paren)
[error] 253-253: Missing space before function parentheses.
(space-before-function-paren)
[error] 278-278: Missing space before function parentheses.
(space-before-function-paren)
src/util/requestDispatcher.js
[error] 16-16: Missing space before function parentheses.
(space-before-function-paren)
[error] 108-108: Missing space before function parentheses.
(space-before-function-paren)
[error] 112-112: Missing space before function parentheses.
(space-before-function-paren)
[error] 120-120: Missing space before function parentheses.
(space-before-function-paren)
[error] 128-128: Missing space before function parentheses.
(space-before-function-paren)
[error] 140-140: Missing space before function parentheses.
(space-before-function-paren)
| } // Drop all requests for blocked account ID | ||
|
|
||
|
|
||
| if (this.account && this.account.id === '8R5-4K5-466Z') { | ||
| this.logger.debug('req dropped for blocked account: ' + this.account.id); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Extract hardcoded account ID to a constant and centralize blocking logic.
The account ID '8R5-4K5-466Z' is hardcoded as a magic string and duplicated across multiple locations (also appears in the post method below and in other files per the AI summary). This violates the DRY principle and makes maintenance difficult.
Additionally, logging specific account IDs may raise privacy or compliance concerns, depending on your data governance policies.
Consider the following refactor:
- Extract to a constant:
+// At the top of the file or in a shared constants module
+const BLOCKED_ACCOUNT_IDS = ['8R5-4K5-466Z'];- Centralize the blocking logic in a single helper function:
function isAccountBlocked(accountId) {
return BLOCKED_ACCOUNT_IDS.includes(accountId);
}- Use the helper function:
-if (this.account && this.account.id === '8R5-4K5-466Z') {
- this.logger.debug('req dropped for blocked account: ' + this.account.id);
+if (this.account && isAccountBlocked(this.account.id)) {
+ this.logger.debug('req dropped for blocked account');
return;
}Run the following script to verify all occurrences of the hardcoded account ID across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find all occurrences of the hardcoded account ID to assess duplication
# Search for the hardcoded account ID
rg -n "8R5-4K5-466Z" --type jsLength of output: 1202
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if clevertap.js is in build directory or gitignore
find . -name ".gitignore" -type f | xargs grep -l "clevertap.js" 2>/dev/null || echo "Not found in gitignore"
ls -la | grep -E "^d.*dist|^d.*build" 2>/dev/null || echo "No obvious build directories"
head -20 clevertap.js | head -5
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Look for constants file defining blocked accounts
rg -n "BLOCKED|blocked.*account" --type js src/ -g "!*.spec.js" | head -20Length of output: 380
🏁 Script executed:
#!/bin/bash
# Examine source implementations in detail
echo "=== src/util/requestDispatcher.js (lines 15-30) ==="
sed -n '15,30p' src/util/requestDispatcher.js
echo -e "\n=== src/modules/request.js (lines 270-290) ==="
sed -n '270,290p' src/modules/request.jsLength of output: 1360
🏁 Script executed:
#!/bin/bash
# Check if there's a dedicated blocked accounts utility/constant
find src -name "*blocked*" -o -name "*constant*" -o -name "*config*" | grep -E "\.(js|ts)$"Length of output: 68
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine constants pattern in the codebase
echo "=== src/util/constants.js (first 50 lines) ==="
head -50 src/util/constants.js | grep -A2 -B2 "ACCOUNT\|BLOCK\|ID\|LIST\|ARRAY" || head -50 src/util/constants.js
echo -e "\n=== Check for any existing blocklist or similar patterns ==="
rg -n "const.*=.*\[" src/util/constants.js | head -10
Length of output: 1035
🏁 Script executed:
#!/bin/bash
# Verify test file intent
echo "=== test/unit/util/blockedAccount.spec.js (first 50 lines) ==="
head -50 test/unit/util/blockedAccount.spec.js
echo -e "\n=== test/unit/modules/blockedAccountRequest.spec.js (first 50 lines) ==="
head -50 test/unit/modules/blockedAccountRequest.spec.jsLength of output: 3286
Extract hardcoded account ID to a constant in src/util/constants.js and centralize blocking logic across src/util/requestDispatcher.js and src/modules/request.js.
The account ID '8R5-4K5-466Z' is duplicated across two source files with identical checks:
src/util/requestDispatcher.js:23(fireRequest method)src/modules/request.js:280(post method)
This violates the DRY principle and makes maintenance difficult. Additionally, logging specific account IDs may raise privacy or compliance concerns.
Recommended refactor:
- Add to src/util/constants.js:
export const BLOCKED_ACCOUNT_IDS = ['8R5-4K5-466Z']- Create a helper function in a shared utility module:
function isAccountBlocked(accountId) {
return BLOCKED_ACCOUNT_IDS.includes(accountId)
}- Update both locations to use the helper:
src/util/requestDispatcher.js:23– Replace the hardcoded check with the helper functionsrc/modules/request.js:280– Replace the hardcoded check with the helper function- Remove account ID from debug logs (log only:
'req dropped for blocked account')
This approach scales for blocking multiple accounts and centralizes the logic for future maintenance.
| // Drop all requests for blocked account ID | ||
| if (_classPrivateFieldLooseBase(this, _account$3)[_account$3] && _classPrivateFieldLooseBase(this, _account$3)[_account$3].id === '8R5-4K5-466Z') { | ||
| _classPrivateFieldLooseBase(this, _logger$3)[_logger$3].debug('post req dropped for blocked account: ' + _classPrivateFieldLooseBase(this, _account$3)[_account$3].id); | ||
|
|
||
| return Promise.reject(new Error('Account blocked')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate blocking logic—consolidate with the check at lines 8619-8624.
This is the same hardcoded account ID check that appears earlier in the fireRequest flow (lines 8619-8624). According to the AI summary, similar logic also exists in src/modules/request.js and src/util/requestDispatcher.js. This widespread duplication makes the codebase fragile and difficult to maintain.
Recommendation:
Consolidate all blocked-account checks into a single, early validation layer—ideally in a centralized request dispatcher or guard function—so that all request paths (fireRequest, post, etc.) benefit from one authoritative check. This eliminates duplication and ensures consistent behavior across the SDK.
See the refactor suggestion in the previous comment (lines 8619-8624) for extracting the account ID to a constant and creating a reusable helper function.
🤖 Prompt for AI Agents
In clevertap.js around lines 16555-16560, the hardcoded blocked-account ID check
is duplicated here; remove this local check and instead call the centralized
guard/helper used earlier (extracted constant and function referenced near lines
8619-8624). Consolidate the blocking logic by importing or referencing the
single BLOCKED_ACCOUNT_ID constant and isAccountBlocked(account) helper from the
shared request validation module (or move that helper to a common util if not
already), then replace this block with a call to that helper and a single
Promise.reject path. Update all other duplicate sites (fireRequest,
src/modules/request.js, src/util/requestDispatcher.js) to use the same helper so
the check exists only once and ensure tests still cover the blocked-account
behavior.
…cked account behavior
Changes
Describe the key changes in this PR with the Jira Issue reference
Changes to Public Facing API if any
Please list the impact on the public facing API if any
How Has This Been Tested?
Describe the testing approach and any relevant configurations (e.g., environment, platform)
Checklist
Link to Deployed SDK
Use these url for testing :
https://static.wizrocket.com/staging/<CURRENT_BRANCH_NAME>/js/clevertap.min.jshttps://static.wizrocket.com/staging/<CURRENT_BRANCH_NAME>/js/sw_webpush.min.jsHow to trigger Automations
Just add a empty commit after all your changes are done in the PR with the command
git commit --allow-empty -m "[run-test] Testing Automation"This will trigger the automation suite
Summary by CodeRabbit
New Features
Improvements