Change the notification's action to redirect to the settings page

[jvillafanez]

Fixes #73 (requires owncloud/notifications#212 support)

There is still minor things to do (mainly unittests), but code should work.

[codecov-io]

Codecov Report

Merging #78 into master will decrease coverage by 3.67%.
The diff coverage is 12.69%.

@@             Coverage Diff              @@
##             master      #78      +/-   ##
============================================
- Coverage      71.8%   68.13%   -3.68%     
- Complexity      190      195       +5     
============================================
  Files            25       26       +1     
  Lines           876      932      +56     
============================================
+ Hits            629      635       +6     
- Misses          247      297      +50

Impacted Files	Coverage Δ	Complexity Δ
appinfo/routes.php	0% <0%> (ø)	0 <0> (ø)	⬇️
...ib/Controller/NotificationRedirectorController.php	0% <0%> (ø)	5 <5> (?)
lib/Jobs/PasswordExpirationNotifierJob.php	100% <100%> (ø)	9 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78dfd05...3972a06. Read the comment docs.

[DeepDiver1975]

Why in here? Shouldn't that all be handled in the notifications app? This feels wrong.

The password_policy app should not care about the processing of the notifications.

[DeepDiver1975]

no csrf on POST? This is critical!

[PVince81]

@DeepDiver1975 this is based on the latest design in owncloud/notifications#212

basically we need the app to perform the action before the client starts its redirect, to be able to mark the notifications as processed

maybe there's another way where the notifications app itself could provide a generic action (action class?) to invalidate a notification by id. @jvillafanez

[jvillafanez]

The password_policy app should not care about the processing of the notifications.

password_policy is the app generating the notification, and since the action targets the password_policy app, this is the one that knows when the action has been correctly executed. In case there is a temporary error, the app might decide not to remove the notification and give the user another chance.

In addition, the app can remove the notification if a proper action has been taken outside of the notification system. For example, if the user has changed the password the "password about to expire" notification should be removed regardless of the user clicking in the notification or not.

Removing the notification from the notifications app might be difficult because the removal has to go through the notification manager, and the notification manager requires a notification (https://github.com/owncloud/core/blob/32b951f72f28018935eadb43567354ea70f58081/lib/public/Notification/IApp.php#L44)
In fact we might be doing things wrong because the interface doesn't say anything about the notification, and other apps implementing the IApp interface might require an exact match, which is something we aren't doing.

Note that the action is directed against the password_policy app, not against the notifications app.

[DeepDiver1975]

This is the flow I have in mind - and this is how I understood the specification:

• password_policy app creates notification as soon as the password needs to be changed
• the created notification holds the url the browser shall be redirected to (some url in the password_policy rendering html to the user - this is already in place - or the settings url)
• the notification is dispatched to all clients
• user clicks on the notification link
• browser calls apps/notification/redirect/{id}
• notifications app marks notification as handled
• notifications app sends back the url as stored in the notification
• browser opens browser tab

[jvillafanez]

maybe there's another way where the notifications app itself could provide a generic action (action class?) to invalidate a notification by id.

I don't think it's possible with the current model. If the notifications app is the one invalidating, then all the actions should go through the notifications app in order to let the app invalidate the notification. So all the actions should have their links modified.
The problem is that the notification manager is part of core and should provide support for more notifications apps, so it shouldn't assume neither the notifications app will always be active nor it will be the only one active. Core needs to be app agnostic, and this won't be true if it needs to overwrite the action links.

the created notification holds the url the browser shall be redirected to (some url in the password_policy rendering html to the user - this is already in place - or the settings url)

The question is where such url is in the notification. We can't put it as link because clients are expected to make an ajax request there, not to redirect.

browser calls apps/notification/redirect/{id}

A couple of questions here:

• how that the client knows that it needs to call that endpoint? because the password_policy app shouldn't create an action targeting another app...
• it seems you're assuming that the notifications app is the only app that shows notifications, but core support more of these apps, so what if I use "myawesomeNotificationViewer" instead of the "notifications" app?