Add rich object support to API

[come-nc]

Add rich object support to both subject and message in the API to create notifications from an admin account.

TODO:

• [ ] Notifier should be backward compatible with old notification in DB (so support key 0 instead of 'parsed')
• [ ] Should this be moved to an other endpoint or a new apiVersion number to make it simpler (ie not have both shortMessage and richSubject but only one of them and so on) ?
• [ ] Fix tests and add some for the new features
• [ ] Should we allow to set icon?
• [ ] Add support for adding actions, in a followup PR

[nickvergessen]

Anything that is doing rich objects sounds like it should have it's own notifier and not abuse this one?

Maybe you can give a sample of what you are trying to do?

[github-actions[bot]]

🐢 Performance warning. It looks like the query count of the integration tests increased with this PR. Database query count is now 8555 was 8200 (+4.32%) Please check your code again. If you added a new test this can be expected and the base value in tests/Integration/base-query-count.txt can be increased.

[come-nc]

Anything that is doing rich objects sounds like it should have it's own notifier and not abuse this one?

I do not see as abuse to be able to use notifications features through the API. It allows external applications and scripts to benefit from them.

Maybe you can give a sample of what you are trying to do?

This is for the future worflows using windmill, to allow them to add notifications with links by using the highlight rich object type. But I can also see usecases for file and user types.

On the same note, what is the reason for the public API to not return the notification id when it’s created, and allow to delete it if it gets obsolete?

[nickvergessen]

I do not see as abuse to be able to use notifications features through the API. It allows external applications and scripts to benefit from them.

Yes and no, app is kind of "dumb". It just passes things on. So there is no validation if you can still access a file, the user got deleted, etc. That's why the current take always was: if you are an app, you have PHP code anyway, you should register your own provider and handle the notifications yourself.

But yeah I can see how it makes sense for highlight and some other things, but we basically have to trust admins to not make a mess. if they provide wrong/invalid file data the frontend and clients might break during rendering.

Should this be moved to an other endpoint or a new apiVersion number to make it simpler (ie not have both shortMessage and richSubject but only one of them and so on) ?

If you keep the current parameter names it could also be possible to just add shortParameters and longParameters as optional parameters additionally, then it wouldn't break BC and could be added to the existing endpoint

what is the reason for the public API to not return the notification id

The main issue is that there are multiple apps that registered as OCP\Notification\IApp and therefore the OCP\Notification\IManager::notify() call we do, can not return a single id. Would need to run a separate SELECT afterwards or globally store the lastInsertId in my handler and take it from there afterwards.

[marcelklehr]

if you are an app, you have PHP code anyway, you should register your own provider and handle the notifications yourself.

ExApps and clients do not have PHP code and cannot register their own providers.

if they provide wrong/invalid file data the frontend and clients might break during rendering.

Isn't this already a problem that should be handled in clients?

[nickvergessen]

On the same note, what is the reason for the public API to not return the notification id when it’s created, and allow to delete it if it gets obsolete?

So I locally started to add the id, but it's a bit weird. Admins don't have an option to delete a notification for another user :P Something to add I guess

[come-nc]

Rebased and fixed the tests, this looks good enough for merging in my opinion.