Closed
Bug 566339
Opened 15 years ago
Closed 15 years ago
CalDAV errors lack enough information.
Categories
(Calendar :: Provider: CalDAV, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b3
People
(Reporter: dimassevfc, Assigned: nomisvai)
Details
(Whiteboard: [needed beta][has l10n impact])
Attachments
(2 files, 4 obsolete files)
16.95 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
1.55 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
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.
This may be a solution.
This is my first patch. Sorry for the errors.
Attachment #445733 -
Flags: review?
Attachment #445733 -
Flags: review? → review?(philipp)
Updated•15 years ago
|
Assignee: nobody → dimassevfc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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 4•15 years ago
|
||
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+
Assignee | ||
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #451812 -
Attachment is patch: true
Attachment #451812 -
Attachment mime type: application/octet-stream → text/plain
Comment 9•15 years ago
|
||
Comment on attachment 451812 [details] [diff] [review]
patch3
Looks fine, r=philipp
Attachment #451812 -
Flags: review+
Assignee | ||
Comment 10•15 years ago
|
||
pushed to comm-central : http://hg.mozilla.org/comm-central/rev/c845e4a3388b
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Target Milestone: --- → 1.0b3
Reporter | ||
Comment 11•15 years ago
|
||
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 → ---
Reporter | ||
Comment 12•15 years ago
|
||
Here is the fixed patch3
Attachment #457038 -
Flags: review?(philipp)
Attachment #457038 -
Attachment is obsolete: true
Attachment #457038 -
Flags: review?(philipp)
Reporter | ||
Comment 13•15 years ago
|
||
Correction for patch3
Attachment #457048 -
Flags: review?(philipp)
Comment 14•15 years ago
|
||
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-
Updated•15 years ago
|
Whiteboard: [needed beta][has l10n impact]
Updated•15 years ago
|
Whiteboard: [needed beta][has l10n impact] → [needed beta][has l10n impact][needs new patch]
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #457048 -
Attachment is obsolete: true
Attachment #485353 -
Flags: review?(philipp)
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Whiteboard: [needed beta][has l10n impact][needs new patch] → [needed beta][has l10n impact]
Comment 16•15 years ago
|
||
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+
Comment 17•15 years ago
|
||
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/7b17b0c177ef>
a=philipp
Assignee | ||
Comment 18•15 years ago
|
||
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•15 years ago
|
||
Can this bug report be closed now?
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•