Feature: Desktop notifications

[boedy]

The possibility to get email notifications is nice, but a bit overkill for my need. I wouldn't want my inbox to get littered with daily backup reports. I suspect that a desktop notification will give me enough feedback to go on. Some design considerations:

• Alert on error
• Notify on success?
• Enabled by default?

The https://github.com/gen2brain/beeep package could be implemented to get desktop notifications working. @jeffaco If you like I could draft a PR. Would first like to have some consensus on its requirements though.

[jeffaco]

Hmm. Interesting idea.

Just FYI: Note that my inbox isn't littered with daily backup reports, I only get notifications on failures. Otherwise, the E-Mail is sidelined and put in the trash (removed after 30 days). Unless I look for it, I never see successful backup reports. For details, see: Management of E-Mail Messages.

I'm totally open to this. It should be a fairly simple change, pretty isolated, and Beeep looks like a fine package to use on first perusal. Note that, unless you use some default icons, duplicacy-util doesn't have any icons so probably can't do that. But that's minor.

In general, I like duplicacy-util to not be in your face except on error. So my view: Extend the global initialization file with the following fields:

notifications:
    enabled: true or false (default: true)
    success: true or false (default: false)
    failure: true or false (default: true)

Unless otherwise configured, duplicacy-util would only notify on error. This makes sense given the rest of the philosophy behind duplicacy-util.

When you run an E-Mail test, duplicacy-util should also test notifications, if enabled. That way, it's easy to tell if they're working properly. If you click on a notification, it would be cool if the notification could pull up the log file in Preview or something, but I didn't see that mentioned in the README for Beeep, so I don't know if that's possible or not.

I've also considered some sort of SMS support. A few times, when I've gotten behind in E-Mail, I would have appreciated some sort of SMS message, or better yet, push notification, to my cell phone. That way, I'd be aware even if I was falling behind in E-Mail. But that's orthogonal to this.

I like this suggestion because, if I'm at my system but not up on E-Mail, I'd see the notification as well and take appropriate action. @boedy If you'd like to submit a PR for this, I'm totally open to that.

Do you feel that the requirements that I've sketched out are reasonable?

[jeffaco]

@boedy What are your thoughts on my feedback?

[boedy]

Sorry for not getting back to you sooner. Had some work to catch up on. Thanks for info on the email notification part. I jumped to conclusions there. I should play with it a bit more to see if it fits me flow.

I think putting a notification config it in the global initialisation file works. But you should be able choose through which channels you get notified. I like the push notifications idea. This would be my preferred approach for headless systems or computers I don't access often. In the future you could possible add extra notification channels. Pushbullet, Slack or Webhook support would be on the top of my list.

I realised it would be good to have an onStart notification. I regularly work on the go where I use my phones cellular connection. I wouldn't want duplicacy burning🔥 through my monthly bandwidth then. Would be nice to skip the backup by clicking on the notification. Another way of solving this would be whitelisting / blacklisting WiFi SSID's, but thats out of scope for this feature. This is what I ended up with for now:

notifications:
    channel: desktop, email (default: email, desktop)
    events:
        onSuccess: true or false (default: false)
        onFailure: true or false (default: true)
        onStart: true or false (default: false)

How's that? Would the cli option flags -em and -tm change as well?

Beeep uses simple system wrappers. I did some testing on my macbook and noticed custom icons are not supported with the current implementation (see gen2brain/beeep#15). I haven't tested the the other platforms. Beeep also does not support custom click handlers. gosx-notifier is an alternative package that features both images and click handlers which I think will allow you to pull up the log. It unfortunately supports macOS only though.

[jeffaco]

Hi @boedy Thanks for your response.

I think channels provide a fine way extend notifications in the future, and I like the concept, even if the first channels are just desktop and email. However, I assume if a channel isn't mentioned it would simply never be enabled? I wonder if people might want different notifications specified on a per-channel basis. For example, perhaps I want onSuccess for email but not for desktop. Thus, perhaps a better method might be:

notifications:
    1:
        channel: desktop
        events:
            ...
    2:
        channel: email
        events:
            ...

This allows you to specify an event mask on a per-channel basis, which might be preferred. And I guess, if you don't specify a channel at all, then it would never be notified.

There is no -em qualifier, you probably mean -m (to send mail). I think that -m has no purpose with the notification section that you defined above. However, it's defined today, so I'd probably keep it for backwards compatibility (at least until a v2.0 release of duplicacy-util). Thus, if you didn't specify -m, then I'd look to the channels to see if email was enabled, and how to handle it. If -m was specified on the command line, that would server to override the email channel such that it was enabled for both success and failure (which is what it does now). I think that makes the most sense.

The -tm (test mail) would probably persist for backwards compatibility (again, until a v2.0 release, if that ever happens). I'd probably add a new -te (test events) qualifier that would test each of the events that you set up for both success and failure (much like -tm does today, but just for e-mail).

I'm totally uninterested in implementing something that only works on a single platform. Part of the entire focus of duplicacy-util is that it runs everywhere with no fuss. But that said, if we supported Beeep for most platforms, but supported gosx-notifier for richer capabilities on Mac OS/X, that's totally fine with me (and only partly because I use a Mac). Honestly, if certain platforms give richer capabilities, that's fine, as long as the basic capability still works properly on other platforms.

Thoughts?

[boedy]

@jeffaco Looks good. I do find the config you propose a bit verbose though. What about this:

notifications:
  onSuccess: ["desktop"]
  onFailure: ["desktop","email", "pushbullet", "slack"]
  onStart: ["desktop"]

email:
  fromAddress: "Donald Duck <[email protected]>"
  toAddress: "Donald Duck <[email protected]>"
  serverHostname: smtp.gmail.com
  serverPort: 465
  authUsername: [email protected]
  authPassword: gaozqlwbztypagwt

slack:
  token: "342u34tu0xunqru2x5y23234234fdsfah4f"

pushBullet:
  token: "342u34tu0xunqru2x5y23234234fdsfah4f"

We would stick to the old configuration style for email to stay backward compatible for now. If channels is not configure correctly but defined in the notification section an error should be raised.

[jeffaco]

I was thinking of making those email changes anyway.

As for the formatting: I like your formatting, but I had difficulty with this sort of thing with Viper. If you can make Viper digest that in a reasonable way, I'm fine with your formatting.

If not, then I'd probably add event-specific data as part of the notification, like this:

notifications:
    1:
        channel: email
        events:
            onSuccess: true or false (default: false)
            onFailure: true or false (default: true)
            onStart: true or false (default: false)
        fromAddress: "Donald Duck <[email protected]>"
        toAddress: "Donald Duck <[email protected]>"
        serverHostname: smtp.gmail.com
        serverPort: 465
        authUsername: [email protected]
        authPassword: gaozqlwbztypagwt

Then, once we read in the channel type, we'd know what to read in from there, and data for each of the notifiers would just be bundled with the notification itself.

Since you were willing to do a pull request, may I suggest you add a notify.go implementation that reads in the new notification section (to be called from the globalConfiguration.go file). Once globalConfiguration.go sees a notifications section, it can dispatch off to notify.go. And these days, when writing new code, I always write unit tests for new code. So I'd humbly suggest that, if you could manage it. See existing code for examples.

I think a good start would be to get this working for E-Mail, then add other event types (like desktop first, and then others as we see fit). But E-Mail comes first, and sets the framework for things to follow.

Thoughts?

[boedy]

I had already started working on this before your suggestions, but it seems we were thinking along the same lines 😄. I thought it would be good to share my progress thus far as this is actually my first Golang project I'm contributing to (could be I'm doing things the non "Go" way)!

PR: #14

Let me know what you think.

[jeffaco]

I think your progress is awesome so far! I'm absolutely fine with what you're doing with your first Golang contribution.

I believe in "baby steps" rather than a massive PR. So, I'd like to see you finalize the framework for notifications (without adding desktop). Once the framework is committed, then we can start adding various notifiers as we see fit. (Hint: What's a good way for push notifications on a cell phone? 😃 )

To finalize, I'd need a few changes:

1. You refactored sending test mail. That's fine, but at this point it should probably be moved from duplicacy-util.go to sendmail.go. I learned go on this project, and overloading everything into duplicacy-utill.go was a mistake (and made testing impossible). If you look at changes since then, I've been refactoring to other files and adding tests at the same time.

2. Unit tests are not passing. Those need to pass, or this can't be merged to master. Note that, depending on platform, you can build with make now, and make test will run the unit tests too. (You'll likely need to go get a few packages for that to work.)

3. You're referencing a test file that doesn't appear to exist (test/assets/doesnotexit.yml). This should be added, unless it's never supposed to exist, in which case it probably shouldn't reference test/assets, but something more obvious, like no/such/path/nosuchfile.yml (along with a comment to that regard).

Great start so far. Please finalize just this work (with more unit tests as you see fit), and let's get it in! Once I have a PR that passes tests, I'll do a formal review. Thanks!

[boedy]

Thanks for the great feedback! I overlooked the Makefile and ran go test, which didn't throw any errors. Good to know I need to use make. Based on travis is see there are also still some linting / code style rules I need to adhere to. I'll try to finished up this week.

Push notifications for mobile is usually use PushBullet (a 3rd party app). I also use it if for my DB backups in production.

[jeffaco]

You don't have to use make, but I do believe in CI, and CI does more stuff than just go test. I wrote GNUmakefile to reduce the likelihood that I'd be surprised/bitten by CI failures. Indeed, over a bit of time, CI now just does a make test operation after setting up the environment.

If I had my druthers (solely based on what's useful to me, not necessarily others - I'm not sure what is priority is for this feature for others), I'd like to see:

1. Finalize this review and get it committed to master. That review should include relevant changes to README.md for the changes in setting up E-Mail. This is a great framework to easily build upon.

2. This is probably big enough to warrant a new release. In the release notes, document the changes for email setup. I'm not sure if/when I'll pull support for the old e-mail format, but I'd definitely like people to start using the new format so we have that option down the road. I'll get off my guff and complete checkpointing, which is something that would be helpful from time to time (although lately, things have been reliable enough for me where the checkpointing code would never kick in). That definitely warrants a new release (along with your changes).

3. Consider having duplicacy-util issue a warning (not a failure), included in logs and e-mail, if e-mail settings are in the old format. That way, people are less likely to miss it and will be much more likely to make the changes. The changes are easy, and I don't want to support the old format forever (it's just code clutter). Something like: Warning: E-Mail configuration in old format, update global configuration file, perhaps?

4. Notification support for push notifications. That would be awesome cool. Even if I'm too busy to be keeping up on e-mail (or even be on the computer), I'd still be aware of failures. I wonder how hard it would be to set up Google to do a push notification if duplicacy-util stops running in the daily script that runs? Any ideas on that? Getting reliable push notification of failures (where a failure is either of: duplicacy-util doesn't run, or it runs but fails) would be a great addition.

5. Notification support for desktop notifications.

These would probably complete my personal needs for notifications, although I recognize that others may want other forms of notifications and am happy to take PRs to support that.

Finally, I'd like to offer that we become co-developers of duplicacy-util. Its fine if you don't want that. But it would be nice to have someone review my code changes and test my code changes prior to committing to master (in addition to CI, I'm a big believer in code reviews, even for me). You obviously care about duplicacy-util and use it (enough to make pretty significant changes, anyway). This way, there's more than one person answering questions, and if something critical comes up, you could handle it if I'm away (you'd have direct repo access). Something I'd like you to think about, if you're amenable to that.

Thanks again for your efforts in this area.

[boedy]

After PR #14 gets merged (which tackles a couple of points you mentioned) I have a couple of things myself I would like to see implemented in this order:

1. WiFi blacklisting - I've actually already implemented this and have PR ready to submit after PR Groundwork for multiple notification channels #14 is merged. This feature allows me to specify my mobile hotspot WiFi SSID which will let duplicacy-util skip a schedule backup.
2. Push notification support - If the WiFi blacklisting feature gets accepted I would personally have less need for enabling onStart desktop notifications, as for me it was mostly tied to my network usage. I still think is has its purpose though! On the implementation side it might be good to address this in a separate issue?
3. Desktop notifications - As discussed in this issue

As for you request. I feel honoured for you asking me to become a co-maintainer for this project! Although I personally think this would be very good to improve my skills, I'm not sure I'll have the time it requires as I'm also very busy with running a start-up. To be clear; I'm open to try it, but I won't be able to able to make any promises on my involvement over time unfortunately.

[jeffaco]

WiFi blacklisting is a great idea. I don't need that because I don't back up my laptop, but perhaps I should! 😄

Of course, push notifications is near and dear to my heart. Desktop notifications would be nice as well.

How about, after this merge, I set you up as a maintainer of the project, and we'll need an accepted code review to check in (me too). Could you commit to timely code reviews, by chance? If so, that would be great. If not, then I'll just continue along my way (no sweat making you a maintainer, though) as things have been. I really like another set of eyes on code changes, though, and even for duplicacy-util, I think it's valuable. Let me know how you feel thanks.

[jeffaco]

I'm going to close this issue. We don't have desktop notifications as such, but we have a new notification framework that has been merged to master. This allows us to easily expand the notification framework, and @boedy, since you're planning to open Pull Requests, that's a good way to track new features.

This is super awesome. I really like the work you've done. Thanks so much!

[jeffaco]

@boedy FYI, I sent you email at the address listed in the commits for the repo. I'm not sure if you normally monitor that email address, so I thought I'd mention it here. When you have a chance, please respond to the email, thanks!