-
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
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 <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
…is added Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Hello @nickvergessen, are you okay with these changes? |
Sorry, I'm heavily overloaded atm. |
lib/Model/Announcement.php
Outdated
$this->addType('scheduleTime', 'int'); | ||
$this->addType('deleteTime', 'int'); | ||
$this->addType('notTypes', 'int'); |
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.
groups is not registered here, it's string on the database?
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.
Yes, it has to be, because when the announcement will be, well, announced in the future, it needs to notifiy the groups, so they need to be stored somewhere
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.
yeah I more meant to register all fields, even if they have the "default" type
} | ||
|
||
if (!$table->hasColumn('announcement_delete_time')) { | ||
$table->addColumn('announcement_delete_time', Types::INTEGER, [ |
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.
most likely we should have an index on both time columns as we select by them?
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.
I unfortunately don't know how to do this 🤔
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.
Yeah no problem for now.
lib/Model/Announcement.php
Outdated
// you can't create groups with linebreaks (and if so you deserve it) | ||
// TODO use better seperator (maybe <sep>?) OR restructure program here |
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.
you want to change to that? I think it'd be better
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> Signed-off-by: mwinkens <104770531+mwinkens@users.noreply.github.com>
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> Signed-off-by: mwinkens <104770531+mwinkens@users.noreply.github.com>
Maybe we should use https://www.php.net/manual/en/function.json-encode.php here, what do you think @nickvergessen ? |
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
I'd appriciate this to avoid any possibility to break with "strange" characters in group names |
So just to summarize, my proposal would be:
Raise issues for follow ups:
And then we can merge this and include it in the next RC which I need to publish for 30 compatibility first half of next week |
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
…tput Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
Signed-off-by: Marvin Winkens <m.winkens@fz-juelich.de>
@nickvergessen I finished everything on your list and opened an issue for the followup. I also reset the package files, however I'd like you to know, that for the next release I'd upgrade the packages: |
new Date(this.scheduleTime).getTime() / 1000, // time in seconds | ||
new Date(this.deleteTime).getTime() / 1000, |
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.
new Date(null).getTime()
0
So you are never sending null.
But also just a detail as in the Controller and Manager null and 0 are treated the same.
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