452610 - Disable CalDAV scheduling (make pref-based)

Status: VERIFIED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Daniel Boelzle [:dbo]]

Since CalDAV scheduling is too immature for 0.9, we will switch it off by default (pref setting).

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

Comment 2

[Daniel Boelzle [:dbo]]

In the patch, disabled scheduling disables caldav free-busy, too. Should we use free-busy even though we don't use/ignore inbox/outbox scheduling purposes?

Comment 3

[Bruno Browning]

Comment on attachment 335878 [details] [diff] [review]
patch - v1


>+// whether CalDAV (experimental) scheduling is enabled or not.
>+pref("calendar.caldav.scheduling", false);
>+

I'd suggest that something like "calendar.caldav-sched.enable" would make the purpose/use of the pref more clear; please consider renaming it.

>     mHaveScheduling: false,
>     mShouldPollInbox: true,
>     get hasScheduling() {
>         return this.mHaveScheduling;
>     },
>+    set hasScheduling(value) {
>+        return (this.mHaveScheduling = (getPrefSafe("calendar.caldav.scheduling", false) && value));
>+    },
> 

We have two things we are tracking here: server capabilities and whether we want to use them. Unless I'm missing something I think we'd be better off letting the bootstrap/discover code store accurate information about server capabilities in .mHaveScheduling and doing a (pref && this.mHaveScheduling) in the .hasScheduling getter. If we do it in the setter mHaveScheduling won't always be correct wrt server capabilities, and changing the pref will require a restart in order to redo the discovery. If it's in the getter the change can take place immediately, and we won't be wishing we had some "restart required" UI. Or am I missing something?

One other thing missing here is that on servers where calendar-URL == inbox-URL (only SOGo, to my knowledge) even with .hasScheduling false we're liable to be finding METHOD:REPLY items returned in our calendar-queries. IMO if caldav-sched is disabled we ought to simply ignore such items. In order to do that we'll need to do a "if (!this.hasScheduling) {return;}" early in processItipReply() or possibly add the .hasScheduling check to the if() in getCalendarData() that calls processItipReply()

We do have working caldav-sched freebusy in 0.8 and I'd hate to regress that in 0.9. Testing shows that this patch does regress it, so I'm going to have to r- it for now. I suspect that this could be easily solved, if we move the prefcheck into the .hasScheduling getter and use .mHaveScheduling in the check at the beginning of getFreeBusyIntervals()

Comment 4

[Daniel Boelzle [:dbo]]

(In reply to comment #3)
> We have two things we are tracking here: server capabilities and whether we
> want to use them. Unless I'm missing something I think we'd be better off
> letting the bootstrap/discover code store accurate information about server
> capabilities in .mHaveScheduling and doing a (pref && this.mHaveScheduling) in
> the .hasScheduling getter. If we do it in the setter mHaveScheduling won't
> always be correct wrt server capabilities, and changing the pref will require a
> restart in order to redo the discovery. If it's in the getter the change can
> take place immediately, and we won't be wishing we had some "restart required"
> UI. Or am I missing something?
I don't think you're missing something. I just think diverging the meaning of the getter hasScheduling against mHaveScheduling isn't a good thing w.r.t. code quality. Moreover I don't think that the required restart is really that bad, because caldav-sched is experimental, we could require that. Anyway, can we assure the provider can cope with switching hasScheduling?

> One other thing missing here is that on servers where calendar-URL == inbox-URL
> (only SOGo, to my knowledge) even with .hasScheduling false we're liable to be
> finding METHOD:REPLY items returned in our calendar-queries. IMO if
> caldav-sched is disabled we ought to simply ignore such items. In order to do
> that we'll need to do a "if (!this.hasScheduling) {return;}" early in
> processItipReply() or possibly add the .hasScheduling check to the if() in
> getCalendarData() that calls processItipReply()
need to fix that

> We do have working caldav-sched freebusy in 0.8 and I'd hate to regress that in
> 0.9. Testing shows that this patch does regress it, so I'm going to have to r-
> it for now. I suspect that this could be easily solved, if we move the
> prefcheck into the .hasScheduling getter and use .mHaveScheduling in the check
> at the beginning of getFreeBusyIntervals()
so you prefer to go with caldav freebusy (comment #2). I can imagine leaving out the check for hasScheduling (added in the patch) would be sufficient.

Comment 5

[Daniel Boelzle [:dbo]]

Attachment: patch - v2

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 336044 [details] [diff] [review]
patch - v2

r=philipp

Comment 7

[Bruno Browning]

I think it would have been more appropriate to allow me to complete the review.

In response to comment #4, it seems to me that with this pref we are diverging the meaning of the two and that it would be helpful to make that clear. Perhaps we ought to have a .mServerSupportsScheduling and a .appDoesScheduling, or some such. I understand the code quality argument, I just think it cuts the other way. Your decision, though. 

Other than that, the patch seems fine to me, as far as it goes. Setting the new pref to true, though, is not sufficient to re-enable caldav scheduling, and we should deal with that either in this bug or another before the 0.9 release. It looks like minimally we'll need to add a canNotify() to the caldav provider and make some related changes in calendar-item-editing.js

But as far as this patch goes, and for what it's worth, r=browning

Comment 8

[Philipp Kewisch [:Fallen][☁️📆]]

I'm sorry for taking away the review, I should have read more context probably. I'll be more careful in the future.

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #7)
> In response to comment #4, it seems to me that with this pref we are diverging
> the meaning of the two and that it would be helpful to make that clear. Perhaps
> we ought to have a .mServerSupportsScheduling and a .appDoesScheduling, or some
> such. I understand the code quality argument, I just think it cuts the other
> way. Your decision, though. 
I don't yet see the need from a programmer's POV, but it only puts more burden to check both flags properly in the code.

> Other than that, the patch seems fine to me, as far as it goes. Setting the new
> pref to true, though, is not sufficient to re-enable caldav scheduling, and we
> should deal with that either in this bug or another before the 0.9 release. It
> looks like minimally we'll need to add a canNotify() to the caldav provider and
> make some related changes in calendar-item-editing.js
I don't think we need changes there. calISchedulingSupport::canNotify indicates whether the server takes care of scheduling (inbound), and thus no further outbound scheduling via iTIP needs to be done in calendar-item-editing.js etc; just calling addItem/modifyItem is enough.
canNotify() does not predict which iTIP transport is taken in case it returns false, but this can be controlled via calICalendar::getProperty("itip.transport") falling back to the iMIP transport in case no caldav sched is supported/enabled.

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 10

[Andreas Treumann]

Checked in lightning build 2008090205 and sunbird 20080901 -> VERIFIED.