Skip to content

Conversation

@Jignesh-dimagi
Copy link
Contributor

Technical Summary

https://dimagi.atlassian.net/browse/CCCT-1566

Labels and Review

  • Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
  • Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@coderabbitai
Copy link

coderabbitai bot commented Oct 14, 2025

📝 Walkthrough

Walkthrough

Adds a new localized string resource personalid_not_login_from_fcm_error across multiple locales (default, es, fr, hi, lt, no, pt, sw, ti). Updates ConnectActivity and ConnectMessagingActivity to gate their onCreate flows on PersonalIdManager login status. When logged in, they proceed with existing initialization (state/nav graph/navigation bar and message retrieval). When not logged in, they show a toast with the new string, start the PersonalId activity via startActivityForResult using REQUEST_CODE_PERSONAL_ID_ACTIVITY (1000), and finish the current activity. Imports Toast where needed and removes unconditional initialization previously executed in onCreate.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant ConnectActivity
  participant PersonalIdManager
  participant NavController as Nav/Fragments
  participant PersonalId as PersonalIdActivity

  User->>ConnectActivity: onCreate()
  ConnectActivity->>PersonalIdManager: isLoggedIn()
  alt Logged in
    ConnectActivity->>ConnectActivity: initStateFromExtras()
    ConnectActivity->>ConnectActivity: updateBackButton()
    ConnectActivity->>NavController: setStartDestination\nattach fragments
    ConnectActivity->>NavController: retrieveMessages()
    Note right of NavController: Normal flow continues
  else Not logged in
    ConnectActivity->>ConnectActivity: Toast(personalid_not_login_from_fcm_error)
    ConnectActivity->>PersonalId: startActivityForResult(REQUEST_CODE=1000)
    ConnectActivity-->>User: finish()
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant ConnectMessagingActivity
  participant PersonalIdManager
  participant UI as Nav Bar / Redirects
  participant PersonalId as PersonalIdActivity

  User->>ConnectMessagingActivity: onCreate()
  ConnectMessagingActivity->>PersonalIdManager: isLoggedIn()
  alt Logged in
    ConnectMessagingActivity->>UI: setupNavigationBar()
    ConnectMessagingActivity->>UI: handleRedirectIfNeeded()
  else Not logged in
    ConnectMessagingActivity->>ConnectMessagingActivity: Toast(personalid_not_login_from_fcm_error)
    ConnectMessagingActivity->>PersonalId: startActivityForResult(REQUEST_CODE=1000)
    ConnectMessagingActivity-->>User: finish()
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Pm qa 7928 #3256 — Touches the same ConnectActivity/ConnectMessagingActivity areas for messaging/navigation; closely related to the new login gating.
  • Pm qa 7928 fix #3260 — Introduces/updates the Connect activities that this PR now gates by PersonalId login.
  • Name changes as per the new name personalid #3110 — Renames/connects ConnectId to PersonalId across navigation/fragments; aligns with this PR’s PersonalId-specific gating and strings.

Suggested reviewers

  • OrangeAndGreen
  • shubham1g5
  • jaypanchal-13

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description only includes a Technical Summary link and the Labels and Review checklist but omits other required template sections such as Product Description, Feature Flag, and detailed Safety Assurance with test coverage and QA plan. Please complete the missing template sections by adding a Product Description, specifying any relevant Feature Flag, detailing the Safety Assurance (including safety story, automated test coverage, and QA plan), and updating the Release Note or QA Note labels as needed.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title accurately describes the core change of enforcing a PersonalId login when the activity is launched from a notification, which aligns with the implemented login gating logic in the code.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jignesh/fix/ccct-1566

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 729dff3 and b95282d.

📒 Files selected for processing (11)
  • app/res/values-es/strings.xml (1 hunks)
  • app/res/values-fr/strings.xml (1 hunks)
  • app/res/values-hi/strings.xml (1 hunks)
  • app/res/values-lt/strings.xml (1 hunks)
  • app/res/values-no/strings.xml (1 hunks)
  • app/res/values-pt/strings.xml (1 hunks)
  • app/res/values-sw/strings.xml (1 hunks)
  • app/res/values-ti/strings.xml (1 hunks)
  • app/res/values/strings.xml (1 hunks)
  • app/src/org/commcare/activities/connect/ConnectActivity.java (2 hunks)
  • app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.

Applied to files:

  • app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
  • app/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.

Applied to files:

  • app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
🧬 Code graph analysis (2)
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
  • PersonalIdManager (60-567)
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
  • PersonalIdManager (60-567)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Lint Code Base

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a code organisation comment but not blocking on it.

Comment on lines +39 to +43
}else{
Toast.makeText(this,R.string.personalid_not_login_from_fcm_error,Toast.LENGTH_LONG).show();
personalIdManager.launchPersonalId(this,REQUEST_CODE_PERSONAL_ID_ACTIVITY);
finish();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a util method like requirePersonalIdLogin(activity) instead that checks for Personal Id login and if not navigate user to sign up page. That way we can simply add a single statement at the top level to these activities without duplicating this code as I think this will need to be added to other places as well soon in future.

Copy link
Contributor Author

@Jignesh-dimagi Jignesh-dimagi Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1g5 I was even thinking to create the BasePersonalIdConnectActivity or also Util but this is small changes so thought of putting directly as only 2 activities are only getting effected. I am still ok moving forward with BasePersonalIdConnectActivity where check for personalId or connect (if any) can be done, your thoughts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be good with BasePersonalIdConnectActivity too, think anything that avoids duplicating this code next time we have to do so should be good enough here. But happy for this change to get merged without that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1g5 Ok let's create the base activity if anything major comes up, merging this changes now.

@Jignesh-dimagi Jignesh-dimagi merged commit 61df57d into master Oct 15, 2025
6 of 8 checks passed
@Jignesh-dimagi Jignesh-dimagi deleted the jignesh/fix/ccct-1566 branch October 15, 2025 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants