-
Notifications
You must be signed in to change notification settings - Fork 242
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
feat: create example event when a user logs in for the first time #6648
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Richard Steinmetz <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6648 +/- ##
============================================
- Coverage 22.97% 22.57% -0.41%
- Complexity 475 511 +36
============================================
Files 252 259 +7
Lines 12128 12343 +215
Branches 2317 2314 -3
============================================
Hits 2786 2786
- Misses 9015 9230 +215
Partials 327 327
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This is an easy fix. Just do a PR in the DAV app to listen for the same event and create the calendar. |
$defaultDescription = <<<EOF | ||
Welcome to Nextcloud Calendar! | ||
|
||
This is a sample event - explore the flexibility of planning with Nextcloud Calendar by making any edits you want! | ||
|
||
With Nextcloud Calendar, you can: | ||
- Create, edit, and manage events effortlessly. | ||
- Create multiple calendars and share them with teammates, friends, or family. | ||
- Check availability and display your busy times to others. | ||
- Seamlessly integrate with apps and devices via CalDAV. | ||
- Customize your experience: schedule recurring events, adjust notifications and other settings. | ||
EOF; | ||
|
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 totally dislike when I see this in code, formatting strings in raw text inside the code.
What about using a multi-line concat with some PHP_EOL's lol
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.
Also this needs to be translated.
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, good point. Both summary and description should be passed through the translation function.
Are you sure that the translation logic is able to handle concatenated strings, e.g. with PHP_EOL
?
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.
Are you sure that the translation logic is able to handle concatenated strings, e.g. with
PHP_EOL
?
Yeah, you are correct that will not work. Use it as is or "blah blah blah\n".
This is actually an ancient bug in server. See my fix at: nextcloud/server#50369 |
Hey, I like the approach of listening to the FirstLoginEvent, but I would do the implementation a bit different. A. I would put the example creation code, inside the DAV app, if you think of it from a point of view, where you can have multiple calendar providers, each provider should create their own sample or use a common example, instead of only creating one example in the first calendar that might not even be the system dav calendar. Also, what if someone does not want to have the calendar web UI installed but still wants example content to show up in CalDAV. Putting the code in the CalDAV, also fixes your other issue of the calendar not existing before you add the example content. B. I would leave the front end UI part of this in the calendar app, that way someone can upload a custom example if they have the UI installed, and the calendar providers can check if a custom event is set. But i feel like this should be part of a general defaults core app. Another thought, we should probably implement localization of default and custom events, for multi national use, in instances like out that have people from all over the world working on the same instance. |
Good point. The uid replacement would also be less hacky inside serve due to the availability of Sabre. I also discussed this with Christoph and Hamza and it might be a good idea to move the code to server and integrate it into Hamza's structure at nextcloud/server#50156. Translation is not a requirement as discussed with designers and Christoph. Admins can very well upload their own events in case they want to have a custom language or simply disable the feature. However, given that the event is built inside PHP code localization could be done quite easily. Whether the event should show up even without the Calendar app should be discussed with designers. As it stand right now, this should not be the case, so disabling Calendar will also disable the example event.
I disagree about leaving it in Calendar. The settings UI is not really dependent on an app and should be moved where the backend lives to reduce complexity. Either way, I implemented a UI in the admin settings where admins can upload arbitrary ICS events. |
I agree.
Is it not requirement because no one thought of it? 😆
True. |
Shhh 😆 No for real, the original idea was to use a static ICS file so translation was not possible technically. (At least not with our current setup.) |
Resolves #6608
Houston, we got a problem: The personal (default) calendar is not yet created when the first login event is fired. The method-> fix at nextcloud/server#50369IManager::getCalendarsForPrincipal()
returns an empty array and thus the example event can't actually be created anywhere.TODO