Feature: Display course wide notifications management
#646
Conversation
eylulnc
changed the title
Feature: Display course notifications management
eylulnc
changed the title
Feature: Display course notifications managementFeature: Display course wide notifications management
There was a problem hiding this comment.
Thank you for enabling the new notifications for Android :)
I have a few code comments and one other remark: In the app settings (accessible from the dashboard), the user can still access the old (disabled) notification settings. We can remove these as part of this PR as we now have the replacement :D
| TopAppBar( | ||
| title = { Text(stringResource(R.string.notification_settings)) }, | ||
| navigationIcon = { | ||
| IconButton(onClick = onNavigateBack) { |
There was a problem hiding this comment.
We have a own composable for this: NavigationBackButton :)
| BasicDataStateUi( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .padding(padding), |
There was a problem hiding this comment.
Add .consumeWindowInsets() here
| ) { | ||
| Card( | ||
| shape = MaterialTheme.shapes.medium, | ||
| colors = CardDefaults.cardColors(containerColor = MaterialTheme.colorScheme.surfaceContainer), |
There was a problem hiding this comment.
I think this line could be removed (let me know if not)
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .background(MaterialTheme.colorScheme.surfaceContainer) |
There was a problem hiding this comment.
Same for this one background color
| Card( | ||
| shape = MaterialTheme.shapes.medium, | ||
| colors = CardDefaults.cardColors(containerColor = MaterialTheme.colorScheme.surfaceContainer), | ||
| elevation = CardDefaults.cardElevation(defaultElevation = 0.dp) |
There was a problem hiding this comment.
Also the default elevation should be 0.dp if I'm not mistaken
| import de.tum.informatics.www1.artemis.native_app.feature.coursenotifications.course_notification_model.NotificationSettings | ||
| import de.tum.informatics.www1.artemis.native_app.feature.coursenotifications.course_notification_model.NotificationSettingsInfo | ||
|
|
||
| interface CourseNotificationSettingsService { |
There was a problem hiding this comment.
add : LoggedInBasedService here
| ) { serverUrl, authToken, _ -> serverUrl to authToken } | ||
| .flatMapLatest { (serverUrl, authToken) -> | ||
| retryOnInternet(networkStatusProvider.currentNetworkStatus) { | ||
| courseNotificationSettingsService.getNotificationSettingsInfo( |
There was a problem hiding this comment.
And then make use the of the performAutoReloadingNetworkCall method here and for the other network calls in this viewmodel
| ) | ||
| } | ||
| } | ||
| .stateIn(viewModelScope + coroutineContext, SharingStarted.Eagerly, DataState.Loading()) |
There was a problem hiding this comment.
There is also a stateIn method for DataStates that does not need the last parameter. (.stateIn(viewModelScope + coroutineContext, SharingStarted.Eagerly))
| <string name="settings_preset_description_all_activity">Receive all notifications to stay on track at all times.</string> | ||
| <string name="settings_preset_description_ignore_all">Receive no notifications for the course.</string> | ||
|
|
||
| <string name="setting_disclaimer">These settings manage push notifications. To manage Email notifications, please use a browser.; |
There was a problem hiding this comment.
The ; at the end is by mistake?
Problem Description
The course-wide notification management is not possible like ios and web.
Changes
Add icon to top bar, on click to bell icon display course wide notification management.
Steps for testing
Screenshots
Screen.Recording.2025-06-15.at.10.51.57.mov