269578 - Deleting events brings up multiple confirmation dialogs

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Matthew (lilmatt) Willis]

Deleting a single, titled event brings up multiple confirmation dialogs.

Sunbird 0.2b

Comment 1

[Matthew (lilmatt) Willis]

Attachment: rev0 - evaluations in nested if statements are more specific to prevent match multiple confirms

Comment 2

[gekacheka]

Comment on attachment 165765 [details] [diff] [review]
rev0 - evaluations in nested if statements are more specific to prevent match multiple confirms

Note the "=" where "==" was intended.  Also note that title could be null or
empty.

It will be clearer to make the control structure simpler rather than use long
test cases where the conditions before the last are exclusive if you study
them.

if ( SelectedItems && SelectedItems.length > 0) {
  if ( !DoNotConfirm )
  {
    var calendarEvent = SelectedItems[0];
    if ( SelectedItems.length > 1) {
      if (!confirm( confirmDeleteAllEvents ) )
	break outerLoop;
      // else fall thru
    } else if (calendarEvent.title) { // null and empty string are false
      if (!confirm( confirmDeleteEvent+" "+calendarEvent.title+"?" ) )
	break outerLoop;
      // else fall thru
    } else {
      if (!confirm( confirmDeleteUntitledEvent ) )
	break outerLoop;
      // else fall thru
    }
  }
  ...
}

Comment 3

[Matthew (lilmatt) Willis]

(In reply to comment #2)
> (From update of attachment 165765 [details] [diff] [review])
> Note the "=" where "==" was intended.  Also note that title could be null or
> empty.

Thanks for that catch.

> It will be clearer to make the control structure simpler rather than use long
> test cases where the conditions before the last are exclusive if you study
> them.
> 
> if ( SelectedItems && SelectedItems.length > 0) {
>   if ( !DoNotConfirm )
>   {
>     var calendarEvent = SelectedItems[0];
>     if ( SelectedItems.length > 1) {
>       if (!confirm( confirmDeleteAllEvents ) )
> 	break outerLoop;
>       // else fall thru 
>     } else if (calendarEvent.title) { // null and empty string are false
Won't this also be true for the multiple event case above when it falls through?

>       if (!confirm( confirmDeleteEvent+" "+calendarEvent.title+"?" ) )
> 	break outerLoop;
>       // else fall thru
>     } else {
Again, won't this be true for the above cases that have fallen through?

>       if (!confirm( confirmDeleteUntitledEvent ) )
> 	break outerLoop;
>       // else fall thru
>     }
>   }
>   ...
> }
> 

I think I have an even easier way to make this work. I'm testing it now, and
I'll attach the patch once I'm pretty sure it's all sussed out.

Comment 4

[Matthew (lilmatt) Willis]

Attachment: rev1 - more elegant, readable solution. Also catches exceptions when actually calling libical to delete

This is more straightforward, and should be much more readable. 

After getting some exceptions when testing (which I am about to file a bug on),
I added the try/catch to take catch them.

Comment 5

[Michael Proteau]

*** Bug 269805 has been marked as a duplicate of this bug. ***

Comment 6

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

Comment on attachment 165803 [details] [diff] [review]
rev1 - more elegant, readable solution. Also catches exceptions when actually calling libical to delete

+		confirmText = (confirmDeleteEvent+" "+calendarEvent.title+"?"
);

That should use stringbundles and formatStringFromName instead. Please file a
followup bug for this.
(it already was like that, and should not hold back fixing the regression)

Comment 7

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

patch checked in.
leaving open for checkin to sunbird0.2

Comment 8

[Mostafa Hosseini]

Checked into 0.2 branch

Comment 9

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!