Push Providers configuration #757
Conversation
WalkthroughThis pull request makes extensive modifications to push notification handling and related configuration. It removes and refactors several methods related to push token registration (for Baidu, Huawei, and Amazon) in the core API, simplifies ProGuard rules, and updates constants. The changes refactor push type management by replacing string and enum representations with a dedicated Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushType.java (1)
83-95: Comprehensive JSON deserialization with error handling.The static
fromJSONObject()method properly deserializes PushType objects from JSON and includes appropriate error handling. One minor recommendation would be to log the exception details for debugging purposes, but the current implementation is functionally correct.Consider adding logging for the JSONException in the fromJSONObject method to help with debugging:
try { String ctProviderClassName = jsonObject.getString("ctProviderClassName"); String messagingSDKClassName = jsonObject.getString("messagingSDKClassName"); String tokenPrefKey = jsonObject.getString("tokenPrefKey"); String type = jsonObject.getString("type"); String logName = jsonObject.getString("logName"); return new PushType(type, tokenPrefKey, ctProviderClassName, messagingSDKClassName, logName); } catch (JSONException e) { + Logger.v("PushType", "Error parsing PushType from JSON: " + e.getMessage()); return null; }clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java (4)
75-75: Consider thread-safety.
If this list is updated from multiple threads, you might need synchronization or a thread-safe structure.
102-109: Method name clarity.
load()is reasonable, but a more descriptive name (e.g.,createAndInitializeProviders()) could be clearer.
655-660: Prevent duplicates.
Consider checking if the push type is already inallEnabledPushTypesbefore adding.public void addPushService(PushType pushType) { - allEnabledPushTypes.add(pushType); + if (!allEnabledPushTypes.contains(pushType)) { + allEnabledPushTypes.add(pushType); + } }
797-798: Side-effect on non-enabled list.
If concurrency is possible, confirm no concurrent modifications. Otherwise fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
clevertap-core/consumer-rules.pro(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java(9 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptRepository.kt(4 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProvider.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProviderListener.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushConstants.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationHandler.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationUtil.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java(13 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushType.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandler.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProvider.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImpl.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/IFcmSdkHandler.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/validation/ManifestValidator.java(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandlerTest.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProviderTest.kt(0 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImplTest.kt(1 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/CTHmsMessageHandler.java(2 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/HmsConstants.java(1 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/HmsProvider.java(1 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/HmsPushProvider.java(3 hunks)clevertap-hms/src/test/java/com/clevertap/android/hms/HmsMessageHandlerTest.kt(1 hunks)clevertap-hms/src/test/java/com/clevertap/android/hms/HmsPushProviderTest.kt(1 hunks)sample/src/main/java/com/clevertap/demo/MyApplication.kt(2 hunks)
💤 Files with no reviewable changes (2)
- clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProviderTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java
✅ Files skipped from review due to trivial changes (7)
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandler.java
- clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImplTest.kt
- clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandlerTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImpl.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java
- clevertap-hms/src/test/java/com/clevertap/android/hms/HmsMessageHandlerTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/IFcmSdkHandler.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (64)
clevertap-hms/src/main/java/com/clevertap/android/hms/HmsProvider.java (1)
1-14: Well-designed encapsulation of HMS push type configurationThis new class follows good design principles by centralizing the Huawei Push Service (HPS) configuration as a static
PushTypeinstance. Using constants fromHmsConstantsensures consistency while making the HMS push type easily accessible throughout the codebase.clevertap-hms/src/main/java/com/clevertap/android/hms/CTHmsMessageHandler.java (2)
4-4: Clean import refactoringThe import now correctly references the HPS constant from the dedicated HmsProvider class, which improves code organization.
68-68: Simplified HPS referenceThe code now uses the directly imported HPS constant, which is cleaner than the previous nested reference.
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProviderListener.java (1)
5-5: Simplified type reference in interfaceThe parameter type has been changed from the likely previous
PushConstants.PushTypeto the directPushTypeclass, which is more maintainable and aligns with the broader refactoring effort to improve push type handling.clevertap-hms/src/main/java/com/clevertap/android/hms/HmsPushProvider.java (3)
4-4: Appropriate static import for HPSThe import now correctly references the HPS constant from the dedicated HmsProvider class.
13-13: Direct import of PushTypeUsing the direct import of PushType class improves code readability and aligns with the refactoring effort.
36-38: Updated method signature for getPushTypeThe return type has been updated from what was likely
PushConstants.PushTypeto the directPushTypeclass, maintaining consistent implementation while simplifying the type reference.Also noting that the
getPlatform()method has been removed, which appears to be part of the broader refactoring to simplify push type handling.clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptRepository.kt (4)
4-4: Import change aligned with constant refactoring.The import of
CleverTapInstanceConfighas been added to accommodate the relocation of constants fromConstantsclass.
27-27: Relocated constant reference to improve code organization.Changed from using
Constants.KEY_ENCRYPTION_LEVELtoCleverTapInstanceConfig.KEY_ENCRYPTION_LEVEL, which better aligns with the class that naturally owns this configuration property.
40-41: Consistent use of relocated constant.Same change as above, ensuring consistency in how the encryption level constant is referenced throughout the codebase.
59-60: Simplified direct reference to imported constant.The code now uses the directly imported
MIGRATION_FAILURE_COUNT_KEYconstant instead of the fully qualifiedCryptMigrator.MIGRATION_FAILURE_COUNT_KEY, improving readability.clevertap-hms/src/test/java/com/clevertap/android/hms/HmsPushProviderTest.kt (2)
4-4: Updated import to use HPS from provider class.Updated import to use
HPSfromHmsProviderinstead of fromPushConstants.PushType, aligning with the push type refactoring throughout the codebase.
52-55:✅ Verification successful
Verify test coverage after push type changes.
With the removal of
testGetPlatform()test method, make suregetPushType()still properly returns the Huawei push type and maintains test coverage of the refactored push type functionality.
🏁 Script executed:
#!/bin/bash # Description: Check for test coverage of push type changes # Look for any tests that cover push type functionality rg -A 3 -B 3 "pushType|getPushType" --glob "*.kt" --glob "*.java"Length of output: 61589
Test Coverage Verified for Huawei Push Type Implementation
The existing test in
clevertap-hms/src/test/java/com/clevertap/android/hms/HmsPushProviderTest.ktconfirms that thegetPushType()method returnsHPSas expected. This ensures that the removal oftestGetPlatform()does not compromise the test coverage for the push type functionality.
- Confirm that
HmsPushProvider.getPushType()returnsHPS.- Verify that the test implementation in
HmsPushProviderTest.ktadequately covers the push type refactor.sample/src/main/java/com/clevertap/demo/MyApplication.kt (3)
14-14: Import for Huawei push support.Added import for
HmsProviderto enable Huawei Mobile Services push notifications.
136-137: Simplified custom handshake domain assignment.The previous conditional check for
handshakeDomainhas been removed, directly assigning the value tocustomHandshakeDomain. This simplification maintains the same functionality but with cleaner code.
138-140: Added support for Huawei push notifications.Explicitly enabled Huawei Push Service (HPS) as an additional push notification type beyond FCM, enhancing the sample app's capabilities for devices without Google services.
clevertap-core/src/main/java/com/clevertap/android/sdk/validation/ManifestValidator.java (2)
20-20: Updated import for refactored push type.Changed from importing
PushConstants.PushTypeto directly importing thePushTypeclass, aligning with the refactored approach to push type management throughout the SDK.
83-96: Simplified error handling for push types validation.Special error handling for the
HPSpush type has been removed, maintaining only the FCM validation with standard exception handling. This change streamlines the validation logic and reduces unnecessary complexity.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProvider.java (1)
13-16: Method signature updated to use standalone PushTypeThe return type has been changed from
PushConstants.PushTypetoPushType, which is part of a larger refactoring to improve type handling for push notifications. The documentation has been appropriately updated to reflect this change.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProvider.java (2)
10-10: Import updated for the standalone PushType classImport has been updated to use the standalone
PushTypeclass instead ofPushConstants.PushType, which is consistent with the interface changes.
28-30: Method implementation updated to match interfaceThe
getPushType()method implementation has been updated to return the standalonePushTypeclass, which is consistent with the changes to theCTPushProviderinterface.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationHandler.java (3)
4-4: Import updated for new PushType structureImport has been updated to use
PushType.FCMdirectly, consistent with the refactoring of push type management.
52-56: Improved method signature formattingThe
onMessageReceivedmethod parameters have been reformatted to appear on separate lines, which improves readability without changing functionality.
88-93:✅ Verification successful
Simplified token refresh logic
The
onNewTokenmethod now only handles FCM tokens, removing support for HMS push tokens. Make sure this change is intentional and aligns with the broader refactoring of push notification providers.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to HMS token refresh in the codebase rg "HPS|HMS" --type java src/main/java/com/clevertap/android/sdk/pushnotification/Length of output: 182
Action Required: Verify HMS Token Removal
The initial script used an incorrect file path (i.e., missing the
clevertap-core/prefix), which resulted in an error. To ensure that no HMS token refresh logic remains in the codebase, please execute the following script to check the proper directory:If this search returns no results, it confirms that the removal of HMS push token handling is intentional and aligns with the current refactoring. Otherwise, further cleanup might be required.
🏁 Script executed:
[run_scripts] #!/bin/bash # Verify that no references to HMS token refresh exist in the correct directory rg -i "HMS" --type java clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/Length of output: 218
HMS Token Removal Confirmed
The updated
onNewTokenmethod now solely handles FCM tokens. A directory-wide search for "HMS" in the correct path (clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/) produced no matches, confirming that the HMS token refresh logic has been fully removed. Please ensure that this change aligns with the overall refactoring strategy for push notification providers.clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (2)
9-9: Import statement changed to use direct PushType reference.The import has been updated to use the new standalone PushType class rather than accessing it through PushConstants. This change aligns with the broader refactoring of push notification handling in the SDK.
69-69: Direct import of the PushType class.Correct import of the standalone PushType class, replacing the previous nested enum approach. This improves code organization by maintaining proper separation of concerns.
clevertap-hms/src/main/java/com/clevertap/android/hms/HmsConstants.java (2)
5-8: New constants added for Huawei push service configuration.These new constants properly encapsulate the Huawei Push Service (HPS) configuration details, including delivery type, provider class, SDK class name, and registration ID property key. This structured approach improves maintainability and consistency across the SDK.
10-10: HMS_LOG_TAG now uses static string instead of dynamic value.Changed from using PushType.HPS.toString() to the static string "HPS". This modification eliminates a dependency on the PushType class and potentially improves performance by removing a method call.
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationUtil.java (2)
5-5: Updated import to use direct reference to FCM from PushType.This import change supports the refactored PushType structure, directly importing the FCM constant instead of accessing it through PushConstants.
31-35: Simplified push type handling with new method returning default push types.The method
getDefaultPushTypes()replaces the previous method that returned all push types. This change streamlines push handling by focusing on FCM as the default push service. The method now returns PushType objects instead of strings, providing stronger type safety.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushType.java (2)
1-96: Well-structured implementation of the new PushType class.This new class properly encapsulates all push notification type details in a single, cohesive entity. The implementation includes:
- A static FCM instance with predefined configuration
- Private final fields with appropriate getters
- JSON serialization/deserialization support
- Clear toString() implementation for logging
This object-oriented approach is superior to the previous enum-based implementation, providing more flexibility, better organization, and stronger typing for push notification configurations.
68-81: Robust JSON serialization with proper error handling.The
toJSONObject()method correctly serializes all PushType properties and handles potential JSONException properly. This will be useful for persisting push type configurations and passing them between components.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushConstants.java (1)
17-17: Looks good.
Defining a descriptive constant for FCM push type logging is clear and consistent with other constants.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java (15)
62-63: No issues found.
Declaring a localTAGfor logs is standard and improves traceability.
229-229: Token key retrieval.
The null/empty check is handled below. Implementation is consistent and safe.
250-250: Straightforward fix.
CallinghandleTokendirectly centralizes the refresh logic.
553-553: Simple handoff.
No concerns—this approach keeps provider-creation logic readable.
571-577: Reflection usage noted.
Catch blocks handle reflection risks. The call structure is consistent.
602-603: Creation flow is clear.
Fetching providers on initialization centralizes setup behavior.
631-634: Non-enabled push types management.
Logic is straightforward: anything not in the available providers goes here.
640-641: No issues.
Fetching allowed push types from the config is standard.
663-663: No issues.
Retrieving ping frequency in a single step is concise.
672-675: Deferred async logic.
Scheduling discovery of providers and non-enabled types in one task is coherent.
683-683: Task creation.
Async execution ensures non-blocking initialization for push AMP.
772-772: Good concurrency practice.
Delegating token refresh to a safe async task keeps the UI thread clean.
777-778: Orderly token refresh.
Refreshing available providers first is logical.
780-780: Custom providers.
Triggers refresh for providers not loaded at init. No concerns here.
786-786: Method scaffolding.
refreshAvailableCTProviderTokens()is a neat separation of concerns.clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java (16)
3-3: Static import for clarity.
UsinggetDefaultPushTypes()fromPushNotificationUtilkeeps code concise.
19-19: PushType import is appropriate.
Direct usage clarifies references to strongly typed push constants.
21-22: JSON usage is standard.
Imports forJSONArrayandJSONExceptionare necessary for push types parsing.
50-50: Default push types.
Pre-populatingpushTypesis a clear way to ensure fallback.
150-152: Propagating PushTypes.
Copying them from the provided config ensures consistent push capabilities.
196-210: JSON configuration loading.
Extracting fields from the JSON aligns with the rest of the logic. No apparent issues, though consider validating domain strings.
214-231: Optional fields.
Booleans and region strings are loaded safely with existence checks.
234-269: Extended config checks.
Parcel-friendly approach ensures consistent handling of optional fields.
262-269: Push types deserialization.
Logic properly iterates the array and constructsPushTypeobjects.
300-310: Reading push types from Parcel.
Unmarshalling logic is thorough; the JSON approach works consistently.
339-341: Getter for push types.
Returning the final list is straightforward.
343-347: Add push types safely.
Preventing duplicates by checkingcontains()ensures each push type is unique.
475-476: Parcel output for push types.
Storing them as a JSON string is a workable approach for typed data.
533-556: Key definitions are clear.
Having them all in one place aids maintenance.
560-582: Robust field assignment.
Assigning each field in the JSON object maintains consistency.
590-597: JSON array construction.
Serializing push types back to JSON is logically symmetrical to the read.
CTLalit
changed the base branch from
develop
to
feat/root/remove-push-providers-basic
CTLalit
changed the base branch from
feat/root/remove-push-providers-basic
to
develop
…ders - removes all hms, amazon, baidu constants
- removes unused code to reduce complexity.
- will reintroduce later if needed in some other way
- makes pluggable push types based on dependencies added - adds method to add new pushtype to be exposed in clevertapapi (todo) - changes find push provider method -> only checks for FCM as decided.
- moves pushtype creation within hms itself - this can be plugged in via new sdk method in future
- moves init of push providers to bg thread - hms push provider init moved to background thread - finds unavailable push providers on non ui thread - moves refreshAllTokens code to correct task threading wise - renames field and methods to correct name
- allows adding push provider via CleverTapInstanceConfig.java method addPushProvider - changes PushProviders.java implementation to add allowed push type info
- appends missed pushproviders - adds sample in demo app
CTLalit
force-pushed
the
feat/root/remove-push-providers
branch
from
e056afc to
33868c3
Compare
- fixes failed test case for hms - fixes issue with onNewToken only checking for FCM
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java
Show resolved
Hide resolved
clevertap-core/src/main/java/com/clevertap/android/sdk/validation/ManifestValidator.java
Show resolved
Hide resolved
| * and false to not receive any messages from CleverTap. | ||
| */ | ||
| @SuppressWarnings("unused") | ||
| public void pushBaiduRegistrationId(String regId, boolean register) { |
There was a problem hiding this comment.
These are used by hybrids, can proably be refactored to pushRegistrationId(id, register, pushType)
| FCM(FCM_DELIVERY_TYPE, FCM_PROPERTY_REG_ID, CT_FIREBASE_PROVIDER_CLASS, FIREBASE_SDK_CLASS), | ||
| HPS(HMS_DELIVERY_TYPE, HPS_PROPERTY_REG_ID, CT_HUAWEI_PROVIDER_CLASS, HUAWEI_SDK_CLASS), | ||
| BPS(BAIDU_DELIVERY_TYPE, BPS_PROPERTY_REG_ID, CT_BAIDU_PROVIDER_CLASS, BAIDU_SDK_CLASS), | ||
| ADM(ADM_DELIVERY_TYPE, ADM_PROPERTY_REG_ID, CT_ADM_PROVIDER_CLASS, ADM_SDK_CLASS); |
There was a problem hiding this comment.
How will baidu be injected, since we don't have a separated SDK for it?
There was a problem hiding this comment.
by the same mechanism as HMS, we will have to call method addPushType(), also added manifest support.
- adds changes to ActivityLifecycleCallback.register method to append providers for those who want to install it - adds new CleverTapAPI.getDefaultInstance method with overload for appending push providers
- allows hms, baidu providers to be added from manifest - adds meta tags in sample app
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushType.java (1)
69-95: Good implementation of JSON serialization/deserialization.The toJSONObject and fromJSONObject methods provide clean conversion between PushType objects and JSON representation, with proper exception handling.
Consider adding validation for null JSON input in the fromJSONObject method to avoid potential NullPointerExceptions.
public static PushType fromJSONObject(JSONObject jsonObject) { + if (jsonObject == null) { + return null; + } try { String ctProviderClassName = jsonObject.getString("ctProviderClassName"); String messagingSDKClassName = jsonObject.getString("messagingSDKClassName"); String tokenPrefKey = jsonObject.getString("tokenPrefKey"); String type = jsonObject.getString("type"); String logName = jsonObject.getString("logName"); return new PushType(type, tokenPrefKey, ctProviderClassName, messagingSDKClassName, logName); } catch (JSONException e) { return null; } }clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (2)
3163-3170: NewcreateInstanceIfAvailableoverload.
Enabling the creation of a CleverTap instance with extra parameters (CleverTapID andpushTypes) is a solid design. Consider ensuring thread safety if multiple threads invoke this concurrently.
3239-3239:// todo : pass manifest info herecomment.
Ensure that the placeholder is addressed or removed once final logic for additional manifest usage is determined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
clevertap-core/consumer-rules.pro(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java(9 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java(10 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java(5 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptRepository.kt(4 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/interfaces/INotificationParser.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/interfaces/IPushAmpHandler.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/interfaces/NotificationHandler.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProvider.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProviderListener.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushConstants.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationHandler.java(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationUtil.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java(13 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushType.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandler.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProvider.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImpl.java(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/IFcmSdkHandler.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/validation/ManifestValidator.java(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandlerTest.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProviderTest.kt(0 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImplTest.kt(1 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/CTHmsMessageHandler.java(2 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/HmsConstants.java(1 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/HmsProvider.java(1 hunks)clevertap-hms/src/main/java/com/clevertap/android/hms/HmsPushProvider.java(3 hunks)clevertap-hms/src/test/java/com/clevertap/android/hms/HmsMessageHandlerTest.kt(1 hunks)clevertap-hms/src/test/java/com/clevertap/android/hms/HmsPushProviderTest.kt(1 hunks)clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/PushTemplateNotificationHandler.java(2 hunks)sample/src/main/AndroidManifest.xml(1 hunks)sample/src/main/java/com/clevertap/demo/MyApplication.kt(3 hunks)
💤 Files with no reviewable changes (3)
- clevertap-core/consumer-rules.pro
- clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProviderTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java
✅ Files skipped from review due to trivial changes (2)
- clevertap-core/src/main/java/com/clevertap/android/sdk/interfaces/IPushAmpHandler.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/interfaces/INotificationParser.java
🚧 Files skipped from review as they are similar to previous changes (16)
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/IFcmSdkHandler.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptRepository.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/response/PushAmpResponse.java
- clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandlerTest.kt
- clevertap-hms/src/main/java/com/clevertap/android/hms/CTHmsMessageHandler.java
- clevertap-hms/src/test/java/com/clevertap/android/hms/HmsMessageHandlerTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmPushProvider.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProviderListener.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/validation/ManifestValidator.java
- clevertap-core/src/test/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImplTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/FcmSdkHandlerImpl.java
- clevertap-hms/src/main/java/com/clevertap/android/hms/HmsProvider.java
- sample/src/main/java/com/clevertap/demo/MyApplication.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationHandler.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/fcm/CTFcmMessageHandler.java
- clevertap-hms/src/main/java/com/clevertap/android/hms/HmsConstants.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (62)
clevertap-hms/src/main/java/com/clevertap/android/hms/HmsPushProvider.java (3)
4-4: Import changes align with new PushType implementation.The static import now correctly references
HPSfrom theHmsProviderclass rather than from the previousPushConstantslocation.
13-13: Updated import to use the new PushType class.This import change is part of the larger refactoring to use a dedicated
PushTypeclass instead of an enum withinPushConstants, which improves type safety and modularity.
36-36: Method signature updated to use the new PushType class.The return type change from
PushConstants.PushTypetoPushTypealigns with the broader refactoring effort while maintaining the same functionality.clevertap-hms/src/test/java/com/clevertap/android/hms/HmsPushProviderTest.kt (2)
4-4: Import updated to reference HPS from the new location.The import now correctly references
HPSfromHmsProviderinstead of fromPushConstants, aligning with the implementation changes.
33-55: Test coverage is maintained.Good to see that tests for the remaining methods are still in place after the removal of
testGetPlatform(). ThetestGetPushType()method properly verifies that the provider returns the correct push type.clevertap-core/src/main/java/com/clevertap/android/sdk/interfaces/NotificationHandler.java (1)
6-6: Support for structured push type handling.The change from a String parameter to a PushType parameter in the
onNewTokenmethod improves type safety and reduces the risk of errors caused by incorrect string values. This refactoring is part of a broader initiative to standardize push notification handling in the SDK.Also applies to: 20-20
clevertap-pushtemplates/src/main/java/com/clevertap/android/pushtemplates/PushTemplateNotificationHandler.java (1)
10-10: Support for structured push type handling.The change from a String parameter to a PushType parameter in the
onNewTokenmethod improves type safety and provides better clarity about the expected push notification types. This change aligns with the interface definition in theNotificationHandlerinterface.Also applies to: 54-54
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/CTPushProvider.java (1)
13-16: Simplify push type handling with dedicated PushType class.The change from
PushConstants.PushTypeto justPushTypesimplifies the API and improves readability. The updated documentation correctly references the new class.This change, along with the removal of the
getPlatform()method (mentioned in the AI summary but not visible in the provided code), shows a positive direction toward better encapsulation and type safety in the push notification system.clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java (5)
39-41: Well-structured constants for push provider configuration.These constants define keys for retrieving push provider values from the Android manifest, allowing for flexible configuration of push notification providers.
95-96: Good addition of storage fields for provider values.These fields will store the push provider values retrieved from the manifest, following the existing pattern of private final fields.
169-170: Proper initialization of push provider fields from manifest.The provider values are correctly initialized by retrieving them from the manifest metadata using the constants defined earlier.
230-231: Default null initialization for providers in parameterized constructor.Setting the provider fields to null in this constructor is appropriate since these values are intended to be retrieved from the manifest.
344-350: Good addition of getter methods for provider fields.These public getter methods follow the existing pattern in the class and provide access to the provider values retrieved from the manifest.
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushNotificationUtil.java (2)
5-5: Simplified import for FCM push type.The direct import of FCM from PushType is more specific than the previous import, aligning with the simplified push type handling approach.
31-34:✅ Verification successful
Refactored method to return PushType objects instead of strings.
This change simplifies push notification handling by returning a typed object instead of strings. Currently, it only returns FCM as the default push type, which might limit flexibility but reduces complexity.
🏁 Script executed:
#!/bin/bash # Check if any code still depends on the older implementation that might return multiple push types # Search for calls to this method rg -A 2 "getDefaultPushTypes\(\)"Length of output: 876
Refactored PushNotificationUtil.getDefaultPushTypes Implementation
The method now returns an ArrayList containing a single, typed push notification (FCM), aligning with its usage in
CleverTapInstanceConfig.java. This refactor simplifies push notification handling by eliminating ambiguity inherent in string-based representations. While the current design limits the default return to FCM, it establishes a type-safe foundation that can be extended in the future if additional push types are required.
- Verified call in
CleverTapInstanceConfig.javaexpects an ArrayList ofPushTypeand correctly uses the updated implementation.clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushType.java (3)
8-17: Well-designed PushType class with FCM as a static instance.The PushType class encapsulates properties and behaviors for push notification types with a clean object-oriented design. The static FCM instance provides a convenient access point for the commonly used push type.
18-40: Good encapsulation of push type properties with immutable fields.The private final fields and constructor properly encapsulate the properties of a push type, making instances immutable and thread-safe.
42-66: Well-implemented getter methods and toString override.The getter methods provide controlled access to the private fields, and the toString method is properly overridden with @nonnull annotation for better debugging output.
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushConstants.java (2)
17-17: Added constant for FCM push type log name.This constant adds a standard name for FCM logging, improving consistency in the codebase.
1-18: Simplified push constants interface.The interface has been significantly simplified to focus only on FCM-related constants, removing unnecessary complexity from other push types. This aligns with the new PushType class approach.
Regarding a previous concern about Baidu injection: Based on the new design, it appears that providers will be configured through the manifest using the new provider1/provider2 fields in ManifestInfo, instead of having hardcoded push types.
clevertap-core/src/main/java/com/clevertap/android/sdk/pushnotification/PushProviders.java (14)
62-63: Good addition of a constant TAG for logging.Adding a named class constant for logging is a best practice for consistency and easier filtering of log messages.
75-75: Design improvement with a single list for non-enabled types.This change simplifies the class by consolidating what were likely multiple lists into a single
nonEnabledPushTypescollection, which improves maintainability and clarity.
229-230: Token key retrieval refactored for better encapsulation.Using
pushType.getTokenPrefKey()properly delegates key retrieval to thePushTypeclass, improving encapsulation and making the code more maintainable.
250-250: Method simplification by removing switch-case structure.The previous implementation likely had a switch-case construct to handle different push types. This refactoring properly delegates the responsibility to the
handleTokenmethod, making the code more concise and maintainable.
282-282: Consistent use of PushType for token key retrieval.Good consistency in using
pushType.getTokenPrefKey()throughout the class, reinforcing the encapsulation of push type properties.
403-403: Added TAG parameter to executor tasks for better logging.Including the TAG parameter in
CTExecutorFactory.executors(config).postAsyncSafelyTask(TAG)calls improves log traceability and debugging. This is a good practice applied consistently throughout the file.Also applies to: 672-672, 773-773, 683-683
553-554: Simplified provider retrieval by removing the isInit parameter.The code now uses a single constructor pattern for
CTPushProviderinstances, removing the conditional logic that was previously needed for initialization vs. non-initialization cases.
576-578: Constructor parameters updated for consistency.The constructor parameters now match the method signature declared in the interface, which improves code clarity and maintainability.
602-604: Improved method name for better semantics.Renaming
findCTPushProviderstofindAvailableCTPushProvidersmakes the method's purpose clearer. The direct call tocreateProviders()also simplifies the implementation.
631-636: Improved non-enabled push types discovery.The new implementation properly uses the
PushTypeobject equality for removing available providers from the list of all enabled types, resulting in a cleaner and more type-safe approach.
640-653: Enhanced SDK class checking for push types.The implementation now properly iterates through configured push types and checks for SDK class availability, providing better error handling and logging.
655-661: Good addition of a method for custom push providers.The
addPushServicemethod enhances the API by allowing clients to add custom push providers, increasing flexibility and extensibility.
673-677: Updated to use lambda for better code readability.Using a lambda expression instead of an anonymous
Callableimplementation makes the code more concise and readable. Good modernization of the code style.
786-805: Better naming for token refresh methods.Renaming
refreshCTProviderTokenstorefreshAvailableCTProviderTokensandrefreshCustomProviderTokenstorefreshNonEnabledProviderTokensimproves clarity and consistency with the rest of the codebase.clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java (12)
3-3: Import updated to use the new push types utility method.Updated import to use
getDefaultPushTypesinstead of a previous method, aligning with the new push type management approach.
19-19: Added import for the new PushType class.Correctly added the import for the new
PushTypeclass that replaces string-based push type handling.
50-50: Improved type safety with PushType objects.Changing from
ArrayList<String>toArrayList<PushType>for push types improves type safety and makes the API more robust.
167-169: Updated copy constructor to properly copy push types.The implementation correctly copies each push type from the source config to maintain complete object state.
211-232: Good addition of manifest-based push provider configuration.The new
buildPushProvidersFromManifestmethod enables loading push provider configuration from the manifest, which provides flexibility for client applications.The implementation handles provider1 and provider2 from the manifest, with proper error handling in case of configuration issues.
302-311: Enhanced JSON parsing to support PushType objects.The code now properly handles push types in JSON format, correctly parsing and instantiating PushType objects from the JSON representation.
341-352: Updated parcel reading for PushType objects.The implementation correctly reads serialized push types from the parcel and reconstructs the objects, maintaining proper serialization/deserialization behavior.
380-388: Better API for push type management.Renamed method from
getAllowedPushTypes()togetPushTypes()for clarity, and addedaddPushType()method with duplicate checking to ensure consistency of the push types collection.
574-597: Good practice with constant keys for serialization.Using named constants for all serialization keys improves maintainability and reduces the risk of typos in string literals.
621-623: Updated JSON serialization to include push types.The JSON serialization now properly includes the push types array, ensuring complete object state preservation.
631-638: Well-implemented helper for push types JSON conversion.The new
getPushTypesArray()method encapsulates the logic for converting push types to a JSON array, improving code organization and reusability.
74-83:✅ Verification successful
Valuable addition of a static factory method for default instances.
Adding
getDefaultInstance(Context context)provides a convenient way to create an instance with manifest-based configuration, which is particularly useful for simplified client code.This addresses a previous review comment about configuring push types from the manifest. Could you verify this implementation works for clients using
getDefaultInstance()instead ofinstanceWithConfig?
🏁 Script executed:
#!/bin/bash # Check if there are any examples of getDefaultInstance usage in the codebase rg -A 5 "getDefaultInstance\\(" --type javaLength of output: 9800
Verified: The new
getDefaultInstance(Context)implementation is working as intended.
The usage across the codebase—evident in modules likeclevertap-pushtemplatesand withinCleverTapAPI—confirms that clients relying ongetDefaultInstance()benefit from the manifest-based configuration introduced here. This change effectively replaces the previous reliance oninstanceWithConfigwithout breaking existing client code.clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (6)
9-9: Imports forPushTypelook good.
They introduce thePushTypereferences for managing push notification types in a centralized way.Also applies to: 69-69
782-786: Addition ofpushTypesparameter ingetDefaultInstance.
IncludingList<PushType>ensures that push types can be proactively registered when creating the default instance. No immediate issues are apparent.Also applies to: 793-793
1001-1012: OverloadedonActivityResumedmethod withcleverTapIDandpushTypes.
This preserves backward compatibility for existing method calls while enabling new parameters. The logic to instantiate CleverTap if necessary looks consistent.
3094-3094: OverloadedonActivityCreatedfor additional parameters.
AllowingonActivityCreated(activity, null, null)ensures existing calls still work. Implementation aligns well with push type handling.
3161-3161: Delegating to the newcreateInstanceIfAvailablevariant.
Redirecting the old method to the new one with null placeholders is a clean approach to avoid code duplication.
3220-3223: ExtendinggetDefaultConfigto acceptList<PushType>.
Good enhancement to unify push type configuration at instance creation time.clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java (10)
7-7: New imports and@NonNullusage.
ImportingPushTypeandNonNullenhances type safety and clarity in lifecycle callbacks.Also applies to: 9-9, 11-11
21-21: AddedpushTypesfield.
Storing push types at the lifecycle level is a clean way to pass them intoCleverTapAPImethods when activities are created or resumed.
25-26: UpdatedonActivityCreatedoverride to passpushTypes.
Ensures a consistent push configuration across activities.
30-30: Applying@NonNullononActivityDestroyed.
This annotation is correct for typical lifecycle use since an Activity reference is indeed non-null.
34-34: Applying@NonNullononActivityPaused.
Consistent with the other lifecycle methods for type safety.
39-41: EnhancedonActivityResumedforcleverTapIdandpushTypes.
Maintains backward compatibility by a null fallback path while allowing the new parameters to be propagated.
48-48: Applying@NonNullononActivitySaveInstanceState.
This is a sensible annotation given Android’s typical lifecycle contract.
52-52: Applying@NonNullononActivityStarted.
Matches the same rationale for consistent null-safety across lifecycle methods.
56-56: Applying@NonNullononActivityStopped.
Follows the same pattern of ensuring theActivityreference is non-null.
94-116: Newregistermethod overload withpushTypes.
Providing an optional overload forpushTypesis well-structured. It also correctly reuses the same lifecycle callbacks.
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java
Show resolved
Hide resolved
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java
Outdated
Show resolved
Hide resolved
clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java
Outdated
Show resolved
Hide resolved
- renames method names to be more descriptive - fixes test cases which were broken due to interface invocation signature for activityResumed, created - adds samples for baidu and hms in sample manifest entries
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java(10 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java(5 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/ActivityLifecycleCallbackTest.kt(2 hunks)sample/src/main/AndroidManifest.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sample/src/main/AndroidManifest.xml
- clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (8)
clevertap-core/src/test/java/com/clevertap/android/sdk/ActivityLifecycleCallbackTest.kt (3)
67-67: Method signature update foronActivityCreatedlooks good.The method call has been updated to include additional parameters to match the new signature, which is part of the push providers configuration changes.
84-84: Method signature update foronActivityCreatedwith CleverTap ID looks good.The method call has been properly updated to include the CleverTap ID as the second parameter and null for the third parameter.
86-86: Method signature update foronActivityResumedwith CleverTap ID looks good.The method call has been properly updated to include the CleverTap ID as the second parameter and null for the third parameter.
clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java (5)
39-41: Well-structured constants with clear naming.The constants are appropriately named following the existing pattern in the file. The comment on line 39 provides context about the intentional naming strategy to avoid static scan flagging.
95-96: Fields are properly encapsulated.The private fields with final modifier ensure immutability, which is good practice for configuration values that shouldn't change after initialization.
169-170: Proper initialization in the main constructor.The fields are correctly initialized by retrieving values from the manifest metadata.
230-231: Initialization to null in the parameter constructor.Setting these fields to null in the parameter constructor is appropriate since this constructor is likely used for testing scenarios where manifest values aren't available.
345-351: Improved method naming addressing previous feedback.The getter method names (
getVendorOneProvider()andgetVendorTwoProvider()) are more descriptive than the previously suggested "getProvider1" and "getProvider2", addressing the feedback from darshanclevertap in the prior review.Consider adding JavaDoc comments to these methods explaining their purpose and what kind of values they're expected to return from the manifest. This would help other developers understand the intended use of these providers.
+ /** + * Returns the push provider configured for vendor one in the manifest + * using the key {@link #LABEL_PUSH_PROVIDER_1} + * + * @return String representing the push provider for vendor one + */ public String getVendorOneProvider() { return provider1; } + /** + * Returns the push provider configured for vendor two in the manifest + * using the key {@link #LABEL_PUSH_PROVIDER_2} + * + * @return String representing the push provider for vendor two + */ public String getVendorTwoProvider() { return provider2; }
| @@ -64,7 +64,7 @@ class ActivityLifecycleCallbackTest : BaseTestCase() { | |||
| mockStatic(CleverTapAPI::class.java).use { | |||
| val mockActivity = mock(Activity::class.java) | |||
| value.onActivityCreated(mockActivity, Bundle()) | |||
| it.verify { CleverTapAPI.onActivityCreated(mockActivity) } | |||
| it.verify { CleverTapAPI.onActivityCreated(mockActivity, null, null) } | |||
| value.onActivityResumed(mockActivity) | |||
| it.verify { CleverTapAPI.onActivityResumed(mockActivity) } | |||
There was a problem hiding this comment.
Inconsistent method signature for onActivityResumed.
Line 69 still uses the old method signature for onActivityResumed without the additional parameters, while line 86 uses the new signature with extra parameters. This inconsistency should be fixed to match the updated API.
- it.verify { CleverTapAPI.onActivityResumed(mockActivity) }
+ it.verify { CleverTapAPI.onActivityResumed(mockActivity, null, null) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.verify { CleverTapAPI.onActivityResumed(mockActivity) } | |
| it.verify { CleverTapAPI.onActivityResumed(mockActivity, null, null) } |
- overloads ActivityLifecycleCallback.register method
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (9)
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java (9)
7-12: Good addition of essential imports.The addition of NonNull/Nullable annotations and the new PushType class improves type safety and aligns with the push notification type refactoring.
22-22: Appropriate private static field for push types.Adding the static List field maintains consistency with the existing pattern used for cleverTapId.
26-28: Good implementation of @nonnull annotation and pushTypes parameter.The NonNull annotation improves type safety, and passing the pushTypes parameter to CleverTapAPI.onActivityCreated enables proper push notification configuration during activity creation.
31-58: Consistent application of @nonnull annotations.All Activity lifecycle callback methods now consistently use @nonnull annotations, which improves type safety throughout the implementation.
40-46: Good conditional handling for pushTypes in onActivityResumed.The code correctly passes pushTypes parameter only when cleverTapId is not null, maintaining backward compatibility with existing implementations.
66-68: Method retained for backward compatibility.Keeping this original registration method and delegating to the new implementation preserves backward compatibility while allowing for the new functionality.
76-78: Good implementation of delegation pattern.This method correctly delegates to the new implementation with the additional pushTypes parameter set to null.
84-106: Comprehensive register method implementation.This updated register method now properly handles all parameters including the new pushTypes parameter, updating the static fields accordingly.
80-82: Address the requested registration method pattern.This matches the method signature requested in the previous comment by @darshanclevertap but has an implementation bug.
Referring to the previous review comment that mentioned:
darshanclevertap: @CTLalit - we might need a register method with Application and push types as the only parameters... We need a 4th -
register(android.app.Application application,final List<PushType> pushTypes)as well
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java
Outdated
Show resolved
Hide resolved
- fixes passing pushtype
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java (2)
80-82: Fixes previous issue with pushTypes handling, but missing documentation.This new method correctly implements the requested functionality to register with application and pushTypes, and fixes the previously identified issue. Consider adding JavaDoc comments to match the documentation style of other register methods.
+/** + * Enables lifecycle callbacks for Android devices with specified push types + * + * @param application App's Application object + * @param pushTypes List of push notification types to register + */ public static void register(Application application, @Nullable final List<PushType> pushTypes) { ActivityLifecycleCallback.register(application, null, pushTypes); }
84-88: Missing documentation for the main register method.This is the core register method that all other overloads delegate to, but it lacks documentation. Adding JavaDoc would improve code maintainability.
+/** + * Enables lifecycle callbacks for Android devices with specified CleverTap ID and push types + * + * @param application App's Application object + * @param cleverTapID Custom CleverTap ID + * @param pushTypes List of push notification types to register + */ public static void register( android.app.Application application, final String cleverTapID, final List<PushType> pushTypes ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-staticChecks-test-build
🔇 Additional comments (6)
clevertap-core/src/main/java/com/clevertap/android/sdk/ActivityLifecycleCallback.java (6)
22-22: Good addition of the pushTypes field.The addition of this field properly stores the push notification types for use throughout the lifecycle callbacks, similar to how cleverTapId is handled.
26-28: LGTM - Activity method signatures now have @nonnull annotation.Good improvement adding @nonnull annotations to all Activity parameters in the lifecycle callbacks, which enhances null safety. The onActivityCreated method now correctly passes the pushTypes parameter to CleverTapAPI.
42-42: Correctly propagating push types to onActivityResumed.The code now properly passes the pushTypes parameter when cleverTapId is not null.
66-68: LGTM - Original method maintains backward compatibility.The original register method now correctly delegates to the updated version with null parameters, maintaining backward compatibility.
76-78: LGTM - Updated method correctly forwards pushTypes parameter.The method correctly delegates to the fully parameterized register method, passing null for pushTypes.
99-101: LGTM - Correctly storing all parameters.The code now properly stores both cleverTapId and pushTypes, allowing them to be used in lifecycle callbacks.
| @@ -1,6 +1,5 @@ | |||
| # For CleverTap SDK's | |||
| -keep class com.clevertap.android.sdk.pushnotification.fcm.FcmPushProvider{*;} | |||
| -keep class com.clevertap.android.hms.HmsPushProvider{*;} | |||
There was a problem hiding this comment.
Should be added to the documentation or consumer rules of HMS SDK
https://wizrocket.atlassian.net/browse/SDK-4446
Summary by CodeRabbit
New Features
Refactor
Chores