Skip to content
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

Bugfix/noid/improve the notifications #50

Merged
merged 4 commits into from Sep 11, 2019

Conversation

@nickvergessen
Copy link
Member

commented Sep 11, 2019

Bildschirmfoto von 2019-09-11 11-24-07

  • When the default settings are in place:
    • Show a note that it's only shown to administrators
    • Offer a button to immediately disable the app
  • Added a primary button to open the announcement (needs to be removed when backporting to 16 and below, due to feature support)
…cation

Signed-off-by: Joas Schilling <coding@schilljs.com>
… default

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
if ($isAdmin) {
$action = $notification->createAction();
$action->setParsedLabel($l->t('Disable announcements'))
->setLink($this->url->linkToOCSRouteAbsolute('provisioning_api.AppsController.disable', ['app' => 'nextcloud_announcements']), IAction::TYPE_DELETE)

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Sep 11, 2019

Author Member

Constant is 17+ but we can use the plain string below

->setIcon($this->url->getAbsoluteURL($this->url->imagePath($this->appName, 'app-dark.svg')));
$action = $notification->createAction();
$action->setParsedLabel($l->t('Read more'))
->setLink($notification->getLink(), IAction::TYPE_WEB)

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Sep 11, 2019

Author Member

Feature is only 17+ so it needs to be removed below, but not sure if the disable button is too confusing then?

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen requested review from blizzz and ChristophWurst Sep 11, 2019
@nickvergessen

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

/backport to stable17

->setPrimary(false);
$notification->addParsedAction($action);
$message .= "\n\n" . $l->t('(These announcements are only shown to administrators)');

This comment has been minimized.

Copy link
@jancborchardt

jancborchardt Sep 11, 2019

Member
Suggested change
$message .= "\n\n" . $l->t('(These announcements are only shown to administrators)');
$message .= "\n\n" . $l->t('(These announcements are only shown to administrators.)');

Very small nitpick. ;D

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Sep 11, 2019

Author Member

I had it in place before, found it looked weird and removed it, damn :D

This comment has been minimized.

Copy link
@jancborchardt

jancborchardt Sep 11, 2019

Member

Yeah, it does look a bit strange and unfinished cause the sentences of the announcement will all have punctuation. As said, just a tiny detail. :)

Copy link
Member

left a comment

Perfect, this makes it super clear to people what this is, who sees it, and has a quick way to "unsubscribe" those are all the complaints we got.

Thanks @nickvergessen! 👏

Copy link
Member

left a comment

Good stuff @nickvergessen

@rullzer rullzer merged commit 97aae16 into master Sep 11, 2019
3 checks passed
3 checks passed
DCO DCO
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fixupbot No fixup commits found. The commit history is clean 👍
Details
@delete-merged-branch delete-merged-branch bot deleted the bugfix/noid/improve-the-notifications branch Sep 11, 2019
@backportbot-nextcloud

This comment has been minimized.

Copy link

commented Sep 11, 2019

backport to stable17 in #52

@basildane

This comment has been minimized.

Copy link

commented Sep 12, 2019

The issue is that the desktop app is not the appropriate place to send notifications like this, it's not that people don't want your notifications. I don't want my electric shaver to tell me about a change in the price of silver. It's not appropriate and it complicates my life because it makes it difficult to manage the interruptions.

The correct way to communicate with your users is an opt-in email distribution. Email is designed for this. Users are able to read emails when they want to and create rules to manage how the emails are handled. This is all circumvented and put upside down when you shove notifications through another app that is not intended for this purpose.

If people WANT to receive your notifications they will give you their email (I did). If they don't, then you should respect their privacy.

If everyone "did their own thing" and had a custom app like this for every conceivable notification, imagine how terrible that would be. It would be a cacophony of random noise and people would just "tune out".

@nickvergessen

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

Well please see the comments in nextcloud/notifications#426
You can opt out easily from this 1-in-a-year notification easily and then you can get it via email if you want, or not at all.

@basildane

This comment has been minimized.

Copy link

commented Sep 12, 2019

I know perfectly well about the setting. That's not the point. But thanks.
I was just trying to steer you guys in the right direction.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@basildane we are not talking about a cacophony of random noise. Nextcloud is a community-driven project and we want to keep it that way, so we think it’s ok that there is a once-a-year notice of the annual conference to all admins who use this open source software for free.

For anyone who does not want it, now it is even easier to unsubscribe. But getting people involved in giving back to the community is the right direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.