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

Schedule publishing and deletion time #767

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

mwinkens
Copy link

@mwinkens mwinkens commented Mar 11, 2024

Being able to schedule announcements is a wished feature from the community, this is completely optional of course

Images:

App view:
Bildschirmfoto vom 2024-03-11 14-02-56

Test user: (he only got the notification of a soon to be deleted announcement)
Bildschirmfoto vom 2024-03-11 14-13-06

Features:

  • UI for scheduling announcements (only in the future)
  • UI for schdeule announcement deletion (only in the future)
  • A background job accomplishing above
  • new database entries for schedule_time, delete_time, groups (required for publishing) and notificationOptions, so you can notifiy over emails, nc-notifications, etc. in the future

closes #187
closes #677

possible milestone for #385

@mwinkens mwinkens force-pushed the main branch 2 times, most recently from 081988d to ab42919 Compare March 11, 2024 15:17
@nickvergessen
Copy link
Member

I invited you to our org, so you can send the PR from with in, so it also automatically runs CI

@mwinkens
Copy link
Author

I invited you to our org, so you can send the PR from with in, so it also automatically runs CI

Thank you! I hope I didn't cause too much traffic

@mwinkens mwinkens force-pushed the main branch 2 times, most recently from 537406e to b1eb577 Compare March 12, 2024 12:40
@mwinkens
Copy link
Author

mwinkens commented Mar 12, 2024

@nickvergessen currently the background job is not being executed and I don't know why. When I re-enable the app, the job is added but as soon as you want to execute it, it gets deleted 🤔

I am pretty sure I set it up as described in the developer documentation, any ideas? I already set the loglevel to debug and added logging to the Service, but I didn't get any output.

mysql> select * from oc_jobs where class LIKE '%nnouncement%' ;
+-----+-------------------------------------------------+----------+------------+--------------+-------------+--------------------+----------------------------------+----------------+
| id  | class                                           | argument | last_run   | last_checked | reserved_at | execution_duration | argument_hash                    | time_sensitive |
+-----+-------------------------------------------------+----------+------------+--------------+-------------+--------------------+----------------------------------+----------------+
|  17 | OCA\NextcloudAnnouncements\Cron\Crawler         | null     | 1710231001 |   1710246001 |           0 |                  0 | 37a6259cc0c1dae299a7866489dff0bd |              1 |
| 195 | OCA\AnnouncementCenter\AnnouncementSchedulerJob | null     |          0 |   1710246080 |           0 |                  0 | 37a6259cc0c1dae299a7866489dff0bd |              1 |
+-----+-------------------------------------------------+----------+------------+--------------+-------------+--------------------+----------------------------------+----------------+
sudo -u www-data php occ background-job:list | grep nnouncement
| 17  | OCA\NextcloudAnnouncements\Cron\Crawler                         | 2024-03-12T08:10:01+00:00 | null     |

https://docs.nextcloud.com/server/latest/developer_manual/basics/backgroundjobs.html#writing-a-background-job


Edit: I found out, that the DependencyInjection is not working/not setup, because the services are not registered. This is also true for your custom Nofitier. I will implement this

https://docs.nextcloud.com/server/latest/developer_manual/basics/dependency_injection.html

@nickvergessen
Copy link
Member

Edit: I found out, that the DependencyInjection is not working/not setup, because the services are not registered. This is also true for your custom Nofitier. I will implement this

https://docs.nextcloud.com/server/latest/developer_manual/basics/dependency_injection.html

This should only be needed in very rare circumstances and in fact in the code I see no reason why it would be needed in this case

@mwinkens
Copy link
Author

mwinkens commented Mar 13, 2024

Edit: I found out, that the DependencyInjection is not working/not setup, because the services are not registered. This is also true for your custom Nofitier. I will implement this
https://docs.nextcloud.com/server/latest/developer_manual/basics/dependency_injection.html

This should only be needed in very rare circumstances and in fact in the code I see no reason why it would be needed in this case

Should I remove this again? I noticed, that at least the Notifier needed to be added there, because I got a warning

{"reqId":"B2lvZSwcW2eEm61Gr5lJ","level":3,"time":"2024-03-12T15:41:04+00:00","remoteAddr":"127.0.0.1","user":"nextcloud27","app":"notifications","method":"GET","url":"/ocs/v2.php/apps/notifications/api/v2/notifications","message":"Failed to load notification notifier class: OCA\\AnnouncementCenter\\Notification\\Notifier","userAgent":"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:122.0) Gecko/20100101 Firefox/122.0","version":"28.0.1.1","exception":{"Exception":"OCP\\AppFramework\\QueryException","Message":"Could not resolve OCA\\AnnouncementCenter\\AppInfo\\IConfig! Class \"OCA\\AnnouncementCenter\\AppInfo\\IConfig\" does not exist","Code":0,"Trace":[{"function":"OC\\AppFramework\\Utility\\{closure}","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":83,"function":"array_map"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":128,"function":"buildClass","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":146,"function":"resolve","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":468,"function":"query","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/ServerContainer.php","line":155,"function":"queryNoFallback","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":64,"function":"query","class":"OC\\ServerContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Notification/Manager.php","line":173,"function":"get","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Notification/Manager.php","line":364,"function":"getNotifiers","class":"OC\\Notification\\Manager","type":"->"},{"file":"/var/www/nextcloud28/apps/notifications/lib/Controller/EndpointController.php","line":118,"function":"prepare","class":"OC\\Notification\\Manager","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Http/Dispatcher.php","line":230,"function":"listNotifications","class":"OCA\\Notifications\\Controller\\EndpointController","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Http/Dispatcher.php","line":137,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/App.php","line":184,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Route/Router.php","line":315,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/var/www/nextcloud28/ocs/v1.php","line":65,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/var/www/nextcloud28/ocs/v2.php","line":23,"args":["/var/www/nextcloud28/ocs/v1.php"],"function":"require_once"}],"File":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","Line":114,"Previous":{"Exception":"OC\\AppFramework\\Utility\\QueryNotFoundException","Message":"Could not resolve OCA\\AnnouncementCenter\\AppInfo\\IConfig! Class \"OCA\\AnnouncementCenter\\AppInfo\\IConfig\" does not exist","Code":0,"Trace":[{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":146,"function":"resolve","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":468,"function":"query","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":440,"function":"queryNoFallback","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":64,"function":"query","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->"},{"file":"/home/marvin/workspace/announcementcenter/lib/AppInfo/Application.php","line":76,"function":"get","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":175,"function":"OCA\\AnnouncementCenter\\AppInfo\\{closure}","class":"OCA\\AnnouncementCenter\\AppInfo\\Application","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/var/www/nextcloud28/3rdparty/pimple/pimple/src/Pimple/Container.php","line":122,"function":"OC\\AppFramework\\Utility\\{closure}","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":142,"function":"offsetGet","class":"Pimple\\Container","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":462,"function":"query","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":440,"function":"queryNoFallback","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":96,"function":"query","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->"},{"function":"OC\\AppFramework\\Utility\\{closure}","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":83,"function":"array_map"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":128,"function":"buildClass","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":146,"function":"resolve","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/DependencyInjection/DIContainer.php","line":468,"function":"query","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/ServerContainer.php","line":155,"function":"queryNoFallback","class":"OC\\AppFramework\\DependencyInjection\\DIContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","line":64,"function":"query","class":"OC\\ServerContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Notification/Manager.php","line":173,"function":"get","class":"OC\\AppFramework\\Utility\\SimpleContainer","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Notification/Manager.php","line":364,"function":"getNotifiers","class":"OC\\Notification\\Manager","type":"->"},{"file":"/var/www/nextcloud28/apps/notifications/lib/Controller/EndpointController.php","line":118,"function":"prepare","class":"OC\\Notification\\Manager","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Http/Dispatcher.php","line":230,"function":"listNotifications","class":"OCA\\Notifications\\Controller\\EndpointController","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/Http/Dispatcher.php","line":137,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud28/lib/private/AppFramework/App.php","line":184,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud28/lib/private/Route/Router.php","line":315,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/var/www/nextcloud28/ocs/v1.php","line":65,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/var/www/nextcloud28/ocs/v2.php","line":23,"args":["/var/www/nextcloud28/ocs/v1.php"],"function":"require_once"}],"File":"/var/www/nextcloud28/lib/private/AppFramework/Utility/SimpleContainer.php","Line":135},"message":"Failed to load notification notifier class: OCA\\AnnouncementCenter\\Notification\\Notifier","exception":{},"CustomMessage":"Failed to load notification notifier class: OCA\\AnnouncementCenter\\Notification\\Notifier"}}

@nickvergessen
Copy link
Member

Should I remove this again? I noticed, that at least the Notifier needed to be added there, because I got a warning

Yeah remove it again. It seems there is an import of use OCP\IConfig; missing somewhere instead.

@mwinkens
Copy link
Author

@nickvergessen this features are now finished and in a usable state, I tested it manually since tests are still missing, scheduled deletion and scheduled announcing works.

Some notes/caveats, do you which for some of these things to be implemented here?:

  • Scheduled notifications might need some time, since they need to be first found by the background-scheduler-job, which then starts a queued-background-job (your job) for notifications
  • Scheduling is only in the future, but you can set the deletion date in front of the publishing date. This is partially mitigated, because announcements won't be deleted, if they are not published. However, this could be, with some extra time, implemented, by checking the selected announcement time. But this would still not work, if you select the deletion time first, then you would need to check the selected deletion time and prevent the announcement time to be set after it.
  • While this doesn't implement an archive, the same background job could check for very old (configurable) announcements and "archive them" (whatever this means)
  • Yet to be announced announcements can't be changed in time or directly published with a button. The workaround is to delete the announcement and redo it with the right date or publish it immediatly
  • Yet to be deleted annoucements the same, see above
  • Announcements without deletion date can't add one, but can ofc. be directly deleted

Next thing on my list is to add tests and call it a day

@mwinkens mwinkens marked this pull request as ready for review March 14, 2024 11:27
@mwinkens
Copy link
Author

Waiting for feedback 🥳

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, nothing really blocking, but didn't have the time to test it yet.

lib/AnnouncementSchedulerJob.php Show resolved Hide resolved
lib/AnnouncementSchedulerJob.php Outdated Show resolved Hide resolved
lib/Migration/Version6009Date20240311074015.php Outdated Show resolved Hide resolved
lib/Migration/Version6009Date20240311074015.php Outdated Show resolved Hide resolved
lib/Migration/Version6009Date20240311074015.php Outdated Show resolved Hide resolved
src/Components/NewForm.vue Outdated Show resolved Hide resolved
src/Components/NewForm.vue Outdated Show resolved Hide resolved
tests/AnnouncementSchedulerJobTest.php Outdated Show resolved Hide resolved
tests/Model/NotificationTypeTest.php Outdated Show resolved Hide resolved
tests/Service/AnnouncementSchedulerProcessorTest.php Outdated Show resolved Hide resolved
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
mwinkens and others added 12 commits March 18, 2024 09:43
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
mwinkens and others added 3 commits March 18, 2024 09:48
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Signed-off-by: mwinkens <[email protected]>
@mwinkens
Copy link
Author

@nickvergessen Thanks for your review, I implemented everything! I tested this feature again manually, and everything (new) was working as expected

@nickvergessen
Copy link
Member

What about the two points from the description:

  • Add button to schedule deletion for existing announcements
  • Add publish button for unannounced announcements

Want to address them in follow ups? Then we could potentially merge this PR here and release it already.

@mwinkens
Copy link
Author

mwinkens commented Mar 21, 2024

What about the two points from the description:

* Add button to schedule deletion for existing announcements

* Add publish button for unannounced announcements

Want to address them in follow ups? Then we could potentially merge this PR here and release it already.

I would address them in follow ups, because I already have a follow up (the banners) and the scheduled deletion would require some extra work, and should maybe be handled by a general edit option, which is still missing. Same applies if you want an other publish time

The direct publishing requires an extra route, but I'll add a follow up for this

@nickvergessen I let you resolve the conflict here, because it's only in the version, do you wish this to be 6.8.1(published on app store), 6.8.2 or 6.9.0?

@mwinkens mwinkens changed the title Schedule Annoucement annoucement and deletion Schedule publishing and deletion time Mar 21, 2024
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@mwinkens
Copy link
Author

Hello @nickvergessen, are you okay with these changes?

@nickvergessen
Copy link
Member

Sorry, I'm heavily overloaded atm.
I hope I find the time soon to have a look.

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.

[Feature request] preschedule accouncements Add option to expire notifications
2 participants