Merge common CC files from Connect #3073
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces support for token-based authentication and enhanced analytics tracking, along with minor UI and resource updates. Several classes now use a unified authentication object ( Sequence Diagram(s)sequenceDiagram
participant UI
participant NetworkLayer
participant AuthManager
participant Analytics
participant NotificationFactory
UI->>NetworkLayer: Initiate network request
NetworkLayer->>AuthManager: buildAuth()
AuthManager-->>NetworkLayer: AuthInfo (TokenAuth or ProvidedAuth)
NetworkLayer->>NetworkLayer: Execute request with AuthInfo
alt TokenAuth used and response is 401
NetworkLayer->>AuthManager: checkForTokenError()
AuthManager-->>NetworkLayer: Invalidate token
NetworkLayer->>NotificationFactory: Show TokenUnavailable/TokenRequestDenied
NetworkLayer->>Analytics: Report token error event
else Success or other error
NetworkLayer->>Analytics: Report event as needed
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
app/res/drawable-anydpi/ic_marker.xml(1 hunks)app/src/org/commcare/activities/CommCareVerificationActivity.java(4 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(2 hunks)app/src/org/commcare/activities/HomeScreenBaseActivity.java(1 hunks)app/src/org/commcare/activities/LoginActivity.java(8 hunks)app/src/org/commcare/activities/LoginActivityUIController.java(6 hunks)app/src/org/commcare/activities/SyncCapableCommCareActivity.java(8 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(1 hunks)app/src/org/commcare/connect/network/IApiCallback.java(0 hunks)app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java(2 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java(2 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(2 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(6 hunks)app/src/org/commcare/network/CommcareRequestGenerator.java(5 hunks)app/src/org/commcare/network/HttpCalloutTask.java(4 hunks)app/src/org/commcare/network/HttpUtils.java(2 hunks)app/src/org/commcare/preferences/AppManagerDeveloperPreferences.java(4 hunks)app/src/org/commcare/services/CommCareSessionService.java(1 hunks)app/src/org/commcare/services/FCMMessageData.java(1 hunks)app/src/org/commcare/tasks/ManageKeyRecordTask.java(3 hunks)app/src/org/commcare/views/notifications/NotificationMessageFactory.java(1 hunks)
💤 Files with no reviewable changes (1)
- app/src/org/commcare/connect/network/IApiCallback.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/activities/LoginActivity.java (5)
app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(154-1260)app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-84)app/src/org/commcare/views/notifications/NotificationMessageFactory.java (1)
NotificationMessageFactory(20-260)app/src/org/commcare/connect/network/TokenRequestDeniedException.kt (1)
message(14-17)app/src/org/commcare/connect/network/TokenUnavailableException.kt (1)
message(12-15)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (50)
app/src/org/commcare/services/FCMMessageData.java (1)
31-31: The added blank line is purely formatting and has no functional impact. No review necessary.app/res/drawable-anydpi/ic_marker.xml (1)
1-11: Approve new custom marker iconThe vector drawable is well-defined with appropriate viewport, tint, and path data, and aligns well with the UI requirements for map-related actions.
app/src/org/commcare/services/CommCareSessionService.java (1)
20-22: Import grouping for AndroidXThe
androidx.core.app.NotificationCompatandandroidx.preference.PreferenceManagerimports are correctly placed immediately after the Android framework imports, maintaining a clear separation between platform and support libraries.app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (2)
33-33: New analytics event constant addedThe
LOGIN_CLICKconstant follows existing naming conventions and pairs with the newly introducedreportLoginClicks()method.
52-52: New analytics event constant addedThe
CCC_REKEYED_DBconstant aligns with otherCCC_*events and integrates withreportRekeyedDatabase().app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (2)
111-111: Added analytics parameter for ConnectID feature.This new constant adds support for tracking ConnectID enablement in analytics, consistent with other feature flag parameters in this section.
160-163: Minor formatting improvement.The whitespace changes improve code readability by providing consistent spacing around related constants.
app/src/org/commcare/activities/HomeScreenBaseActivity.java (1)
583-585: Fixed result intent passing for Connect opportunity flow.You've properly updated the
setResult()call to include the created intent, ensuring that theREDIRECT_TO_CONNECT_OPPORTUNITY_INFOflag is correctly passed back to the calling activity.app/src/org/commcare/activities/EntitySelectActivity.java (1)
723-723: UI improvement: Map icon now shows in action bar.Good change to use the custom
ic_markerdrawable and display it in the action bar when there's room, making the map feature more discoverable.app/src/org/commcare/activities/LoginActivityUIController.java (3)
29-30: Import organization improvement.Better organization of imports improves code readability.
Also applies to: 50-51
72-74: UI element grouping for better code structure.Moving the
connectLoginButtondeclaration to group it with other UI elements improves code organization and maintainability.
101-102: Consistent spacing for improved readability.Adding consistent blank lines between logical sections of code improves readability and follows good coding practices.
Also applies to: 228-229, 462-463
app/src/org/commcare/activities/CommCareVerificationActivity.java (4)
46-47: Added new constant for Connect ID integration.The new constant
KEY_LAUNCH_FROM_CONNECTfollows the existing naming pattern and helps identify when the activity is launched from Connect.
73-73: Added flag to track Connect ID origin.The new
fromConnectfield appropriately complements the existingfromManagerandfromSettingsfields.
92-93: Initialized fromConnect flag from intent extras.The initialization follows the existing pattern used for other origin flags.
137-137: Updated visibility logic to consider Connect ID launch.The condition correctly includes the new
fromConnectflag in the activity visibility check.app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (3)
9-9: Added analytics parameter for build number.This constant follows the established naming pattern for analytics parameters.
15-15: Added analytics parameter for Connect ID enablement.This parameter will be useful for tracking Connect ID feature usage in analytics.
29-29: Added analytics parameter for notification type.This constant allows tracking different notification types in analytics reporting.
app/src/org/commcare/tasks/ManageKeyRecordTask.java (3)
12-13: Added imports for token authentication exceptions.These imports support the new token-based authentication error handling.
220-227: Added error handling for token authentication failures.The new cases appropriately handle token-specific error conditions following the established pattern in the switch statement.
355-355: Updated method signature to declare token exceptions.The method now properly declares that it can throw the new token-related exceptions.
app/src/org/commcare/activities/SyncCapableCommCareActivity.java (3)
17-17: Added import for ConnectIDManager.This import is necessary for the new Connect ID integration logic.
108-114: Added error logging for Connect ID authentication failures.This change enhances diagnostics by logging specific error information when token authentication fails for Connect ID-managed apps.
458-459: Improved variable declaration formatting.Split the variable declarations for better readability without changing functionality.
app/src/org/commcare/network/HttpUtils.java (2)
32-33: Proper addition of token authentication support.The change correctly adds token authentication support by handling the
AuthInfo.TokenAuthtype in thegetCredentialmethod.
78-84: Well-implemented bearer token formatting.This helper method correctly formats the bearer token authorization header according to the OAuth 2.0 standard.
app/src/org/commcare/network/CommcareRequestGenerator.java (5)
151-203: Well-designed authentication centralization.You've created a comprehensive method that centralizes authentication logic, making code more maintainable and consistent. The logic handles multiple authentication scenarios properly:
- Uses Connect ID tokens when available
- Detects when token auth should be available but isn't and logs appropriately
- Falls back to current session auth when possible
- Uses provided credentials as a final fallback
204-210: Good token invalidation logic.This method correctly identifies token failures (401 responses) and invalidates the tokens to prevent repeated failures.
222-234: Consistent request update pattern.The update from direct auth instantiation to using the centralized
buildAuth()method is implemented consistently, including proper error handling withcheckForTokenError.
276-292: Consistent request update pattern.This change follows the same pattern as the other methods, maintaining consistency across the codebase.
304-315: Consistent request update pattern.The simple GET method is properly updated to use centralized auth handling and token error checking.
app/src/org/commcare/preferences/AppManagerDeveloperPreferences.java (3)
25-25: Consistent preference key and title definition.The new ConnectID preference key and title mapping follow the established pattern.
Also applies to: 32-32
62-69: Well-implemented preference click listener.The click listener correctly reports analytics and calls the toggle method.
100-100: Default value changed from false to true.The default value for ConnectID is changed from false to true. This may affect app behavior for new installations. Make sure this is intentional.
app/src/org/commcare/network/HttpCalloutTask.java (4)
5-6: Added imports for new token-related exceptions.Correctly added imports for token-related exceptions that will be handled.
49-51: New enum values for token-related outcomes.Added appropriate enum values to represent token authentication failures.
105-111: Comprehensive token exception handling.The exception handlers for token-related exceptions follow the same pattern as other exception handlers, maintaining consistency.
149-149: Updated method signature to include new exceptions.The
doHttpRequestmethod signature correctly declares the new token-related exceptions.app/src/org/commcare/activities/LoginActivity.java (2)
95-97: Consistent ordering of modifiersThe code now consistently uses
public static finalordering for constants, which follows Java standard conventions.Also applies to: 100-103
275-276: Improved code readability with better line breaksLong method chains and parameter lists have been broken into multiple lines for improved readability, making the code easier to maintain and review.
Also applies to: 589-590, 617-618, 733-734, 767-769
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (9)
45-47: Helpful utility method for simpler event loggingAdded a convenient overload for
reportEvent()that accepts just an event name without parameters, reducing boilerplate when logging simple events.
93-96: Fixed build profile property settingThe code now correctly checks if the build profile ID is non-empty before setting it as a user property, preventing potential null reference issues.
108-109: Added ConnectID login state trackingNew user property
CCC_ENABLEDtracks whether the user is logged in with ConnectID, providing valuable analytics data for feature usage.
487-489: New method to track ConnectID sign-outAdded analytics tracking for when users sign out of ConnectID, completing the lifecycle tracking of the feature.
491-493: New method to track login clicksAdded a method to report login clicks, which helps measure user engagement with the authentication features.
495-508: Added screen navigation tracking capabilityThis navigation listener implementation elegantly captures screen transitions in the app, providing valuable user journey analytics. The implementation correctly handles fragment navigation destinations and extracts relevant information for analytics reporting.
510-514: Added ConnectID tab change trackingNew method to track user navigation between tabs in the Connect interface, providing insights into feature usage patterns.
516-519: Added notification type trackingNew method to report the types of notifications displayed to users, which helps understand the frequency of different system messages.
521-523: Added database rekeying event trackingNew method to report when a database is rekeyed, which helps monitor potential security-related operations.
app/src/org/commcare/views/notifications/NotificationMessageFactory.java
Show resolved
Hide resolved
app/src/org/commcare/preferences/AppManagerDeveloperPreferences.java
Outdated
Show resolved
Hide resolved
shubham1g5
force-pushed
the
cccToMasterLoginHome
branch
from
fb0a93e to
cd5fe4c
Compare
| if(ConnectIDManager.getInstance().isSeatedAppLinkedToConnectId(username)) { | ||
| Logger.exception("Token auth error for connect managed app", | ||
| new Throwable("Token Auth failed during sync for a ConnectID managed app")); | ||
| } |
There was a problem hiding this comment.
@OrangeAndGreen wondering if it's fine to remove this now as we can land here in a genuine scenario due to token request failing ?
There was a problem hiding this comment.
I agree that we can get rid of the non-fatal exception since now it is always logged from a better place (in buildAuth), making this a duplicate. However, the sync.fail.auth.loggedin message should probably be changed in this if-case, so it indicates a possibly-temporary network issue rather than a changed password.
There was a problem hiding this comment.
Another option would be to add PullTaskResult cases for token unavailable and rejected, similar to the new HttpCalloutOutcomes. Currently these exceptions are handled as generic IOExceptions.
|
|
||
| this.fromSettings = this.getIntent(). | ||
| getBooleanExtra(KEY_LAUNCH_FROM_SETTINGS, false); | ||
| this.fromConnect = this.getIntent(). |
There was a problem hiding this comment.
we have removed this flag
| if(ConnectIDManager.getInstance().isSeatedAppLinkedToConnectId(username)) { | ||
| Logger.exception("Token auth error for connect managed app", | ||
| new Throwable("Token Auth failed during sync for a ConnectID managed app")); | ||
| } |
There was a problem hiding this comment.
I agree that we can get rid of the non-fatal exception since now it is always logged from a better place (in buildAuth), making this a duplicate. However, the sync.fail.auth.loggedin message should probably be changed in this if-case, so it indicates a possibly-temporary network issue rather than a changed password.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
Show resolved
Hide resolved
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
Show resolved
Hide resolved
| if(ConnectIDManager.getInstance().isSeatedAppLinkedToConnectId(username)) { | ||
| Logger.exception("Token auth error for connect managed app", | ||
| new Throwable("Token Auth failed during sync for a ConnectID managed app")); | ||
| } |
There was a problem hiding this comment.
Another option would be to add PullTaskResult cases for token unavailable and rejected, similar to the new HttpCalloutOutcomes. Currently these exceptions are handled as generic IOExceptions.
app/src/org/commcare/preferences/AppManagerDeveloperPreferences.java
Outdated
Show resolved
Hide resolved
shubham1g5
force-pushed
the
cccToMasterLoginHome
branch
from
5a561f3 to
e0847bf
Compare
|
Had one network related failure on integration test which I verified locally and hence running build without integration test now. |
|
@damagatchi retest this please |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1147
Safety Assurance
Safety story
Automated test coverage
Good coverage around changed areas.
QA Plan
No speicifc QA other than release test plan
Labels and Review