-
Notifications
You must be signed in to change notification settings - Fork 24
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
base: main
Are you sure you want to change the base?
Conversation
081988d
to
ab42919
Compare
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 |
537406e
to
b1eb577
Compare
@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
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
|
Yeah remove it again. It seems there is an import of |
@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?:
Next thing on my list is to add tests and call it a day |
Waiting for feedback 🥳 |
There was a problem hiding this 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.
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
…is added Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
Signed-off-by: Marvin Winkens <[email protected]>
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]>
Signed-off-by: Marvin Winkens <[email protected]>
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]>
Signed-off-by: Marvin Winkens <[email protected]>
@nickvergessen Thanks for your review, I implemented everything! I tested this feature again manually, and everything (new) was working as expected |
What about the two points from the description:
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 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 |
Hello there, 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! |
Hello @nickvergessen, are you okay with these changes? |
Sorry, I'm heavily overloaded atm. |
Being able to schedule announcements is a wished feature from the community, this is completely optional of course
Images:
App view:
Test user: (he only got the notification of a soon to be deleted announcement)
Features:
schedule_time
,delete_time
,groups
(required for publishing) andnotificationOptions
, so you can notifiy over emails, nc-notifications, etc. in the futurecloses #187
closes #677
possible milestone for #385