Closed Bug 566339 Opened 15 years ago Closed 15 years ago

CalDAV errors lack enough information.

Categories

(Calendar :: Provider: CalDAV, enhancement)

Lightning 1.0b1
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dimassevfc, Assigned: nomisvai)

Details

(Whiteboard: [needed beta][has l10n impact])

Attachments

(2 files, 4 obsolete files)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.9.0.19) Gecko/2010040116 Ubuntu/9.04 (jaunty) Firefox/3.0.19 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.11pre) Gecko/20100514 Lightning/1.0b2pre Shredder/3.0.6pre Whenever a MODIFICATION_FAILED error message appears, the "Description" field is empty and the user won't know the cause of the error. Reproducible: Always Steps to Reproduce: 1. Create a remote calendar, the user should only have lecture permission over it. 2. Create an event and save it. Actual Results: The following error message appears: - An error occurred when writing to the calendar "calendar name" * Error code: MODIFICATION_FAILED * Description: (empty) Expected Results: The following error message should appear: - An error occurred when writing to the calendar "calendar name" * Error code: MODIFICATION_FAILED * Description: There was an error storing the item on the server. The resource at "server uri" is refusing to respond to it, because the user lacks the required permission. The error code of the most common failures should be displayed: - 400 = HTTP "Bad Request" - 403 = HTTP "Forbidden" - 404 = HTTP "Not Found" - 5XX = HTTP "Server Error" For less common errors, a generic message would be enough.
Version: unspecified → Lightning 1.0b1
Attached patch Patch for Bug 566339 - v1 (obsolete) โ€” โ€” Splinter Review
This may be a solution. This is my first patch. Sorry for the errors.
Attachment #445733 - Flags: review?
Attachment #445733 - Flags: review? → review?(philipp)
Assignee: nobody → dimassevfc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
While I agree about the bug itself, I think the fix should contain the response body of the server instead of the generic description of a http error code. This is a constant headache for our users and support staff (not knowing why a write operation failed on the server). And I believe that displaying the server status + response body (which often contains the reason why the operation failed) would be invaluable.
Attachment #449618 - Flags: review?(philipp)
Comment on attachment 449618 [details] [diff] [review] patch that displays the response body in the Details for MODIFICATION_FAILED. >- cal.LOG("CalDAV: Unexpected status adding item to " + >- thisCalendar.name + ": " + status); >+ let responseBody=""; >+ try { >+ responseBody = cal.convertByteArray(aResult, aResultLength); >+ } catch(e) {} > >- thisCalendar.reportDavError(Components.interfaces.calIErrors.DAV_PUT_ERROR); >+ cal.ERROR("CalDAV: Unexpected status adding item to " + >+ thisCalendar.name + ": " + status + "\n" + >+ responseBody + "\n" + >+ serializedItem); >+ >+ thisCalendar.reportDavError(Components.interfaces.calIErrors.DAV_PUT_ERROR, >+ "Status:"+status+"\n" + responseBody); Doesn't calling reportDavError also show that message in the user-visible dialog box? I think showing the responseBody there is overkill and not needed for the average user. Others who would like to provide more elaborate debug info can still visit the error console and see the responseBody there. >- ""); >+ (detailedMessage?detailedMessage:"")); Please add some spaces here, i.e (foo ? bar : baz) Aside from that, this patch looks fine, r=philipp. Do I understand correctly, that both patches should be added?
Attachment #449618 - Flags: review?(philipp) → review+
Comment on attachment 445733 [details] [diff] [review] Patch for Bug 566339 - v1 >+itemPutErrorBadRequest=There was an error storing the item on the server. The request contains bad syntax or cannot be fulfilled. >+itemDeleteErrorBadRequest=There was an error deleting the item from the server. The request contains bad syntax or cannot be fulfilled. >+itemPutErrorForbidden=There was an error storing the item on the server. The resource at %1$S is refusing to respond to it, because the user lacks the required permission. >+itemDeleteErrorForbidden=There was an error deleting the item from the server. The resource at %1$S is refusing to respond to it, because the user lacks the required permission. >+itemPutErrorNotFound=There was an error storing the item on the server. The resource at %1$S could not be found but may be available again in the future. >+itemDeleteErrorNotFound=There was an error deleting the item from the server. The resource at %1$S could not be found but may be available again in the future. >+itemPutErrorServer=There was an error storing the item on the server. The server failed to fulfill an apparently valid request. >+itemDeleteErrorServer=There was an error deleting the item from the server. The server failed to fulfill an apparently valid request. Since these are two separate sentences, I think we can get away with splitting the strings, i.e # Use these existing strings itemPutError=There was an error storing the item on the server. itemDeleteError=There was an error deleting the item from the server. # Add these strings itemPutErrorBadRequest=The request contains bad syntax or cannot be fulfilled. itemPutErrorForbidden=The resource at %1$S is refusing to respond, because the user lacks the required permission. ... # Then, in the code, just concatenate the two. This will mean less strings to translate. Note also I slightly modified the PutErrorForbidden message. >+ } else if (status == 404){ Missing space before "{". >+ cal.LOG("CalDAV: The calendar " + thisCalendar.name + >+ " could not be found (Error 404)"); >+ thisCalendar.reportDavError(Components.interfaces.calIErrors.DAV_PUT_ERROR_NOT_FOUND); >+ } else if (status >= 500 && status < 600){ You could also write this as: } else if (Math.floor(status / 100) == 5) {, if you like. r=philipp with the above comments fixed. Dimas, do you need someone to check this in for you? If so, please upload a new patch with the above nits fixed. Simon, maybe you could do this when checking in your own patch?
Attachment #445733 - Flags: review?(philipp) → review+
I will try to merge both in a meaningful way and resubmit the patch, but I disagree with not putting the response body in the Details dialog, mostly because: 1) Users don't know the error console exist. 2) We can tell them right away what went wrong, why not do it? This solution is still not perfect but we're slowly getting there, here is the use case to justify putting the body in the Details: With no response body in the dialog: - Bob creates a meeting with 30 attendees, saves it - Error pops up, Bob clicks on detail and sees a useless : "403 The resource at %1$S is refusing to respond, because the user lacks the required permission." - Bob presses OK and is pretty mad because he has to re-enter the full meeting without really knowing *why* it really failed. With a response body: - Bob creates a meeting with 30 attendees, saves it - Error pops up, Bob clicks on detail will be able to see in the server response, something like "Access Denied: User Bob cannot invite User Alice." - Bob is still mad that he has to re-create the meeting with 30 attendees but at least he knows not to invite Alice. The next step would be to have a "Edit..." Button eventually so that the user can re-edit and re-submit the meeting after changing it so the server accepts it. This should be another bug.
Assignee: dimassevfc → simon.at.orcl
Thanks Philip, I think showing the responseBody there is overkill and not needed for the average user, too. I will upload a new patch with the corrections.
Attached patch patch3 โ€” โ€” Splinter Review
Here is the patch merging both patches in a slightly different way. I still think very strongly that the http status code generic messages alone are not enough for giving useful immediate input to the user about what went wrong.
Attachment #445733 - Attachment is obsolete: true
Attachment #449618 - Attachment is obsolete: true
Attachment #451812 - Attachment is patch: true
Attachment #451812 - Attachment mime type: application/octet-stream → text/plain
Simon thank you very much, then I think the bug is fixed.
Comment on attachment 451812 [details] [diff] [review] patch3 Looks fine, r=philipp
Attachment #451812 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
There are errors in the patch. When receives a 403 error, for example, in the error console appears the following error "extrainfo is not defined". extrainfo is the responsebody and this is not necessary to display the message so I think it should be removed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch4 (obsolete) โ€” โ€” Splinter Review
Here is the fixed patch3
Attachment #457038 - Flags: review?(philipp)
Attachment #457038 - Attachment is obsolete: true
Attachment #457038 - Flags: review?(philipp)
Attached patch patch5 (obsolete) โ€” โ€” Splinter Review
Correction for patch3
Attachment #457048 - Flags: review?(philipp)
Comment on attachment 457048 [details] [diff] [review] patch5 > var thisCalendar = this; >+ let serializedItem = this.getSerializedItem(aItem); > addListener.onStreamComplete = >- cal.LOG("CalDAV: Unexpected status adding item to " + >- thisCalendar.name + ": " + status); >+ let responseBody=""; Spaces around = >+ >+ buildDetailedMessage : function caldav_buildDetailedMessage(status) { >+ if (!status) { >+ return ""; >+ } >+ >+ var props = calGetStringBundle("chrome://calendar/locale/calendar.properties"); >+ let statusString; >+ try { >+ statusString = props.GetStringFromName("caldavRequestStatusCodeString" + status); >+ } catch (e) { >+ // Fallback on generic string if no string is defined for the status code >+ statusString = props.GetStringFromName("caldavRequestStatusCodeStringGeneric"); >+ } >+ return props.formatStringFromName("caldavRequestStatusCode", [ status ], 1) + ", " + >+ statusString; >+ > }, Since this function is relatively short, I'd prefer the following. Also, you can use calGetString instead of getting the bundle: let statuString = ""; if (status) { statusString = cal.calGetString("calendar", "caldavRequestStatusCodeString" + status) || cal.calGetString("calendar", "caldavRequestStatusCodeStringGeneric"); statusString = cal.calGetString("calendar", "caldavRequestStatusCode", [status]) + ", " + statusString; } return statuString; I haven't re-read the rest of this bug, but I remember reading something about not actually showing the body of the request? r- for now to get a new patch and to give me time to read (or someone to recap :-)
Attachment #457048 - Flags: review?(philipp) → review-
Whiteboard: [needed beta][has l10n impact]
Whiteboard: [needed beta][has l10n impact] → [needed beta][has l10n impact][needs new patch]
Attachment #457048 - Attachment is obsolete: true
Attachment #485353 - Flags: review?(philipp)
Status: REOPENED → ASSIGNED
Whiteboard: [needed beta][has l10n impact][needs new patch] → [needed beta][has l10n impact]
Comment on attachment 485353 [details] [diff] [review] patch showing error information even if there is no error body r=philipp, but please consider the above comments before checking in.
Attachment #485353 - Flags: review?(philipp) → review+
Can this bug report be closed now?
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: