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.

Sign up for GitHub

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

Undo event deletion #149

Closed
wants to merge 2 commits into from
Closed

Undo event deletion #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce14199
fix timezone service test
georgehrke Oct 22, 2016
10a0eb8
undo event deletion
georgehrke Oct 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions js/app/controllers/calcontroller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ app.controller('CalController', ['$scope', 'Calendar', 'CalendarService', 'VEven
VEventService.create(calendar, data).then(function(vevent) {
if (calendar.enabled) {
fc.elm.fullCalendar('refetchEventSources', calendar.fcEventSource);
fc.elm.fullCalendar('removeEvents', function(fcEvent) {
return fcEvent.artificiallyRendered;
});
}
});
}
Expand Down Expand Up @@ -232,9 +235,25 @@ app.controller('CalController', ['$scope', 'Calendar', 'CalendarService', 'VEven
createAndRenderEvent(result.calendar, result.vevent.data, view.start, view.end, $scope.defaulttimezone);
}
}).catch(function(reason) {
if (reason === 'delete') {
deleteAndRemoveEvent(vevent, fcEvent);
if (reason !== 'delete') {
return true;
}

fc.elm.fullCalendar('removeEvents', fcEvent.id);
fcEvent.delete().then(function() {
return VEventService.delete(vevent).catch(function() {
throw new Error();
});
}).catch(function() {
const start = view.start.subtract(1, 'day');
const end = view.end.add(1, 'day');

const eventsToRender = vevent.getFcEvent(start, end, $scope.defaulttimezone);
angular.forEach(eventsToRender, function (event) {
event.artificiallyRendered = true;
fc.elm.fullCalendar('renderEvent', event);
});
});
Copy link
Member

@wrexroad wrexroad Oct 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Rerendering undeleted events like this causes them to double render when another event is created. I know that is a confusing way to word it so I took a short video.
Double render

Fortunately, this is simply a rendering artifact and resolves itself on reload.

If you rerender the events like this:
fc.elm.fullCalendar('refetchEventSources', vevent.calendar.fcEventSource);
it does not appear to cause this problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

I tried using refetchEventSources, but I think it's a bit problematic. The event takes at least one second to reappear, because the browser has to send a CalDAV request to the server.
It would be way better, if the event would reappear immediately.

});
},
eventResize: function (fcEvent, delta, revertFunc) {
Expand Down
38 changes: 28 additions & 10 deletions js/app/models/fcEventModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
*
*/

app.factory('FcEvent', function(SimpleEvent) {
app.factory('FcEvent', function($timeout, SimpleEvent) {
'use strict';

/**
Expand All @@ -33,6 +33,7 @@ app.factory('FcEvent', function(SimpleEvent) {
function FcEvent(vevent, event, start, end) {
const context = {vevent, event};
context.iCalEvent = new ICAL.Event(event);
context.isDeleted = false;

let id = context.vevent.uri;
if (event.hasProperty('recurrence-id')) {
Expand Down Expand Up @@ -192,17 +193,34 @@ app.factory('FcEvent', function(SimpleEvent) {
};

/**
* lock fc event for editing
* Show undo notification for deletion
* @returns {Promise}
*/
iface.lock = function() {
context.lock = true;
};
iface.delete = function() {
return new Promise(function(resolve, reject) {
context.isDeleted = true;

/**
* unlock fc event
*/
iface.unlock = function() {
context.lock = false;
const timeout = $timeout(function() {
if (context.isDeleted) {
resolve();
}
}, 7500);
Copy link
Member

Choose a reason for hiding this comment

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

7.5 seconds is too high IMHO. The user has got plenty of time to switch to another app, resulting in the deletion action not executing (correct ?).

I don't know if there's a way to execute queue actions when we're leaving the app. Leaving the browser could be handled though, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Member Author

Choose a reason for hiding this comment

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

(https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload)

Copy link
Member

Choose a reason for hiding this comment

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

Seems good. So undo actions should be in a queue that gets emptied on timeout or on the beforeunload event.


const msg = t('calendar', '<strong>{title}</strong> has been deleted. <strong>Undo?</strong>', {
title: iface.title
});
const html = $('<div/>').append($(msg));

const elm = OC.Notification.showTemporary(html, {
isHTML: true
});
angular.element(elm[0]).click(function() {
context.isDeleted = false;
OC.Notification.hide(elm);
$timeout.cancel(timeout);
reject('Deletion cancelled by user');
});
});
};

return iface;
Expand Down
111 changes: 109 additions & 2 deletions tests/js/unit/models/fcEventModelSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
describe('The FullCalendar Event factory', function () {
'use strict';

let FcEvent, SimpleEvent;
let FcEvent, SimpleEvent, $timeout, $rootScope;

const ics1 = `BEGIN:VCALENDAR
VERSION:2.0
Expand Down Expand Up @@ -221,11 +221,23 @@ END:STANDARD
END:VTIMEZONE`)));
ICAL.TimezoneService.register('Europe/Berlin', tzBerlin);

OC.Notification = {};
OC.Notification.hide = jasmine.createSpy();
OC.Notification.showTemporary = jasmine.createSpy();

$provide.value('SimpleEvent', SimpleEvent);
}));

beforeEach(inject(function (_FcEvent_) {
beforeEach(inject(function (_FcEvent_, $q, _$timeout_, _$rootScope_) {
FcEvent = _FcEvent_;
$timeout = _$timeout_;
$rootScope = _$rootScope_;

// mixing ES6 Promises and $q ain't no good
// ES6 Promises will be replaced with $q for the unit tests
if (window.Promise !== $q) {
window.Promise = $q;
}
}));

it ('should initialize correctly', function() {
Expand Down Expand Up @@ -1085,4 +1097,99 @@ DTEND;TZID=America/New_York:20161106T015900
END:VEVENT`.split("\n").join("\r\n"));
expect(vevent.touch).toHaveBeenCalled();
});

it('should resolve the delete promise after 7.5s when not cancelled', function() {
let called = false;
const comp = new ICAL.Component(ICAL.parse(ics3));
const vevent = {
calendar: {
color: '#000',
textColor: '#fff',
tmpId: '3.1415926536',
isWritable: jasmine.createSpy()
},
uri: 'fancy1337',
touch: jasmine.createSpy()
};
const event = comp.getFirstSubcomponent('vevent');
const start = event.getFirstPropertyValue('dtstart');

const fcEvent = FcEvent(vevent, event, start, start);

const elm = angular.element('<div/>');
const elms = [elm];

OC.Notification.showTemporary.and.returnValue(elms);

const promise = fcEvent.delete();

expect(OC.Notification.showTemporary.calls.count()).toEqual(1);
expect(OC.Notification.showTemporary.calls.argsFor(0)[0].html()).toEqual('<strong>{title}</strong> has been deleted. <strong>Undo?</strong>');
expect(OC.Notification.showTemporary.calls.argsFor(0)[1]).toEqual({isHTML: true});

$timeout.flush(7499);
expect(promise.$$state.status).toEqual(0);

promise.then(function() {
called = true;
}).catch(function() {
fail();
});

$timeout.flush(1);
expect(promise.$$state.status).toEqual(1);
expect(called).toEqual(true);
});

it('should reject the delete promise when cancelled by the user', function() {
let called = false;
const comp = new ICAL.Component(ICAL.parse(ics3));
const vevent = {
calendar: {
color: '#000',
textColor: '#fff',
tmpId: '3.1415926536',
isWritable: jasmine.createSpy()
},
uri: 'fancy1337',
touch: jasmine.createSpy()
};
const event = comp.getFirstSubcomponent('vevent');
const start = event.getFirstPropertyValue('dtstart');

const fcEvent = FcEvent(vevent, event, start, start);

const elm = angular.element('<div/>');
const elms = [elm];

OC.Notification.showTemporary.and.returnValue(elms);

const promise = fcEvent.delete();

expect(OC.Notification.showTemporary.calls.count()).toEqual(1);
expect(OC.Notification.showTemporary.calls.argsFor(0)[0].html()).toEqual('<strong>{title}</strong> has been deleted. <strong>Undo?</strong>');
expect(OC.Notification.showTemporary.calls.argsFor(0)[1]).toEqual({isHTML: true});

$timeout.flush(7499);
expect(promise.$$state.status).toEqual(0);

promise.then(function() {
fail();
}).catch(function() {
called = true;
});

angular.element(elm).click();

$rootScope.$apply();

expect(called).toEqual(true);
expect(promise.$$state.status).toEqual(2);
expect(OC.Notification.hide).toHaveBeenCalledWith(elms);

$timeout.flush(1);
$rootScope.$apply();

expect(promise.$$state.status).toEqual(2);
});
});
13 changes: 9 additions & 4 deletions tests/js/unit/services/timezoneServiceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@ describe('Timezone Service', function () {
}));


beforeEach(inject(function (_TimezoneService_, $httpBackend, _$rootScope_) {
beforeEach(inject(function (_TimezoneService_, $httpBackend, $q, _$rootScope_) {
TimezoneService = _TimezoneService_;
$rootScope = _$rootScope_;
$rootScope.baseUrl = 'fancy-url/';
http = $httpBackend;

// mixing ES6 Promises and $q ain't no good
// ES6 Promises will be replaced with $q for the unit tests
if (window.Promise !== $q) {
window.Promise = $q;
}
}));

afterEach(function () {
Expand Down Expand Up @@ -63,8 +69,7 @@ describe('Timezone Service', function () {
called = true;
});

window.setTimeout(function() {
expect(called).toBe(true);
}, 1000);
$rootScope.$apply();
expect(called).toBe(true);
});
});