Rewrite notifications and add its tests #906
Conversation
Codecov Report
@@ Coverage Diff @@
## main #906 +/- ##
==========================================
- Coverage 73.56% 70.38% -3.18%
==========================================
Files 17 27 +10
Lines 1755 2161 +406
Branches 278 323 +45
==========================================
+ Hits 1291 1521 +230
- Misses 464 640 +176
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Try now @tupaschoal |
|
Works like a charm now, thanks :) Let me do the full review of the code |
| return createNotification(getCurrentTranslation('$Notification.time-to-leave'), [dismissBtn]) | ||
| .addListener('action', (response) => | ||
| { | ||
| // Actions are only supported on macOS |
There was a problem hiding this comment.
Did we then lose functionality of the dismiss button for Windows?
There was a problem hiding this comment.
Actions are not available on the current version of Electron, but once it's updated the commented code can be used. Otherwise we would've had to deal with two libraries
|
Hi @ochan12, have you had a chance of looking at the comments I left in the review? |
|
Hey @tupaschoal sorry, I've been a little behind with these comments, couldn't find any time to do it. Will do it during this week |
Co-authored-by: Tulio Leao <tupaschoal@gmail.com>
There was a problem hiding this comment.
Seems like a few functions are failing consistently on macos and windows tests:
✕ Should quit on click
✕ Should create notification on click
✕ Should show dialog for importing db
✕ Should show fail dialog for importing db
✕ Should show fail dialog for importing db
✕ Should not show dialog for clearing db
|
Hey @tupaschoal ! I see it. Test are passing locally for me, so gonna have to investigate further |
|
Fixed now @tupaschoal :) |
There was a problem hiding this comment.
Sorry it took us a while @ochan12 , but I'm glad we got it through and am very happy not only with the issues being fixed, but also with the great increase in code coverage, a huge thanks!
tupaschoal
changed the title
|
\changelog-update |
|
\changelog-update |
Related issue
Closes #865
Closes #883
Closes #811
Context / Background
In order to stop using SnoreToast we need to favor Electron Notifications on top of
node-notifierpackage.Also, there was an exception being thrown because it was trying to use jQuery on main process.
What change is being introduced by this PR?
Remove usage of
$("#leave-by")fromjs/notifications.jsand fetch it through ipcRenderer.Remove usage of node-notifier to use Electron Notifications to avoid having SnoreToast.exe on top.
Added event listener on notification to open app or dissmiss notifications
NOTE: Actions on windows will be disabled until Electron is upgraded at least 12.0.0. Once it's upgraded we can use the commented section with
toastXmlconstructor.How will this be tested?
I added a test for showing the notification. I had to do a little hack to emit a notification instead of showing it for
linuxtests, since it was failing. This will only happen on CI and Linux, I ran tests on linux with no problem.