Notifications overhaul #15040

nickvergessen · 2019-04-10T14:17:03Z

New "state of the art" registration method for notifiers and apps, this breaks, yes. But it's intentionally to make the devs aware of all the changes. Also I will provide PRs where possible
Added return types and parameter type hints
New action type "WEB" to allow to redirect to a given page. E.g. for calls, because you can not start a call on any page (yet)
Unify the way how notifications are marked "processed" when they should be deleted, also to allow … v
Silent push notification when one is deleted.
Notification app adjustments: Send a push message when a notification was deleted notifications#318
Update all shipped notifiers
Send PRs to all other notifiers

Fix #13980

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen · 2019-07-16T11:16:31Z

Ready to review.

Would also be nice if you can check the notifications PR (see PR description) and the PRs to all affected apps (linked around the last comment from morris)

Signed-off-by: Joas Schilling <[email protected]>

MorrisJobke · 2019-07-16T14:45:38Z

Missing apps:

lib/private/Notification/Manager.php

MorrisJobke

Beside my little nitpick the code change looks good 👍

Signed-off-by: Joas Schilling <[email protected]>

MorrisJobke · 2019-07-16T15:00:43Z

Tested and works 👍

nickvergessen · 2019-07-16T15:32:39Z

Missing apps:

cospend: https://gitlab.com/eneiluj/cospend-nc/blob/master/appinfo/app.php#L21

gravatar: https://github.com/weeman1337/nextcloud-gravatar/blob/fc078a0023f926262fb65936e92aede1ebf09837/lib/AppInfo/Application.php#L108

passwords: https://git.mdns.eu/nextcloud/passwords/blob/master/src/lib/AppInfo/Application.php#L207

passman: https://github.com/nextcloud/passman/blob/f3ba9899b85f41116973d2c4f098df5a92ad0a61/appinfo/app.php#L29

phonetrack: https://gitlab.com/eneiluj/phonetrack-oc/blob/master/appinfo/app.php#L21

W2G2: https://github.com/newroco/W2G2/blob/1ae5c6177bfa7e4cb206972f63353d210e449ce0/w2g2/lib/AppInfo/Application.php#L30

Created an issue on all repos and emailed the one where I couldn't due to permissions

lib/private/Notification/Manager.php

blizzz

changes look good

skjnldsv · 2019-07-28T08:17:37Z

@nickvergessen Our deprecation guidelines is to wait 3 releases before removing the old legacy way. ~~Now on latest master everything is failing for me.~~

~~I get empty css on almost all apps that uses the old notifications way because it gets thrown before.~~

Updating spreed to latest master fixed it for me.
Though this is not the way we usually do and everyone have to wait before changing apis. Why did we suddenly approved breaking change on a single pull request without a proper deprecation process? 🤷‍♀️

Deprecated is different from removed. If you kill a method, you don't deprecate it, you remove it. Deprecation = warning but still functional! 🤔

{
  "reqId": "H90SnKlyMmWFtO2itxYU",
  "level": 3,
  "time": "2019-07-28T08:16:57+00:00",
  "remoteAddr": "172.21.0.1",
  "user": "Test1",
  "app": "no app in context",
  "method": "GET",
  "url": "/ocs/v2.php/apps/spreed/api/v1/room",
  "message": {
    "Exception": "InvalidArgumentException",
    "Message": "Notifier Talk (id: spreed) is not considered because it is using the old way to register.",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/html/apps2/spreed/lib/AppInfo/Application.php",
        "line": 136,
        "function": "registerNotifier",
        "class": "OC\\Notification\\Manager",
        "type": "->",
        "args": [
          {
            "__class__": "Closure"
          },
          {
            "__class__": "Closure"
          }
        ]
      },
      {
        "file": "/var/www/html/apps2/spreed/lib/AppInfo/Application.php",
        "line": 99,
        "function": "registerNotifier",
        "class": "OCA\\Spreed\\AppInfo\\Application",
        "type": "->",
        "args": [
          {
            "__class__": "OC\\Server"
          }
        ]
      },
      {
        "file": "/var/www/html/apps2/spreed/appinfo/app.php",
        "line": 24,
        "function": "register",
       {
  "reqId": "H90SnKlyMmWFtO2itxYU",
  "level": 3,
  "time": "2019-07-28T08:16:57+00:00",
  "remoteAddr": "172.21.0.1",
  "user": "Test1",
  "app": "no app in context",
  "method": "GET",
  "url": "/ocs/v2.php/apps/spreed/api/v1/room",
  "message": {
    "Exception": "InvalidArgumentException",
    "Message": "Notifier Talk (id: spreed) is not considered because it is using the old way to register.",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/html/apps2/spreed/lib/AppInfo/Application.php",
        "line": 136,
        "function": "registerNotifier",
        "class": "OC\\Notification\\Manager",
        "type": "->",
        "args": [
          {
            "__class__": "Closure"
          },
          {
            "__class__": "Closure"
          }
        ]
      },
      {
        "file": "/var/www/html/apps2/spreed/lib/AppInfo/Application.php",
        "line": 99,
        "function": "registerNotifier",
        "class": "OCA\\Spreed\\AppInfo\\Application",
        "type": "->",
        "args": [
          {
            "__class__": "OC\\Server"
          }
        ]
      },
      {
        "file": "/var/www/html/apps2/spreed/appinfo/app.php",
        "line": 24,
        "function": "register",
        "class": "OCA\\Spreed\\AppInfo\\Application",
        "type": "->",
        "args": []
      },
      {
        "file": "/var/www/html/lib/private/legacy/app.php",
        "line": 262,
        "args": [
          "/var/www/html/apps2/spreed/appinfo/app.php"
        ],
        "function": "require_once"
      },
      {
        "file": "/var/www/html/lib/private/legacy/app.php",
        "line": 155,
        "function": "requireAppFile",
        "class": "OC_App",
        "type": "::",
        "args": [
          {
            "__class__": "OCA\\Spreed\\AppInfo\\Application"
          }
        ]
      },
      {
        "file": "/var/www/html/lib/private/legacy/app.php",
        "line": 128,
        "function": "loadApp",
        "class": "OC_App",
        "type": "::",
        "args": [
          "spreed"
        ]
      },
      {
        "file": "/var/www/html/ocs/v1.php",
        "line": 56,
        "function": "loadApps",
        "class": "OC_App",
        "type": "::",
        "args": []
      },
      {
        "file": "/var/www/html/ocs/v2.php",
        "line": 24,
        "args": [
          "/var/www/html/ocs/v1.php"
        ],
        "function": "require_once"
      }
    ],
    "File": "/var/www/html/lib/private/Notification/Manager.php",
    "Line": 86,
    "CustomMessage": "--"
  },
  "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36",
  "version": "17.0.0.1"
} "class": "OCA\\Spreed\\AppInfo\\Application",
        "type": "->",
        "args": []
      },
      {
        "file": "/var/www/html/lib/private/legacy/app.php",
        "line": 262,
        "args": [
          "/var/www/html/apps2/spreed/appinfo/app.php"
        ],
        "function": "require_once"
      },
      {
        "file": "/var/www/html/lib/private/legacy/app.php",
        "line": 155,
        "function": "requireAppFile",
        "class": "OC_App",
        "type": "::",
        "args": [
          {
            "__class__": "OCA\\Spreed\\AppInfo\\Application"
          }
        ]
      },
      {
        "file": "/var/www/html/lib/private/legacy/app.php",
        "line": 128,
        "function": "loadApp",
        "class": "OC_App",
        "type": "::",
        "args": [
          "spreed"
        ]
      },
      {
        "file": "/var/www/html/ocs/v1.php",
        "line": 56,
        "function": "loadApps",
        "class": "OC_App",
        "type": "::",
        "args": []
      },
      {
        "file": "/var/www/html/ocs/v2.php",
        "line": 24,
        "args": [
          "/var/www/html/ocs/v1.php"
        ],
        "function": "require_once"
      }
    ],
    "File": "/var/www/html/lib/private/Notification/Manager.php",
    "Line": 86,
    "CustomMessage": "--"
  },
  "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.142 Safari/537.36",
  "version": "17.0.0.1"
}

nickvergessen · 2019-07-28T12:02:35Z

This has been discussed in length above and in the linked issues.

nickvergessen · 2019-07-28T12:05:38Z

But then again, the exception is only logged, not thrown and it should therefor also not fail somewhere resulting in such pages.

skjnldsv · 2019-07-28T14:10:40Z

@nickvergessen yes, you're right!
After upgrading spreed, it worked again (though, all my instance was broken because of this)

Nonetheless, I think we should respect the deprecation process we have as much as we can :)
But maybe we couldn't avoid this! I was just surprised ;)

Nice work though! Much cleaner code!

MorrisJobke · 2019-07-29T07:22:49Z

@nickvergessen Our deprecation guidelines is to wait 3 releases before removing the old legacy way.

@skjnldsv It's years and not releases. We are waiting 3 years until deprecated stuff is removed.

nickvergessen added enhancement 2. developing Work in progress feature: activity and notification labels Apr 10, 2019

nickvergessen added this to the Nextcloud 17 milestone Apr 10, 2019

nickvergessen self-assigned this Apr 10, 2019

nickvergessen mentioned this pull request Apr 10, 2019

Send a push message when a notification was deleted nextcloud/notifications#318

Merged

nickvergessen force-pushed the feature/13980/push-for-deleted-notifications branch from 0d70f8a to 0d2573c Compare April 12, 2019 11:49

nickvergessen force-pushed the feature/13980/push-for-deleted-notifications branch from 0d2573c to 24a5493 Compare April 30, 2019 13:54

tobiasKaminsky mentioned this pull request May 23, 2019

Handle silent delete/delete-all push notifications nextcloud/android#3969

Merged

1 task

nickvergessen mentioned this pull request Jun 21, 2019

Fix userid casting in notifications #15912

Merged

nickvergessen added 8 commits July 15, 2019 15:12

Make all interfaces strict

9b288cd

Signed-off-by: Joas Schilling <[email protected]>

Change how Notifiers and Apps are registered

9690b31

Signed-off-by: Joas Schilling <[email protected]>

Fix creation of the Manager

f376b9f

Signed-off-by: Joas Schilling <[email protected]>

Update twofactor_backupcodes Notifier

a386ece

Signed-off-by: Joas Schilling <[email protected]>

Update Federated sharing notifier

44bc697

Signed-off-by: Joas Schilling <[email protected]>

Fix detection of Notifiers

865c12a

Signed-off-by: Joas Schilling <[email protected]>

Fix new core notifier

64f6781

Signed-off-by: Joas Schilling <[email protected]>

Update shipped implementations of the INotifier

6d71e47

Signed-off-by: Joas Schilling <[email protected]>

MorrisJobke mentioned this pull request Jul 15, 2019

17.0.0 Beta 1 #16408

Merged

28 tasks

Keep the old method as a fallback and adjust the tests

55f5bc7

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen force-pushed the feature/13980/push-for-deleted-notifications branch from 24a5493 to 55f5bc7 Compare July 16, 2019 09:37

nickvergessen added the 3. to review Waiting for reviews label Jul 16, 2019

nickvergessen requested review from MorrisJobke, rullzer and blizzz July 16, 2019 11:15

nickvergessen added 2 commits July 16, 2019 13:17

Update autoloader

c3ef1cd

Signed-off-by: Joas Schilling <[email protected]>

Update unit tests

565838d

Signed-off-by: Joas Schilling <[email protected]>

MorrisJobke reviewed Jul 16, 2019

View reviewed changes

lib/private/Notification/Manager.php Outdated Show resolved Hide resolved

MorrisJobke approved these changes Jul 16, 2019

View reviewed changes

Update since to the correct version

594efca

Signed-off-by: Joas Schilling <[email protected]>

rullzer reviewed Jul 16, 2019

View reviewed changes

lib/private/Notification/Manager.php Show resolved Hide resolved

marius-wieschollek mentioned this pull request Jul 17, 2019

Support Nextcloud 17 Notifications marius-wieschollek/passwords#175

Closed

blizzz approved these changes Jul 17, 2019

View reviewed changes

MorrisJobke merged commit 5b604ea into master Jul 17, 2019

delete-merged-branch bot deleted the feature/13980/push-for-deleted-notifications branch July 17, 2019 18:22

JSoko mentioned this pull request Sep 26, 2019

NC17 Notifier Ransomware recovery (id: ransomware_detection) is not considered because it is using the old way to register. at <<closure>> undo-ransomware/ransomware_detection#22

Closed

kesselb mentioned this pull request Jun 28, 2020

Error: InvalidArgumentExpression files_sharing is using the old way to register #21613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications overhaul #15040

Notifications overhaul #15040

nickvergessen commented Apr 10, 2019 •

edited by juliusknorr

Loading

nickvergessen commented Jul 16, 2019

MorrisJobke commented Jul 16, 2019 •

edited by nickvergessen

Loading

MorrisJobke left a comment

MorrisJobke commented Jul 16, 2019

nickvergessen commented Jul 16, 2019

blizzz left a comment

skjnldsv commented Jul 28, 2019 •

edited

Loading

nickvergessen commented Jul 28, 2019

nickvergessen commented Jul 28, 2019

skjnldsv commented Jul 28, 2019

MorrisJobke commented Jul 29, 2019

Notifications overhaul #15040

Notifications overhaul #15040

Conversation

nickvergessen commented Apr 10, 2019 • edited by juliusknorr Loading

nickvergessen commented Jul 16, 2019

MorrisJobke commented Jul 16, 2019 • edited by nickvergessen Loading

MorrisJobke left a comment

Choose a reason for hiding this comment

MorrisJobke commented Jul 16, 2019

nickvergessen commented Jul 16, 2019

blizzz left a comment

Choose a reason for hiding this comment

skjnldsv commented Jul 28, 2019 • edited Loading

nickvergessen commented Jul 28, 2019

nickvergessen commented Jul 28, 2019

skjnldsv commented Jul 28, 2019

MorrisJobke commented Jul 29, 2019

nickvergessen commented Apr 10, 2019 •

edited by juliusknorr

Loading

MorrisJobke commented Jul 16, 2019 •

edited by nickvergessen

Loading

skjnldsv commented Jul 28, 2019 •

edited

Loading