Issue #SCB2535

Activity report on

  *Defect SCB2535 - notification screen fails from auto created users*
 
  Scarab Link: http://www.solitone.org/scarab/issues/id/SCB2535
  Module: Scarab


  Activity generated by  Michael Semb Wever ([email protected])  at 05/07/2008 08:44



  *Comments*
  - By Michael Semb Wever - 05/07/2008 08:44 ---
  "> If we agree, that empty ActivityTypes must not be null, then the bug here is the empty ActivityType, not the exception thrown by NotificationStatus.compareTo.
> NotificationStatus.compareTo shouldn't have to deal with bugs in the code.

Well yes my first preference would have been to leave NotificationStatus.compareTo as it is, that is for it to throw the exception, and to catch it that exception in NotificationStatusPeer.getPendingNotfications and to isolate the error there. Problem with this is we're within a Collections.sort call, one bad notification event in the collection so it won't sort and your whole notification system is shot. 
While i agree with your argument, it is also common practice to check for null values and let them /float/ to the top or bottom of the list when implementing compareTo. If you look at the existing NotificationStatus.compareTo it does just this already for the issue ids. Where do we support issues without ids (other than before than have been first saved)? It also checks for null notifications as well!

What about adding to my patch error messages, and assertion, to make it clear that state is unhealthy and the method is /working around/ it.
EG
Index: src/java/org/tigris/scarab/om/NotificationStatus.java
===================================================================
--- src/java/org/tigris/scarab/om/NotificationStatus.java	(revision 10546)
+++ src/java/org/tigris/scarab/om/NotificationStatus.java	(working copy)
@@ -250,6 +250,25 @@
             rdo = user1.compareTo(user2);
             if (0 == rdo)
             {
+                //assert null == not1.getActivityType() : "Activity type must not be null:" + not1; //java5
+                //assert null == not2.getActivityType() : "Activity type must not be null:" + not2; //java5
+                
+                // when assertions are not enabled let's workaround null activityTypes
+                if(null == not1.getActivityType() && null == not2.getActivityType())
+                {
+                    getLog().error("Activity type must not be null:" + not1);
+                    getLog().error("Activity type must not be null:" + not2);
+                    return 0;
+                }else if(null == not1.getActivityType())
+                {
+                    getLog().error("Activity type must not be null:" + not1);
+                    return 1;
+                }else if(null == not2.getActivityType())
+                {
+                    getLog().error("Activity type must not be null:" + not2);
+                    return -1;
+                }"