439620 - Calendars without proper provider deployment should be marked as "disabled"

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Daniel Boelzle [:dbo]]

In case a calendar provider hasn't been installed, the corresponding calendar should be marked as disabled, leaving the user the choice to remove it.

Comment 1

[Stefan Sitter [:ssitter]]

For reference: Currently only a popup error dialog "An error was encountered preparing the calendar located at [URL] for use. It will not be available." is displayed and the calendar is not listed in the calendars lists. Therefore the user can't remove the calendar in question.

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

This keeps erroneous in the list, but forces them to stay disabled (added calendar property force-disabled). Calendar property auto-enabled is set in case the calendar has been enabled before, so it's checked again on next startup.

I've used calProviderBase.js in calCalendarManager.js code which seems a bit odd to me. I'd rather prefer to consolidate calProviderUtils.js and calProviderBase.js into a single calProviderUtils.jsm instead. Any objections?

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

See also bug 428538. Do you want to take care of that bug too while you are at it?

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

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

>+    let calendarDisabled = false;
>+    if (gCalendar.getProperty("force-disabled")) {
>+        document.getElementById("calendar-enabled-checkbox").disabled = true;
>+    } else {
>+        calendarDisabled = gCalendar.getProperty("disabled");
>+        document.getElementById("calendar-enabled-checkbox").checked = !calendarDisabled;
>+    }
I'd prefer something more descriptive like "provider-missing"

>+function calDummyCalendar(type) {
>+    this.initProviderBase();
>+    this.type = type;
Please add type as a property of the prototype.


>+    {cid: null,
>+     contractid: null,
>+     script: "calProviderBase.js",
>+     constructor: null},
We should consider using js modules for this file.


r=philipp

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> (From update of attachment 355545 [details] [diff] [review])
> >+    let calendarDisabled = false;
> >+    if (gCalendar.getProperty("force-disabled")) {
> >+        document.getElementById("calendar-enabled-checkbox").disabled = true;
> >+    } else {
> >+        calendarDisabled = gCalendar.getProperty("disabled");
> >+        document.getElementById("calendar-enabled-checkbox").checked = !calendarDisabled;
> >+    }
> I'd prefer something more descriptive like "provider-missing"
Hm, "provider missing" may not be the only problem.

> >+    {cid: null,
> >+     contractid: null,
> >+     script: "calProviderBase.js",
> >+     constructor: null},
> We should consider using js modules for this file.
Will do so once bug 403006 has landed.

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: patch - v2

This patch depends on the patch on bug 403006.

Consolidating calProviderUtils.js/calProviderBase.js into calProviderUtils.jsm. Since this requires quite some amount of simple adoptions in other files, I am re-requesting review for this patch.

More QA wanted of course, especially on caldav!

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

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

>+cal.prepHttpChannel = function calPrepHttpChannel(aUri, aUploadData, aContentType, aNotificationCallbacks, aExisting) {
no extra namespace this time?


>+/**
>+ * getInterface method for providers. This should be called in the context of
>+ * the respective provider, i.e
>+ *
>+ * return calInterfaceRequestor_getInterface.apply(this, arguments);
>+ *
>+ * or
>+ * ...
>+ * getInterface: calInterfaceRequestor_getInterface,

>+cal.InterfaceRequestor_getInterface = function calInterfaceRequestor_getInterface(aIID) {

Please update the comment block.

>+    /**
>+ * Gets the configured identity and account of a particular calendar instance, or null.
Strange indentation

>+cal.ProviderBase.mTransientProperties = {};
>+["cache.uncachedCalendar", "currentStatus",
>+ "itip.transport", "imip.identity", "imip.account",
>+ "imip.identity.disabled", "organizerId", "organizerCN"].forEach(
>+    function(prop) {
>+        cal.ProviderBase.mTransientProperties[prop] = true;
>+    });
I'd prefer setting directly, it might make more lines but is more readable:
cal.ProviderBase.mTransientProperties = {
   "cache.uncachedCalendar": true,
   "currentStatus": true,
   ...
};



>+    get observers() {
Go ahead and unanonymize the getters/setters while you are at it.

>+//         ASSERT(this.mProperties.toSource() == "({})", "setProperty calls before id has been set!");
stray comment.

>+        // xxx todo: move this code hack when migrating storage prefs to moz prefs,
>+        //           presumably with bug 378754
This comments looks like it belongs removed in the other bug.


>+    __proto__: cal.ProviderBase.prototype,
Almost forgot this one: We should stick to camelCase, i.e cal.providerBase.protoType, or rename it to baseProvider. The same goes for the InterfaceRequestor.

>+++ b/calendar/installer/removed-files.in
>@@ -424,8 +424,11 @@ js/calWcapErrors.js
>+
>+calendar-js/calProviderBase.js
I'd love to see a bug# referenced for this change.

>     loadUtils: function cDCM_loadUtils() {
>+        Components.utils.import("resource://calendar/modules/calProviderUtils.jsm");
>+        cal.loadScripts(["calUtils.js", "calAuthUtils.js", "calDavCalendar.js" ],
We should think about this: unless we use functions from those imported files in the module itself, I think it makes more sense to do the Cu.import in calDavCalendar.js, as you are probably doing anyway.


r=philipp

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: patch - v3

(In reply to comment #8)
> (From update of attachment 355757 [details] [diff] [review])
> >+cal.prepHttpChannel = function calPrepHttpChannel(aUri, aUploadData, aContentType, aNotificationCallbacks, aExisting) {
> no extra namespace this time?
I don't think so. There's no specific one that fits IMO.

> >+/**
> >+ * getInterface method for providers. This should be called in the context of
> >+ * the respective provider, i.e
> >+ *
> >+ * return calInterfaceRequestor_getInterface.apply(this, arguments);
> >+ *
> >+ * or
> >+ * ...
> >+ * getInterface: calInterfaceRequestor_getInterface,
> 
> >+cal.InterfaceRequestor_getInterface = function calInterfaceRequestor_getInterface(aIID) {
> 
> Please update the comment block.
done

> >+    /**
> >+ * Gets the configured identity and account of a particular calendar instance, or null.
> Strange indentation
done

> >+cal.ProviderBase.mTransientProperties = {};
> >+["cache.uncachedCalendar", "currentStatus",
> >+ "itip.transport", "imip.identity", "imip.account",
> >+ "imip.identity.disabled", "organizerId", "organizerCN"].forEach(
> >+    function(prop) {
> >+        cal.ProviderBase.mTransientProperties[prop] = true;
> >+    });
> I'd prefer setting directly, it might make more lines but is more readable:
> cal.ProviderBase.mTransientProperties = {
>    "cache.uncachedCalendar": true,
>    "currentStatus": true,
>    ...
> };
done

> >+    get observers() {
> Go ahead and unanonymize the getters/setters while you are at it.
done

> >+        // xxx todo: move this code hack when migrating storage prefs to moz prefs,
> >+        //           presumably with bug 378754
> This comments looks like it belongs removed in the other bug.
removed

> >+    __proto__: cal.ProviderBase.prototype,
> Almost forgot this one: We should stick to camelCase, i.e
> cal.providerBase.protoType, or rename it to baseProvider. The same goes for the
> InterfaceRequestor.
I don't think so; ProviderBase serves as a base class, so we should capitalize it. We don't need prefixes any longer :)

> >+++ b/calendar/installer/removed-files.in
> >@@ -424,8 +424,11 @@ js/calWcapErrors.js
> >+
> >+calendar-js/calProviderBase.js
> I'd love to see a bug# referenced for this change.
done

> >     loadUtils: function cDCM_loadUtils() {
> >+        Components.utils.import("resource://calendar/modules/calProviderUtils.jsm");
> >+        cal.loadScripts(["calUtils.js", "calAuthUtils.js", "calDavCalendar.js" ],
> We should think about this: unless we use functions from those imported files
> in the module itself, I think it makes more sense to do the Cu.import in
> calDavCalendar.js, as you are probably doing anyway.
done

updated patch, taking forward r+

Comment 10

[Stefan Sitter [:ssitter]]

I think such calendars should have a tooltip or information text in the properties dialog indicating the issue. For example "Failed to instantiate calendars of type "gdata". Probably the corresponding provider extension is disabled or has been removed from Sunbird/Thunderbird."

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #10)
> I think such calendars should have a tooltip or information text in the
> properties dialog indicating the issue. For example "Failed to instantiate
> calendars of type "gdata". Probably the corresponding provider extension is
> disabled or has been removed from Sunbird/Thunderbird."

This is handled in bug 428538.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: patch - v4

Found another regression: calAuthPrompt undefined. Migrating calUtils.js code into calAuthUtils.jsm... This patch gets longer and longer ;-)

Re-requesting review because of that.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 355972 [details] [diff] [review]
patch - v4

Changes look fine, r=philipp. I hope we have all needed files now :-)

Comment 14

[Daniel Boelzle [:dbo]]

Phew, a good amount of consolidation into js modules.. good for silent regressions: watch out!

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/1ff3aca86da6>

-> FIXED

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".