Added calendar-user preferences. #288

ben-denham · 2014-01-06T01:14:26Z

These changes allow a user to hide/show and change the display name and colour of a calendar that is shared with them, without affecting how the calendar is displayed to other users.

It does this by storing calendar-user preferences for each calendar a user has access to (whether it is owned by them or shared with them or a group they belong to) in a new "clndr_calendar_user_preferences" database table.

Therefore, the "cldnr_calendars.active" column has been removed, because that setting is now stored per user in "clndr_calendar_user_preferences.active". However, the "cldnr_calendars.calendarcolor" and "cldnr_calendars.displayname" have been retained to serve as default preferences for the calendar.

When a user loses access to a calendar (e.g. it is unshared with them), their preferences are retained in the database. That way, if the user regains access to a calendar that used to be shared with them, their old preferences will still be applied to the calendar.

Because retaining these unused preferences could create a lot of obsolete rows in the database, a new button has also been added in the calendar settings for administrators to "Delete unused calendar preferences", which will delete any preferences where the user no longer has access to the calendar.

This fixes owncloud/calendar#21 and owncloud/calendar#131, and was mentioned in owncloud/calendar#265 (comment).

georgehrke · 2014-01-07T14:15:55Z

Thanks
a few notes:

I think we shouldn't remove the active field in clndr_calendars. Everytime we changed the databasestructure of existing tables, there was a major fuckup (just search the core repo for database migration issues)
"_calendar_user_preferences" is too long for oracledb, please use smth shorter

@DeepDiver1975 What do you think about removing this field?

georgehrke · 2014-01-07T14:16:17Z

and "_calendar_user_preferences" should be a key-value store

ben-denham · 2014-01-08T02:52:10Z

Thanks for the feedback @georgehrke!

I've shortened the table name to clndr_user_preferences instead of clndr_calendar_user_preferences.

I have also refactored clndr_user_preferences to be a key-value store.

Would you like me to put the active field back in clndr_calendars now, or should I wait until @DeepDiver1975 has replied?

Cheers!

georgehrke · 2014-01-08T05:28:35Z

Would you like me to put the active field back in clndr_calendars now, or should I wait until @DeepDiver1975 has replied?

Let's wait for @DeepDiver1975 's opinion

jancborchardt · 2014-01-08T10:34:54Z

Are there any design changes, i.e. is my review necessary?

ben-denham · 2014-01-08T20:20:23Z

@jancborchardt I wouldn't say there are any design changes, other than the addition of the "Delete unused calendar preferences" button in the calendar settings.

jancborchardt · 2014-01-09T10:13:51Z

@ben-denham hm, strange button. Do we need that? What does it do? Would a user ever need to click it – and if so, what for? :)

georgehrke · 2014-01-09T18:01:20Z

@ben-denham hm, strange button. Do we need that? What does it do? Would a user ever need to click it – and if so, what for? :)

Because retaining these unused preferences could create a lot of obsolete rows in the database, a new button has also been added in the calendar settings for administrators to "Delete unused calendar preferences", which will delete any preferences where the user no longer has access to the calendar.

Those should be deleted automatically. There are sharing hooks for it.

jancborchardt · 2014-01-09T18:38:29Z

Yes, that should all be automatic. No additional settings for users or admins for this needed. :) @ben-denham can you change that?

ben-denham · 2014-01-09T23:06:09Z

Ok, calendar-user preferences are now deleted automatically when:

The calendar is deleted (unchanged in latest commits).
The user is deleted (unchanged in latest commits).
The calendar is unshared with the user.
The calendar is unshared with a group the user is a member of.
A group is deleted that the calendar is shared with and the user is a member of.
The user is removed from a group that the calendar is shared with.

However, preferences are only deleted if the user does not have access to the calendar in some other way (e.g. being the owner or a having a second share) :)

jancborchardt · 2014-01-10T10:34:21Z

Cool, so if now there’s no interface change anymore I can at least give my design-:+1: :)

georgehrke · 2014-01-13T13:09:54Z

@butonic What was the limit for table names in oracle? 26 chars?

butonic · 2014-01-13T21:35:41Z

@georgehrke 30 chars

jancborchardt · 2014-01-31T14:06:29Z

@georgehrke so what’s your call on this?

@rysiekpl can you review this as well?

tanghus · 2014-03-02T18:20:35Z

@ben-denham Sorry for the long delay. Can I get you to rebase to master?

Shortened table name from clndr_calendar_user_preferences to clndr_user_preferences Initial conversion of calendar-user preferences to use a key-value store. Updated the update hook to work with the key-value stored preferences. Fixed SQL in update hook. Fixed minor bugs with key-value calendar-user preferences implementation. Fixed mainly indentation and comments. Calendar-user preferences are now automatically cleaned when a user loses access to a calendar. Also fixed minor bugs with creating default calendar-user preferences. Removed unused cleanpreferences.php

ben-denham · 2014-03-02T21:01:50Z

@tanghus Just rebased to master.

tanghus · 2014-03-02T21:11:23Z

Thanks @ben-denham :)

tanghus · 2014-03-04T19:01:29Z

lib/calendar.php

@@ -249,7 +397,7 @@ public static function touchCalendar($id) {
 	 */
 	public static function deleteCalendar($id) {
 		$calendar = self::find($id);
-		if ($calendar['userid'] != OCP\User::getUser() && !OC_Group::inGroup(OCP\User::getUser(), 'admin')) {


Removing the admin check means you'll get an error when admin deletes a user possibly causing other cleanup hooks to fail. Since OC_User::isAdminUser(OC_User::getUser()) is not in the public namespace, this is the only way to check.

Just noticed that I used non-public OC_Group here. Old code ;)

Ok I'll put this back.

Do you know what the public equivalent of OC_Group is? I can't find anything like it in core's lib/public

tanghus · 2014-03-04T19:21:35Z

@ben-denham Generally the code looks good, but the direct manipulation of the share table is a no-go.

Instead use something like this:

Add a method in lib/hooks.php:

    public static function calendarDeletion($parameters) {
        // Clean up sharing
        \OCP\Share::unshareAll('calendar', $parameters['id']);
}

And in appinfo/app.php add:

OCP\Util::connectHook('OC_Calendar', 'deleteCalendar', 'OC_Calendar_Hooks', 'calendarDeletion');

vgezer · 2014-04-12T01:54:58Z

Any updates on this? I really would like to see this on OC7 :)

jancborchardt · 2014-04-12T08:34:13Z

@ben-denham yeah, me too. :) Can you rebase this branch?

ben-denham · 2014-04-23T22:40:04Z

@tanghus Ok, I have fixed the typos, and I have stopped using the oc_share table directly.

However, when using the public share API, we cannot get a list of calendars that are shared with a particular group, which has caused me to make two key changes:

When a user is removed from a group, or a group is deleted, we must check all calendars that a user has preferences for to see if any preferences need to be removed (i.e. The user no longer have permission to access the calendar), rather than just checking the calendars that were shared with the group.
When a user is added to a group, there is no way to know what new calendars may be shared with them through the group, so we do not know which calendars to add default preferences for. Therefore, I have changed the logic so that whenever a calendar is being retrieved (through allCalendars() or find()), we will add default preferences if no preferences currently exist. The default preferences will only be added the first time the calendar is retrieved for the user.
- Because we now add default preferences this way, I have removed the query that would add preferences when the calendar app is updated.

Also, as you say that OC_Group is private and should not be used, do you know the correct way to check if a user is an admin? It seems that OCP\User::checkAdminUser() would redirect the user to the home page if they were not an admin.

tanghus · 2014-04-23T22:50:56Z

@georgehrke can you take over from here?

rwasserstrom · 2014-05-28T21:45:10Z

lib/calendar.php

@@ -52,10 +45,19 @@ public static function allCalendars($uid, $active=false) {
 				| OCP\PERMISSION_READ | OCP\PERMISSION_UPDATE
 				| OCP\PERMISSION_DELETE | OCP\PERMISSION_SHARE;
 			$row['description'] = '';
-			$calendars[] = $row;


I am looking at my owncloud files (version 0.6.3) and I don't see a lot of these lines of code anywhere in the files. Is there a reason why I see something different from what you have? Thanks

rwasserstrom · 2014-05-28T21:46:33Z

I am looking in our files and we don't have the same code for a lot of these calendar files. Our owncloud is 6.3, fresh install. Please advise...

vgezer · 2014-05-28T22:00:13Z

@rwasserstrom This is an unmerged branch. You will be seeing this feature when it is merged.

rwasserstrom · 2014-05-29T14:12:32Z

Ok, so can we help implement this branch? We would like to help since we need this feature.

vgezer · 2014-05-29T16:12:34Z

@rwasserstrom You can fork https://github.com/ben-denham/calendar and work on shared-calendar-preferences branch. You may want to talk to @ben-denham @georgehrke as they were working on this recently.

georgehrke mentioned this pull request Mar 3, 2014

[WIP] redesign calendar app #359

Merged

9 tasks

tanghus reviewed Mar 4, 2014
View reviewed changes

vgezer mentioned this pull request Apr 11, 2014

Allow only owner of calendar edit shared calendars #385

Closed

vgezer added the Enhancement label Apr 12, 2014

jancborchardt assigned georgehrke Apr 24, 2014

jancborchardt added this to the ownCloud 7 milestone Apr 24, 2014

jancborchardt added the Engineering label Apr 24, 2014

rwasserstrom reviewed May 28, 2014
View reviewed changes

This was referenced May 30, 2014

Feature Request : Changing color of a shared calendar owncloud-archive/apps#377

Closed

Feature Request : Disable/enable view of a shared calendar in my calendar window. owncloud-archive/apps#376

Closed

georgehrke removed the Engineering label Aug 7, 2014

georgehrke force-pushed the master branch from b5aefcc to a723bf9 Compare September 2, 2014 13:04

DeepDiver1975 modified the milestones: ownCloud 7, backlog Oct 12, 2015

ben-denham unassigned georgehrke Mar 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added calendar-user preferences. #288

Added calendar-user preferences. #288

ben-denham commented Jan 6, 2014

georgehrke commented Jan 7, 2014

georgehrke commented Jan 7, 2014

ben-denham commented Jan 8, 2014

georgehrke commented Jan 8, 2014

jancborchardt commented Jan 8, 2014

ben-denham commented Jan 8, 2014

jancborchardt commented Jan 9, 2014

georgehrke commented Jan 9, 2014

jancborchardt commented Jan 9, 2014

ben-denham commented Jan 9, 2014

jancborchardt commented Jan 10, 2014

georgehrke commented Jan 13, 2014

butonic commented Jan 13, 2014

jancborchardt commented Jan 31, 2014

tanghus commented Mar 2, 2014

ben-denham commented Mar 2, 2014

tanghus commented Mar 2, 2014

tanghus Mar 4, 2014

tanghus Mar 4, 2014

ben-denham Apr 22, 2014

tanghus commented Mar 4, 2014

vgezer commented Apr 12, 2014

jancborchardt commented Apr 12, 2014

ben-denham commented Apr 23, 2014

tanghus commented Apr 23, 2014

rwasserstrom May 28, 2014

rwasserstrom commented May 28, 2014

vgezer commented May 28, 2014

rwasserstrom commented May 29, 2014

vgezer commented May 29, 2014

Added calendar-user preferences. #288

Are you sure you want to change the base?

Added calendar-user preferences. #288

Conversation

ben-denham commented Jan 6, 2014

georgehrke commented Jan 7, 2014

georgehrke commented Jan 7, 2014

ben-denham commented Jan 8, 2014

georgehrke commented Jan 8, 2014

jancborchardt commented Jan 8, 2014

ben-denham commented Jan 8, 2014

jancborchardt commented Jan 9, 2014

georgehrke commented Jan 9, 2014

jancborchardt commented Jan 9, 2014

ben-denham commented Jan 9, 2014

jancborchardt commented Jan 10, 2014

georgehrke commented Jan 13, 2014

butonic commented Jan 13, 2014

jancborchardt commented Jan 31, 2014

tanghus commented Mar 2, 2014

ben-denham commented Mar 2, 2014

tanghus commented Mar 2, 2014

tanghus Mar 4, 2014

Choose a reason for hiding this comment

tanghus Mar 4, 2014

Choose a reason for hiding this comment

ben-denham Apr 22, 2014

Choose a reason for hiding this comment

tanghus commented Mar 4, 2014

vgezer commented Apr 12, 2014

jancborchardt commented Apr 12, 2014

ben-denham commented Apr 23, 2014

tanghus commented Apr 23, 2014

rwasserstrom May 28, 2014

Choose a reason for hiding this comment

rwasserstrom commented May 28, 2014

vgezer commented May 28, 2014

rwasserstrom commented May 29, 2014

vgezer commented May 29, 2014