CCCT-2085 Unsubscribe From Messaging Channels#3547
CCCT-2085 Unsubscribe From Messaging Channels#3547conroy-ricketts wants to merge 13 commits intomasterfrom
Conversation
Removed unused function fromMessagePayload() from ConnectMessagingMessageRecord.
Gave the unsubscribe from messaging channel dialog real functionality. Sorted the messaging channels list. Removed unused string resources that were there for the messaging channel consent feature. Deleted some unused code that was implemented as part of the messaging channel consent feature. Added some UI tweaks and UI additions for the unsubscribed messaging channel state.
Removed code that was setting answeredConsent when unsubscribing from messaging channels.
conroy-ricketts
requested review from
Jignesh-dimagi,
OrangeAndGreen,
jaypanchal-13 and
shubham1g5
…nto CCCT-2085-allow-unsubscribe-from-messaging-channels
📝 WalkthroughWalkthroughThis PR removes the Connect messaging consent bottom sheet and its layout, deletes consent-related strings across multiple locales, and removes dialog/navigation entries referencing the bottom sheet. Consent handling is consolidated into ConnectMessageFragment (new subscribe/unsubscribe flows and UI state helpers). ChannelAdapter now partitions and displays consented vs unsubscribed channels with visual differences. Database preview generation was adjusted to only show previews for consented channels. Three new color resources and a drawable using one of them were added. ConnectMessagingActivity gained progress-dialog management for messaging network tasks. Sequence DiagramsequenceDiagram
participant User as User
participant CMF as ConnectMessageFragment
participant MM as MessageManager
participant DB as ConnectMessagingDatabaseHelper
participant UI as UI Layer
User->>CMF: Open channel screen
CMF->>DB: fetch channel (id)
DB-->>CMF: channel (with consent flag)
alt consented
CMF->>UI: setChannelSubscribedState()
UI-->>User: input enabled, previews shown
else unconsented
CMF->>UI: setChannelUnsubscribedState()
UI-->>User: input disabled, unsubscribed message shown
end
User->>CMF: Tap unsubscribe
CMF->>UI: showProgressDialog (NETWORK_ACTIVITY_MESSAGING_CHANNEL_ID)
CMF->>MM: updateChannelConsent(channelId, false)
MM->>DB: persist consent=false
DB-->>MM: confirm
MM-->>CMF: success
CMF->>UI: dismissProgressDialog
CMF->>UI: setChannelUnsubscribedState()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. 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: 1
🧹 Nitpick comments (3)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)
127-131: Consider encapsulatingnotifyDataSetChanged()insidesetChannels().
setChannels()mutates internal adapter state but relies on callers to remember to callnotifyDataSetChanged(). This is fragile — any future caller ofsetChannels()that forgets the notify call will produce a stale UI.♻️ Proposed refactor in ChannelAdapter.java
public void setChannels(List<ConnectMessagingChannelRecord> incomingChannels) { // ... partitioning logic ... channels.clear(); channels.addAll(subscribedChannels); channels.addAll(unsubscribedChannels); + notifyDataSetChanged(); }Then remove the explicit call in
ConnectMessageChannelListFragment.refreshUi():channelAdapter.setChannels(channels); - channelAdapter.notifyDataSetChanged();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java` around lines 127 - 131, The adapter's setChannels(...) mutates internal state but relies on callers to call notifyDataSetChanged(), which is brittle; update ChannelAdapter.setChannels(List<ConnectMessagingChannelRecord>) to call notifyDataSetChanged() after updating its internal list, remove the explicit channelAdapter.notifyDataSetChanged() call from ConnectMessageChannelListFragment.refreshUi(), and search for other callers of setChannels(...) to remove redundant notifyDataSetChanged() calls so the adapter itself is responsible for notifying the UI.app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
97-113: Commented-out menu items leave the analytics mapping (lines 115-120) and dialog handling as dead code.Since both menu items are commented out, the
menuItemsAnalyticsParamsMapping,onMenuItemSelectedlogic for unsubscribe/resubscribe,showDialogForMenuItem, and the entire unsubscribe callback at lines 338-361 are unreachable. This is fine as scaffolding if the plan is to re-enable soon, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java` around lines 97 - 113, The commented-out menu creation for MENU_UNSUBSCRIBE and MENU_RESUBSCRIBE in ConnectMessageFragment makes related code dead; either re-enable the menu items or remove the unreachable helpers and mappings: restore the conditional block that adds MENU_UNSUBSCRIBE/MENU_RESUBSCRIBE (using channel.getConsented()) so menuItemsAnalyticsParamsMapping, onMenuItemSelected, showDialogForMenuItem, and the unsubscribe callback become reachable, or if you intend to keep resubscribe disabled for now, delete the unused menuItemsAnalyticsParamsMapping entry and remove/clean up the unsubscribe/resubscribe branches in onMenuItemSelected and the unsubscribe callback to eliminate dead code and stale analytics mappings.app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java (1)
73-74: PreferisEmpty()over manual size/length comparisons.Lines 73 and 107 both check collection/string emptiness via
> 0instead of the idiomatic!isEmpty().♻️ Proposed refactor
- if (channels.size() > 0) { + if (!channels.isEmpty()) {- if (existing.getKey().length() > 0) { + if (!existing.getKey().isEmpty()) {Also applies to: 107-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java` around lines 73 - 74, The code in ConnectMessagingDatabaseHelper uses size() > 0 (e.g., the channels check returning channels.get(0)) and a similar > 0 string/collection check later; replace those with the idiomatic !isEmpty() (for collections like channels) or isEmpty() (for positive/negative checks on strings) to improve readability and null-safety—locate the checks around the channels variable and the second check referenced in the diff (lines near 73 and 107) and change them to use !channels.isEmpty() and the equivalent !someString.isEmpty()/someCollection.isEmpty() checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java`:
- Around line 339-361: The code sets channel.setConsented(false) before calling
MessageManager.updateChannelConsent, but on API failure the in-memory channel is
never reverted; update the failure branch of
MessageManager.updateChannelConsent's callback to reset the in-memory state
(call channel.setConsented(true)) before showing the Toast and hiding the
spinner so the object remains consistent; ensure this change is made alongside
existing UI updates (binding.pbLoadingSpinner.setVisibility and not calling
setChannelUnsubscribedState) and keep requireActivity().invalidateMenu() only on
success as now.
---
Nitpick comments:
In `@app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java`:
- Around line 73-74: The code in ConnectMessagingDatabaseHelper uses size() > 0
(e.g., the channels check returning channels.get(0)) and a similar > 0
string/collection check later; replace those with the idiomatic !isEmpty() (for
collections like channels) or isEmpty() (for positive/negative checks on
strings) to improve readability and null-safety—locate the checks around the
channels variable and the second check referenced in the diff (lines near 73 and
107) and change them to use !channels.isEmpty() and the equivalent
!someString.isEmpty()/someCollection.isEmpty() checks.
In
`@app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java`:
- Around line 127-131: The adapter's setChannels(...) mutates internal state but
relies on callers to call notifyDataSetChanged(), which is brittle; update
ChannelAdapter.setChannels(List<ConnectMessagingChannelRecord>) to call
notifyDataSetChanged() after updating its internal list, remove the explicit
channelAdapter.notifyDataSetChanged() call from
ConnectMessageChannelListFragment.refreshUi(), and search for other callers of
setChannels(...) to remove redundant notifyDataSetChanged() calls so the adapter
itself is responsible for notifying the UI.
In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java`:
- Around line 97-113: The commented-out menu creation for MENU_UNSUBSCRIBE and
MENU_RESUBSCRIBE in ConnectMessageFragment makes related code dead; either
re-enable the menu items or remove the unreachable helpers and mappings: restore
the conditional block that adds MENU_UNSUBSCRIBE/MENU_RESUBSCRIBE (using
channel.getConsented()) so menuItemsAnalyticsParamsMapping, onMenuItemSelected,
showDialogForMenuItem, and the unsubscribe callback become reachable, or if you
intend to keep resubscribe disabled for now, delete the unused
menuItemsAnalyticsParamsMapping entry and remove/clean up the
unsubscribe/resubscribe branches in onMenuItemSelected and the unsubscribe
callback to eliminate dead code and stale analytics mappings.
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
Outdated
Show resolved
Hide resolved
Tweaked UI a bit for the unsubscribe state.
Moved notifyDataSetChanged() back to ChannelAdapter.
Replaced checks with isEmpty() where appropriate.
Prevented stale consented value on in-memory channel.
| <ProgressBar | ||
| android:id="@+id/pbLoadingSpinner" | ||
| style="@style/Base.Widget.AppCompat.ProgressBar" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| app:layout_constraintTop_toTopOf="parent" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| android:visibility="gone"/> |
There was a problem hiding this comment.
@conroy-ricketts We should not introduce new progressbar in individual fragments but try to use available here
There was a problem hiding this comment.
Ah, exactly! See my related comment in the resubscribe PR
There was a problem hiding this comment.
Thanks guys! Will update this soon
There was a problem hiding this comment.
@Jignesh-dimagi @OrangeAndGreen Let me know what you guys think! - e9799e1
Screen_Recording_20260219_140931_CommCare.Debug.mp4
| // Menu.NONE, | ||
| // R.string.connect_messaging_channel_menu_item_resubscribe | ||
| // ); | ||
| // if (channel.getConsented()) { |
There was a problem hiding this comment.
@conroy-ricketts It think it's ok to uncomment it.
There was a problem hiding this comment.
This code is uncommented in #3549. The reason for doing it like this is so that the master branch is always in a release-ready state. I wanted to avoid showing any new menu items until this feature is fully implemented when merging into master.
| @Override | ||
| public void onTextChanged(CharSequence s, int start, int before, int count) { | ||
| if (s.length() > 0) { | ||
| if (s.length() > 0 && channel.getConsented()) { |
There was a problem hiding this comment.
Do we need this, as the editbox is already disabled at the beginning when the fragment view is created?
There was a problem hiding this comment.
Yes, we need this check or else the send button will be visible in the unsubscribed state:
Let me know if I should refactor this area of the code to make this more intuitive
| channel.setConsented(true); | ||
| Toast.makeText( | ||
| requireContext(), | ||
| errorText, |
There was a problem hiding this comment.
errorText is alway connect_messaging_channel_unsubscribe_error, will be shown if error in subscribe call also.
There was a problem hiding this comment.
The error text will change slightly depending on the action
If there is an error during the "unsubscribe" action, then the error text should be "Failed to unsubscribe". If there is an error during the "resubscribe" action then the error text should be "Failed to resubscribe"
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Nothing additional to add other than the requested change to progress/wait dialog
| <ProgressBar | ||
| android:id="@+id/pbLoadingSpinner" | ||
| style="@style/Base.Widget.AppCompat.ProgressBar" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| app:layout_constraintTop_toTopOf="parent" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| android:visibility="gone"/> |
There was a problem hiding this comment.
Ah, exactly! See my related comment in the resubscribe PR
Removed the new progress spinner in favor of reusing the already existing blocking loading dialog.
…nto CCCT-2085-allow-unsubscribe-from-messaging-channels
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
Outdated
Show resolved
Hide resolved
Refactored code to fetch channel from DB after an API call.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/org/commcare/adapters/ChannelAdapter.java (1)
98-120: Unread badge is still visible for unsubscribed channels — intentional?Lines 98–104 check
unread > 0regardless of consent status. An unsubscribed channel with leftover unread messages would show the badge but no timestamp and no preview, which looks slightly inconsistent. If unsubscribed channels should appear fully muted, consider hiding the unread count as well.♻️ Suggested change if unread badge should be hidden for unsubscribed channels
- boolean showUnread = unread > 0; + boolean showUnread = unread > 0 && channel.getConsented();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/adapters/ChannelAdapter.java` around lines 98 - 120, Unread badge is being shown regardless of consent; update the visibility logic so the unread count (binding.tvUnreadCount) is only displayed when unread > 0 AND the channel is consented (channel.getConsented()). Concretely, change the showUnread condition or add a surrounding consent check in the bind block that sets tvUnreadCount text and visibility (referencing binding.tvUnreadCount and channel.getConsented()) so that unsubscribed channels always hide the unread badge.app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
96-148: Menu items and analytics map wired but unreachable — intentional per TODO.The menu items are commented out (lines 103–117) with clear TODO comments. Note that
menuItemsAnalyticsParamsMapping(lines 119–124) is still initialized with mappings for these menu IDs. This is harmless but slightly wasteful until the menu items are re-enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java` around lines 96 - 148, The menu ID-to-analytics mapping is created in setupMenuItems() even though the actual MenuItem additions are commented out; remove or defer that initialization so we don't create unreachable mappings. Update setupMenuItems(): move the menuItemsAnalyticsParamsMapping assignment into the same branch where menu.add(...) would be executed (inside onCreateMenu), or simply delete the Map.of(...) until the MENU_UNSUBSCRIBE/MENU_RESUBSCRIBE additions are re-enabled; ensure onMenuItemSelected still guards use of menuItemsAnalyticsParamsMapping if it may be null. Reference symbols: setupMenuItems, onCreateMenu, menuItemsAnalyticsParamsMapping, onMenuItemSelected, MENU_UNSUBSCRIBE, MENU_RESUBSCRIBE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java`:
- Around line 107-109: The code in ConnectMessagingDatabaseHelper calls
existing.getKey().isEmpty() which can NPE if getKey() returns null; change the
conditional around the existing->incoming key copy (the block referencing
existing.getKey() and incoming.setKey()) to first null-check existing.getKey()
(e.g., ensure existing.getKey() != null) before calling isEmpty(), and only call
incoming.setKey(existing.getKey()) when the key is non-null and non-empty so
nulls from database/SqlStorage are handled safely.
In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java`:
- Around line 343-377: The async callback passed to
MessageManager.updateChannelConsent currently calls requireContext() to re-fetch
the channel via ConnectMessagingDatabaseHelper.getMessagingChannel before
checking isAdded(), which can throw if the fragment is detached; move the call
to ConnectMessagingDatabaseHelper.getMessagingChannel(requireContext(),
channelId) inside the if (isAdded()) block (and likewise any calls that use
requireContext(), e.g., Toast.makeText and requireActivity().invalidateMenu())
so that requireContext()/requireActivity() are only invoked after isAdded() has
returned true; keep calls that must run regardless (if any) outside the
isAdded() guard and ensure
activity.dismissProgressDialogForTask(ConnectConstants.NETWORK_ACTIVITY_MESSAGING_CHANNEL_ID)
is still called from the UI-thread-safe, isAdded-guarded section.
---
Nitpick comments:
In `@app/src/org/commcare/adapters/ChannelAdapter.java`:
- Around line 98-120: Unread badge is being shown regardless of consent; update
the visibility logic so the unread count (binding.tvUnreadCount) is only
displayed when unread > 0 AND the channel is consented (channel.getConsented()).
Concretely, change the showUnread condition or add a surrounding consent check
in the bind block that sets tvUnreadCount text and visibility (referencing
binding.tvUnreadCount and channel.getConsented()) so that unsubscribed channels
always hide the unread badge.
In `@app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java`:
- Around line 96-148: The menu ID-to-analytics mapping is created in
setupMenuItems() even though the actual MenuItem additions are commented out;
remove or defer that initialization so we don't create unreachable mappings.
Update setupMenuItems(): move the menuItemsAnalyticsParamsMapping assignment
into the same branch where menu.add(...) would be executed (inside
onCreateMenu), or simply delete the Map.of(...) until the
MENU_UNSUBSCRIBE/MENU_RESUBSCRIBE additions are re-enabled; ensure
onMenuItemSelected still guards use of menuItemsAnalyticsParamsMapping if it may
be null. Reference symbols: setupMenuItems, onCreateMenu,
menuItemsAnalyticsParamsMapping, onMenuItemSelected, MENU_UNSUBSCRIBE,
MENU_RESUBSCRIBE.
app/src/org/commcare/connect/database/ConnectMessagingDatabaseHelper.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java
Show resolved
Hide resolved
Hid unread count if channel is unsubscribed.
Moved DB fetch to isAdded() check
4 participants
CCCT-2085
Product Description
I gave the the "unsubscribe" action real functionality and added some UI tweaks for the unsubscribed state for messaging channels. Here's a video showing the unsubscribe functionality successfully working and updating the UI state, along with what it looks like if there is an error:
Screen_Recording_20260218_140043_CommCare.Debug.mp4
Edit:
Here's an updated video where I reuse the already existing network loading dialog instead:
Screen_Recording_20260219_140931_CommCare.Debug.mp4
Technical Summary
I deleted the
fromMessagePayload()function fromConnectMessagingMessageRecordas it's no longer being used anywhere. And, I removed a lot of code regarding the user consenting to messaging channels given that channels are now auto-consented - this includes the deletion of few string resources that are no longer being used.Also, it was noted in the ticket comments that an error top bar is not in the scope of this ticket, so I just added a temporary toast error message for now (for errors when calling the API to unsubscribe).
Safety Assurance
Safety story
I've verified the following:
QA Plan
Verify all the same things listed under "Safety Story" above.