430280 - bad aOperationType on addItem

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Hubert FONGARNAND]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Build Identifier: 

It seems that when you call an addItem on an CalDav calendar the aListener.onOperationComplete is always called with Components.interfaces.calIOperationListener.MODIFY. 

I thinks it comes from the underlying calMemoryCalendar (cache system) 
in fact an modifyItem is called from this calMemoryCalendar when you do an adoptItem on caldav calendar

Reproducible: Always

Steps to Reproduce:
1.call aTargetCalendar.addItem(aCalItem, aListener);
2.test the value of aListener.onOperationComplete(aOperationType
3.
Actual Results:  
aOperationType always = Components.interfaces.calIOperationListener.MODIFY.

Expected Results:  
aOperationType = Components.interfaces.calIOperationListener.ADD

i need this because i want that the ITIP bar display a message like :

- The event has been updated 
- The event has been added (which is now always that)

Comment 1

[Hubert FONGARNAND]

Attachment: Quick fix for this problem

please review?

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 317020 [details] [diff] [review]
Quick fix for this problem

Hubert: It is best to ask someone specific for review. See http://wiki.mozilla.org/Calendar:Module_Ownership for a list of reviewers.

Comment 3

[Bruno Browning]

Comment on attachment 317020 [details] [diff] [review]
Quick fix for this problem

Thanks for catching this!

>                                 if (!thisCalendar.mItemInfoCache[item.id]) {
>                                     thisCalendar.mItemInfoCache[item.id] = {};
>+                                    thisCalendar.mItemInfoCache[item.id].newItem = true;
>+                                } else {
>+                                    thisCalendar.mItemInfoCache[item.id].newItem = false;
>                                 }

I think that for readability we should name this property isNew or perhaps isNewItem


>             for (var i = 0; i < items.length; i++) {
>                 if (thisCalendar.mItemInfoCache[items[i].id]) {
>-                    thisCalendar.mMemoryCalendar.modifyItem(items[i], null,
>+                    
>+                    if (thisCalendar.mItemInfoCache[items[i].id].newItem == false){
>+                        thisCalendar.mMemoryCalendar.modifyItem(items[i], null,
>                                                             aListener);
>+                    }
>+                    else {
>+                            thisCalendar.mMemoryCalendar.adoptItem(items[i], aListener);
>+                        }
>                 } else {
>                     thisCalendar.mMemoryCalendar.adoptItem(items[i], aListener);
>                 }
>             }

I don't think we need the nested if()s here. Unless I'm missing something (and please tell me if I am) I would think it sufficient to:

            for (var i = 0; i < items.length; i++) {
                if (thisCalendar.mItemInfoCache[items[i].id].isNew) {
                    thisCalendar.mMemoryCalendar.adoptItem(items[i], aListener);
                } else {
                    thisCalendar.mMemoryCalendar.modifyItem(items[i], null,
                                                            aListener);
                }
            }

r=browning with that

Comment 4

[Hubert FONGARNAND]

is

 if (thisCalendar.mItemInfoCache[items[i].id].isNew) crashing if thisCalendar.mItemInfoCache[items[i].id] is null?

Comment 5

[Hubert FONGARNAND]

Attachment: Fix v2

With bbrowning comments

Comment 6

[Sven Giermann]

(In reply to comment #4)
> is
> 
>  if (thisCalendar.mItemInfoCache[items[i].id].isNew) crashing if
> thisCalendar.mItemInfoCache[items[i].id] is null?
> 
Since the creation and setting of 'isNew = true' is only executed when 'aStatusCode == 200', I would also prefer to check for this to be null - otherwise it should throw an exception while trying to access 'isNew'...

            for (var i = 0; i < items.length; i++) {
                if (thisCalendar.mItemInfoCache[items[i].id] &&
                    !thisCalendar.mItemInfoCache[items[i].id].isNew) {
                    thisCalendar.mMemoryCalendar.modifyItem(items[i], null,
                                                            aListener);
                } else {
                    thisCalendar.mMemoryCalendar.adoptItem(items[i], aListener);
                }
            }

So please discuss with Bruno again and set the 'checkin-needed' keyword after uploading the final patch.

Comment 7

[Bruno Browning]

Sorry, I should have commented on this in the bug as well as on IRC. The "items" array we're iterating over here was set to null just before the "if (aStatusCode == 200)" block and is filled, entirely within that block, with items whose mItemInfoCache reference has just had the .isNew property set - after creating that reference if necessary. So I don't believe the extra check is necessary.

Comment 8

[Bruno Browning]

patch (v2) checked in on HEAD and MOZILLA_1_8_BRANCH
->FIXED

Comment 9

[Martin Schröder [:mschroeder]]

Comment on attachment 317256 [details] [diff] [review]
Fix v2

Bruno has already checked in this patch what implicates r+. :)