-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test notification factory #1899
Conversation
putExtra(Settings.EXTRA_CHANNEL_ID, channelID) | ||
} | ||
startActivity(intent) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not used and the user story will probably never be re-introduced in EXA
@@ -166,7 +159,7 @@ class RoomGroupMessageCreator @Inject constructor( | |||
inSpans(StyleSpan(Typeface.BOLD)) { | |||
append(roomName) | |||
append(": ") | |||
event.senderName | |||
append(event.senderName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug fixed here!
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1899 +/- ##
===========================================
+ Coverage 64.81% 66.44% +1.63%
===========================================
Files 1308 1308
Lines 33071 33059 -12
Branches 7087 7087
===========================================
+ Hits 21434 21966 +532
+ Misses 8311 7733 -578
- Partials 3326 3360 +34 ☔ View full report in Codecov by Sentry. |
Kudos, SonarCloud Quality Gate passed! |
@@ -61,6 +62,8 @@ class DefaultNotificationDrawerManager @Inject constructor( | |||
private val buildMeta: BuildMeta, | |||
private val matrixClientProvider: MatrixClientProvider, | |||
) : NotificationDrawerManager { | |||
private var appNavigationStateObserver: Job? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name observer sounds a bit weird to me, but why not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes...
Add unit tests around notification.
Fix a small bug, see the comment.
Tests will be improved, in particular on DefaultNotificationDrawerManager. Will do in a separate PR.
Hopefully it will increase the test coverage.