381853 - When calling modifyItem(), it is necessary to set X-GOOGLE-EDITURL for new item

Status: RESOLVED FIXED

(Calendar :: Provider: GData, defect)

Description

[Tim]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: 20070326

When calling modifyItem() function, it is necessary to set X-GOOGLE-EDITURL property for this new item. But actually, this property is identical to the old item one. In order to isolate the Google provider, it would be better if the property would be automatically filled in modifyItem() function.

So for instance, right before:
            var extradata = { olditem: aOldItem, listener: aListener };
We could add:
            aNewItem.setProperty("X-GOOGLE-EDITURL", aOldItem.getProperty("X-GOOGLE-EDITURL")) ;

This way, someone who calls modifyItem() doesn't need to be aware of the X-GOOGLE-EDITURL property.

Reproducible: Always

Steps to Reproduce:
Call modifyItem(). If you don't set X-GOOGLE-EDITURL property for the new item, it will results in an error.

Comment 1

[Stefan Sitter [:ssitter]]

The only place that uses X-GOOGLE-EDITURL seems to be in [http://lxr.mozilla.org/mozilla1.8/source/calendar/providers/gdata/components/calGoogleUtils.js]

So it's already isolated to the Google provider. Also, what is the error you see? You can copy and paste the error messages from the Error Console.

Comment 2

[Tim]

There is no particular error in the javascript console. But if you call modifyItem() and its parameter aNewItem hasn't X-GOOGLE-EDITURL property defined, you'll get Ci.calIErrors.CAL_IS_READONLY error returned (by function getItemEditURI() in calGoogleUtil.js).
It means that you cannot call modifyItem() "generically" (I mean without knowing that you use Google provider) since you have to set X-GOOGLE-EDITURL in aNewItem.

Comment 3

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

Attachment: Trivial Fix

You are correct, the edit url should definitly be taken from the old item. All went well, since currently the old item is cloned before the changes are made, which retains the X-GOOGLE-EDITURL property. 

The following patch should do the trick: take the edit url from the old item. It will nevertheless be impossible to use modifyItem() without knowing that you are using the GData Provider, since the old item needs to have an edit url to edit it.

Comment 4

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

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 5

[Tim]

Thanks a lot for dealing with this problem !

But I wonder if there is not a slight mistake in the correction (sorry I didn't see it af first glance).
The correction did:
request.uri = getItemEditURI(aOldItem);
instead of:
request.uri = getItemEditURI(aExtraData.olditem);
(there is no aOldItem method parameter and aExtraData.olditem was filled in calling modifyItem function())

Another remark I have, is that in case of problem (aOldItem not defined for instance) an exception is raised and caught in calGoogleCalendar.js modifyItem() function but e.result is not set: it means that the listener will be called but maybe will not be aware of the error. Or is there another way to detect it ?
Thanks.

Comment 6

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

Attachment: Additional fix

This additional fix should do it. I know it might seem redundant to add an extra parameter, but I'd like to keep the structure of extraData reserved for the response function. Using an extra parameter makes clear that it is needed for the session.

Kal, did I miss anything else?

(In reply to comment #5)
> Another remark I have, is that in case of problem (aOldItem not defined for
> instance) an exception is raised and caught in calGoogleCalendar.js
> modifyItem() function but e.result is not set: it means that the listener will
> be called but maybe will not be aware of the error. Or is there another way to
> detect it ?
> Thanks.
> 

if e.result is for any reason not set, then of course the listener will not know what type of error is happening, but this would be more of a problem with the piece of code that creates the exception. Are there any places where I am not setting the result correctly? I know I am using an empty message often, but this is mainly because there is no easy way to get a localized error message without replicating all errors into your own dtd file.

I think right now, onOperationComplete listeners don't do much with the error code. It is only important that the function is called at all.

Comment 7

[Tim]

Thanks Philipp for your quick correction. I understand why you wanted to add this additional parameter.

Actually, when the javascript interpretor meets a problem in the code (a method parameter is not defined for instance), an exception will be raised automatically but with no "result" property. The problem is that the exception is caught but without an error code the listener will think that everything went smoothly which is not the case. But I don't know if it is possible in that case to return a generic error code which such kind of errors (generated by the interpretor). But I agree that this kind of error is not supposed to happen.

Comment 8

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

As you can see in the catch() block, there is a call to LOG(). If calendar.debug.log is enabled, I am outputting that message which also contains the exception. Therefore, while developing you can see error messages. In any other case, you should not get syntax errors.

r=ctalbert
Checked in on MOZILLA_1_8_BRANCH and HEAD 

-> FIXED

Comment 9

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

Comment on attachment 267694 [details] [diff] [review]
Additional fix

r=ctalbert