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

feat: create example event when a user logs in for the first time #6648

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Jan 22, 2025

Resolves #6608

Houston, we got a problem: The personal (default) calendar is not yet created when the first login event is fired. The method IManager::getCalendarsForPrincipal() returns an empty array and thus the example event can't actually be created anywhere. -> fix at nextcloud/server#50369

TODO

  • Make sure that the personal (default) calendar is available
  • Tests

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny added 2. developing Work in progress enhancement New feature request labels Jan 22, 2025
@st3iny st3iny self-assigned this Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 0% with 87 lines in your changes missing coverage. Please review.

Project coverage is 22.57%. Comparing base (cad954e) to head (bb61fc9).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/views/GroupwareAdminSettings.vue 0.00% 71 Missing ⚠️
src/services/exampleEventService.js 0.00% 9 Missing ⚠️
src/settingsAdminGroupware.js 0.00% 7 Missing ⚠️
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              
Flag Coverage Δ
javascript 14.37% <0.00%> (-0.15%) ⬇️
php 56.37% <ø> (-2.93%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SebastianKrupinski
Copy link
Contributor

Houston, we got a problem: The personal (default) calendar is not yet created when the first login event is fired. The method IManager::getCalendarsForPrincipal() returns an empty array and thus the example event can't actually be created anywhere.

This is an easy fix. Just do a PR in the DAV app to listen for the same event and create the calendar.

appinfo/routes.php Outdated Show resolved Hide resolved
lib/Listener/CreateExampleEventListener.php Outdated Show resolved Hide resolved
Comment on lines +70 to +82
$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;

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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".

@st3iny
Copy link
Member Author

st3iny commented Jan 23, 2025

Houston, we got a problem: The personal (default) calendar is not yet created when the first login event is fired. The method IManager::getCalendarsForPrincipal() returns an empty array and thus the example event can't actually be created anywhere.

This is an easy fix. Just do a PR in the DAV app to listen for the same event and create the calendar.

This is actually an ancient bug in server. See my fix at: nextcloud/server#50369

@SebastianKrupinski
Copy link
Contributor

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.

@st3iny
Copy link
Member Author

st3iny commented Jan 23, 2025

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.

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.

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.

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.

@SebastianKrupinski
Copy link
Contributor

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.

I agree.

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.

Is it not requirement because no one thought of it? 😆

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.

True.

@st3iny
Copy link
Member Author

st3iny commented Jan 23, 2025

Is it not requirement because no one thought of it? 😆

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create example events
2 participants