566339 - CalDAV errors lack enough information.

Status: RESOLVED FIXED

Description

[Dimas]

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.

Comment 1

[Dimas]

Created attachment 445733 [details] [diff] [review]
Patch for Bug 566339 - v1

This may be a solution.

This is my first patch. Sorry for the errors.

Comment 2

[Simon Vaillancourt]

Created attachment 449618 [details] [diff] [review]
patch that displays the response body in the Details for MODIFICATION_FAILED.

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.

Comment 3

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

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?

Comment 4

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

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?

Comment 5

[Simon Vaillancourt]

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.

Comment 6

[Dimas]

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.

Comment 7

[Simon Vaillancourt]

Created attachment 451812 [details] [diff] [review]
patch3

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.

Comment 8

[Dimas]

Simon thank you very much, then I think the bug is fixed.

Comment 9

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

Comment on attachment 451812 [details] [diff] [review]
patch3

Looks fine, r=philipp

Comment 10

[Simon Vaillancourt]

pushed to comm-central : http://hg.mozilla.org/comm-central/rev/c845e4a3388b

Comment 11

[Dimas]

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.

Comment 12

[Dimas]

Created attachment 457038 [details] [diff] [review]
patch4

Here is the fixed patch3

Comment 13

[Dimas]

Created attachment 457048 [details] [diff] [review]
patch5

Correction for patch3

Comment 14

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

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 :-)

Comment 15

[Simon Vaillancourt]

Created attachment 485353 [details] [diff] [review]
patch showing error information even if there is no error body

Comment 16

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

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.

Comment 17

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

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/7b17b0c177ef>

a=philipp

Comment 18

[Simon Vaillancourt]

v3 pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/eccc23e8ab74

and back ported:
http://hg.mozilla.org/releases/comm-1.9.2/rev/42cc863a2c4a

Comment 19

[Martin Schröder [:martinschroeder]]

Can this bug report be closed now?