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

Add rich object support to API #1973

Merged
merged 9 commits into from
Aug 12, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jul 9, 2024

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
Copy link
Member

nickvergessen commented Jul 9, 2024

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?

Copy link
Contributor

github-actions bot commented Jul 9, 2024

🐢 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
Copy link
Contributor Author

come-nc commented Jul 11, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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 come-nc force-pushed the enh/add-rich-object-support-to-api branch from 1d52255 to 555568b Compare August 5, 2024 13:07
@come-nc
Copy link
Contributor Author

come-nc commented Aug 5, 2024

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

@nickvergessen nickvergessen merged commit 180fac9 into master Aug 12, 2024
42 checks passed
@nickvergessen nickvergessen deleted the enh/add-rich-object-support-to-api branch August 12, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants