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

WIP: Setting for default calendar when creating new events #2331

Closed
wants to merge 2 commits into from

Conversation

LukasBoersma
Copy link
Contributor

Signed-off-by: Lukas Boersma mail@lukas-boersma.com

Description

This adds a setting for the default calendar when creating a new event.

To summarize my changes:

  • I added a 'disabled' property to the CalendarPicker component so that the dropdown for the new setting can be disabled while the setting is saved.
  • I added a setting for the default calendar in the settings panel and all the other places where settings need to be handled (I just followed the usage of the existing settings here).
  • In the functions getCalendarObjectInstanceForNewEvent (calendarObjectInstance.js) and createNewEvent (calendarObjects.js) I added calendarId as an new (optional) parameter to specify which calendar to use for a new event.
  • In the event editor, I use that parameter to use the configured default calendar.
  • When no default calendar is specified yet, the behaviour is the same as before (i.e. the first calendar in sortedCalendars is used).

This is work in progress. I still have an issue with the calendar picker, it does only close when double clicking it, any help here is greatly appreciated. Also I have yet to write tests for this new setting. However it would be cool if somebody from the maintainers could let me know if this change has a chance of being merged.

Fixes #1835

Type of change

  • New feature (non-breaking change which adds functionality)

How to test / use your changes?

  1. Have more than one calendar in the calendar app.
  2. Change the default calendar in the settings panel.
  3. Create a new event. The previously selected default calendar will now be preselected for the new event.

UI Changes

Before After
defaultcalendar-before defaultcalendar-after

Checklist:

  • My code follows the style guidelines of this project
  • I signed off my changes (git commit -sm "Your commit message")
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Signed-off-by: Lukas Boersma <mail@lukas-boersma.com>
Signed-off-by: Lukas Boersma <mail@lukas-boersma.com>
@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #2331 into master will decrease coverage by 5.48%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2331      +/-   ##
============================================
- Coverage     27.60%   22.11%   -5.49%     
============================================
  Files           145      138       -7     
  Lines          5297     4929     -368     
  Branches        798      804       +6     
============================================
- Hits           1462     1090     -372     
- Misses         3835     3839       +4     
Flag Coverage Δ Complexity Δ
#javascript 22.11% <0.00%> (-0.12%) 0.00 <0.00> (ø)
#php ? ?
Impacted Files Coverage Δ Complexity Δ
src/components/AppNavigation/Settings.vue 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/components/Shared/CalendarPicker.vue 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/mixins/EditorMixin.js 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/store/calendarObjectInstance.js 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/store/calendarObjects.js 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/store/settings.js 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/views/Calendar.vue 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
lib/Controller/ContactController.php
lib/AppInfo/Application.php
lib/Controller/EmailController.php
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 533ad83...4e4ebca. Read the comment docs.

@LukasBoersma LukasBoersma changed the title Setting for default calendar when creating new events WIP: Setting for default calendar when creating new events Jun 13, 2020
Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

@LukasBoersma Thanks a lot for this PR. Looks good on the first look.

My only remark so far: We shouldn't store the default calendar in the calendar app itself, but in the dav app. We already have a config option for that in the server.
appId: dav, configKey: defaultCalendar
https://github.com/nextcloud/server/pull/19852/files#diff-fb3dce4f54794d914797a6c29849c11eR292
cc @tcitworld

If we use that config-key, the default calendar will serve as both the calendar for new events as well as the calendar for new scheduling invites. (That's how most calendar software, which offer this feature, handle it.)

@georgehrke georgehrke requested a review from tcitworld June 15, 2020 07:29
@georgehrke georgehrke added the 2. developing Work in progress label Jun 15, 2020
@tcitworld
Copy link
Member

Looks good!

I'd also like to have this with #2304 instead of adding another setting item in our crowded setting menu.

@LukasBoersma
Copy link
Contributor Author

@georgehrke Thanks for the feedback! I finally found some time to look at this again.

I managed to get the setting for the initial state from the dav app by loading it along with the calendar's own settings in the calendar's ViewControllers, but I can't figure out how to store the setting in the dav app the right way.

I expect that there is already an API that I can call to store the value, but I can't find it. Or should I just add a function to the calendar's SettingsController.php that stores the value for the dav app?

@georgehrke
Copy link
Member

I expect that there is already an API that I can call to store the value, but I can't find it. Or should I just add a function to the calendar's SettingsController.php that stores the value for the dav app?

@tcitworld You were working on this, right? Is there already an API?
Otherwise it's fine to use:

			$this->config->setUserValue(
 				$this->userId,
 				'dav',
 				'defaultCalendarId',
 				$value
 			);

@Bolli84
Copy link

Bolli84 commented Nov 8, 2020

With more and more collaboration in Nextcloud HUB, this is more and more improtant.

@tcitworld
Copy link
Member

@ChristophWurst Since we'll have a groupware user settings section with nextcloud/server#27466 wouldn't it make sense to have this option in Server Settings instead of the Calendar app?

@szaimen
Copy link
Contributor

szaimen commented Jun 25, 2021

@ChristophWurst Since we'll have a groupware user settings section with nextcloud/server#27466 wouldn't it make sense to have this option in Server Settings instead of the Calendar app?

I'd honestly vote against it because the setting would be very hard to discover this way. And this setting actually only correspends to the calendar so it should be in the calendar app, imo.

@ChristophWurst
Copy link
Member

I agree with @szaimen. The availability is relevant for any CalDAV client. This is only relevant for this app, right? It should be here.

It will help to have a settings modal in this app like we have in Mail and Talk. The current bottom left settings area is already quite crammed.

@tcitworld
Copy link
Member

This is only relevant for this app, right?

Nope, it's a dav setting. It should use the setting added in nextcloud/server#19852 instead of the custom one added here, like Georg said above.

@szaimen
Copy link
Contributor

szaimen commented Jul 2, 2021

Though I still think that having the setting in the calendar app is better because of better discoverability, no?

@tcitworld
Copy link
Member

I have no strong opinions on this one, other than the fact that it would be better for the setting to be available without the app installed, but who doesn't install the calendar app anyway? 😎

@szaimen
Copy link
Contributor

szaimen commented Jul 22, 2021

it would be better for the setting to be available without the app installed

You are right, makes sense because of that. But in this case I am definitely voting for adding a link in the calendar settings that links directly to the corresponding personal setting section like I've done it here: nextcloud/activity#621

@st3iny
Copy link
Member

st3iny commented Jun 21, 2022

We already have a link to the personal groupware settings. It might has to be renamed for this feature.

image

@st3iny
Copy link
Member

st3iny commented Jun 21, 2022

If we implement the setting globally (Personal Setting -> Groupware) the picker (+ its backend) has to move into the dav app at apps/dav/src/views.

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

Successfully merging this pull request may close these issues.

Defining a default calendar
7 participants