Closed Bug 439620 Opened 12 years ago Closed 11 years ago

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

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Duplicate of this bug: 450121
Blocks: 428538
Attached patch patch - v1 (obsolete) β€” β€” Splinter Review
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?
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #355545 - Flags: review?(philipp)
See also bug 428538. Do you want to take care of that bug too while you are at it?
Attachment #355545 - Flags: review?(philipp) → review+
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
(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.
Attached patch patch - v2 (obsolete) β€” β€” Splinter Review
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!
Attachment #355545 - Attachment is obsolete: true
Attachment #355757 - Flags: review?(philipp)
Keywords: qawanted
Attachment #355757 - Flags: review?(philipp) → review+
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
Attached patch patch - v3 (obsolete) β€” β€” Splinter Review
(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+
Attachment #355757 - Attachment is obsolete: true
Attachment #355804 - Flags: review+
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."
(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.
Attached patch patch - v4 β€” β€” Splinter Review
Found another regression: calAuthPrompt undefined. Migrating calUtils.js code into calAuthUtils.jsm... This patch gets longer and longer ;-)

Re-requesting review because of that.
Attachment #355804 - Attachment is obsolete: true
Attachment #355972 - Flags: review?(philipp)
Comment on attachment 355972 [details] [diff] [review]
patch - v4

Changes look fine, r=philipp. I hope we have all needed files now :-)
Attachment #355972 - Flags: review?(philipp) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: qawanted
Resolution: --- → FIXED
Target Milestone: --- → 1.0
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".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.