feat: Design & Implementation of B2C subscription banner - #223
feat: Design & Implementation of B2C subscription banner#223kalalmounesh wants to merge 21 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a dismissible “Subscriptions” informational banner to Discovery (native + webview) and Profile screens, backed by remote config and persisted dismissal/session tracking so the banner can reappear across app sessions up to a configured limit.
Changes:
- Introduces
SubscriptionAlertBannerViewModel,SubscriptionBannerStorage, andAppSessionTrackerto control banner visibility/dismissal across app sessions. - Adds a new Compose
SubscriptionBannerUI and wires it into Profile and Discovery UIs with dismiss actions. - Extends app config/domain models + DI wiring to support the subscription banner feature.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| profile/src/test/java/org/openedx/profile/presentation/profile/ProfileViewModelTest.kt | Updates tests to inject the new SubscriptionAlertBannerViewModel dependency. |
| profile/src/main/java/org/openedx/profile/presentation/profile/ProfileViewModel.kt | Adds banner URL/visibility/dismiss delegation to SubscriptionAlertBannerViewModel. |
| profile/src/main/java/org/openedx/profile/presentation/profile/ProfileFragment.kt | Wires banner visibility refresh on resume and passes banner params/actions into ProfileView. |
| profile/src/main/java/org/openedx/profile/presentation/profile/compose/ProfileView.kt | Renders SubscriptionBanner in the Profile UI and adds dismiss action. |
| discovery/src/test/java/org/openedx/discovery/presentation/WebViewDiscoveryViewModelTest.kt | Updates tests to inject the new banner view model dependency. |
| discovery/src/test/java/org/openedx/discovery/presentation/NativeDiscoveryViewModelTest.kt | Updates tests to inject the new banner view model dependency. |
| discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryViewModel.kt | Adds banner URL/visibility/dismiss delegation for web discovery. |
| discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryFragment.kt | Displays the banner above the webview and handles dismiss; refreshes visibility on resume. |
| discovery/src/main/java/org/openedx/discovery/presentation/NativeDiscoveryViewModel.kt | Adds banner URL/visibility/dismiss delegation for native discovery. |
| discovery/src/main/java/org/openedx/discovery/presentation/NativeDiscoveryFragment.kt | Displays the banner in native discovery and handles dismiss; refreshes visibility on resume. |
| core/src/test/java/org/openedx/core/system/AppSessionTrackerTest.kt | Adds tests for global session counting and per-screen dismissal reset behavior. |
| core/src/test/java/org/openedx/core/presentation/SubscriptionAlertBannerViewModelTest.kt | Adds unit tests for banner visibility/dismiss logic and URL retrieval. |
| core/src/openedx/org/openedx/core/ui/theme/Colors.kt | Adds subscription banner color tokens for the openedx flavor. |
| core/src/edx/org/openedx/core/ui/theme/Colors.kt | Adds subscription banner color tokens for the edx flavor. |
| core/src/main/res/values/strings.xml | Adds banner title/content strings. |
| core/src/main/java/org/openedx/core/ui/theme/Theme.kt | Wires new banner colors into AppColors palettes. |
| core/src/main/java/org/openedx/core/ui/theme/AppColors.kt | Extends AppColors with banner background and stroke colors. |
| core/src/main/java/org/openedx/core/ui/IAPUI.kt | Adds the reusable SubscriptionBanner Compose component (and preview). |
| core/src/main/java/org/openedx/core/system/AppSessionTracker.kt | Introduces app foreground/background session tracking to reset per-screen dismissed flags. |
| core/src/main/java/org/openedx/core/presentation/SubscriptionAlertBannerViewModel.kt | Implements visibility, dismissal counting, and URL retrieval logic. |
| core/src/main/java/org/openedx/core/domain/model/AppConfig.kt | Adds subscriptionBannerConfig to domain AppConfig. |
| core/src/main/java/org/openedx/core/data/storage/SubscriptionBannerStorage.kt | Adds storage contract for banner session/dismiss tracking. |
| core/src/main/java/org/openedx/core/data/model/AppConfig.kt | Adds subscription_banner parsing and mapping into domain config. |
| core/src/main/java/org/openedx/core/config/SubscriptionBannerConfig.kt | Introduces subscription banner remote config model and defaults. |
| core/src/main/java/org/openedx/core/config/Config.kt | Adds config accessors/keys for subscription banner config parsing. |
| core/build.gradle | Adds MockK test dependency for the core module tests. |
| app/src/main/java/org/openedx/app/OpenEdXApp.kt | Registers a process lifecycle observer to drive AppSessionTracker. |
| app/src/main/java/org/openedx/app/di/ScreenModule.kt | Updates Koin viewModel definitions to pass banner dependencies. |
| app/src/main/java/org/openedx/app/di/AppModule.kt | Adds DI bindings for SubscriptionBannerStorage, AppSessionTracker, and SubscriptionAlertBannerViewModel. |
| app/src/main/java/org/openedx/app/data/storage/PreferencesManager.kt | Implements SubscriptionBannerStorage persistence via SharedPreferences. |
Suppressed comments (1)
discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryFragment.kt:329
- Kotlin does not allow positional arguments after a named argument. SubscriptionBanner is called with a named argument (visible = ...) and then positional args, which will not compile. Use named arguments for modifier/url.
visible = true,
Modifier.fillMaxWidth(),
subscriptionBannerUrl,
onDismiss = onDismissSubscriptionBanner,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| certificatePreviewHeading = light_certificate_preview_headline, | ||
| certificatePreviewMessage = light_certificate_preview_message, | ||
| subscriptionBannerBackground = light_subscription_banner_background, | ||
| subscriptionBannerBorderStroke = dark_banner_stroke, | ||
| ) |
There was a problem hiding this comment.
We implemented it as per the requirement.
| <string name="banner_content">Subscription features aren\'t supported in the mobile app just yet. To access your subscription, please log in to your account via a desktop or mobile web browser?</string> | ||
|
|
||
|
|
||
| </resources> |
| override fun onStart(owner: LifecycleOwner) { | ||
| super.onStart(owner) | ||
| } |
| override fun onStart(owner: LifecycleOwner) { | ||
| super.onStart(owner) | ||
| } |
There was a problem hiding this comment.
check this file, is this view model? since it is not extended as view model.
if it is view model then move the file to respective folder from presentation folder.
if not rename the file.
| single<SubscriptionBannerStorage> { get<PreferencesManager>() } | ||
|
|
||
| single { AppSessionTracker(get<CorePreferences>(), get<SubscriptionBannerStorage>()) } | ||
| factory { SubscriptionAlertBanner(get<CorePreferences>(), get<SubscriptionBannerStorage>()) } |
There was a problem hiding this comment.
Here factory creates a new instance every time it's injected
Different screens (Profile, Discovery) get different instances of SubscriptionAlertBanner
This breaks the intended behavior:
When one screen checks banner visibility, another screen checks independently
Config changes aren't shared across instances
Memory waste (creating multiple instances for same logic)
Expected: Banner visibility should be consistent across all screens from a single source
| class OpenEdXApp : Application() { | ||
|
|
||
| private val config by inject<Config>() | ||
| private val appSessionTracker by inject<AppSessionTracker>() |
There was a problem hiding this comment.
AppSessionTracker is injected into OpenEdXApp and observes lifecycle
But no direct connection between session changes and UI updates
Fragments only check banner visibility on ON_RESUME
Make AppSessionTracker expose observable state instead of only maintaining internal state.
There was a problem hiding this comment.
We can ignore this because session definition is tied to app foregrounding, and when the app comes back, the fragment usually resumes too. So in practice, the UI gets refreshed at the right moment most of the time.
| val uiMessage by viewModel.uiMessage.observeAsState() | ||
| val refreshing by viewModel.isUpdating.observeAsState(false) | ||
| val lifecycleOwner = LocalLifecycleOwner.current | ||
| val bannerVisibilityState = androidx.compose.runtime.remember { |
There was a problem hiding this comment.
Creates local state instead of using ViewModel state
Duplicated logic across Profile & Discovery (2 fragments)
Manual lifecycle observation when ViewModel could handle it
State loss on configuration changes
Approach: Move to ProfileViewModel as a Flow/StateFlow
There was a problem hiding this comment.
The banner visibility state has been moved from fragment-local Compose state into each screen's ViewModel as a StateFlow,
| return false | ||
| } | ||
|
|
||
| val visible = true |
There was a problem hiding this comment.
Check this if we are using it any where , if not kindly remove it.
LEARNER-11275
LEARNER-11276
LEARNER-11293
LEARNER-11294