CRM_Activity_BAO_Activity::getActivities and activities reports don't check is_deleted

Discussed with Dave Greenberg on IRC (times are GMT):

Feb 01 17:39:16 <davej_> CRM_Activity_BAO_Activity::getActivities has no reference to is_deleted. Hence deleted activities show in dashlet after setting CRM_Case_Info showActivitiesInCore to 1
Feb 01 17:39:41 <davej_> But I'm wary of touching a BAO function!
Feb 01 17:43:51 <dgg> hey- davej_ can u give me some background
Feb 01 17:44:05 <dgg> why are u seting showActivitiesinCore to true?
Feb 01 17:44:44 <davej_> Hi dgg, wanting to show case activities in activities dashlet...
Feb 01 17:45:18 <davej_> tracked down why they weren't showing to showActivitiesinCore & lobo pointed me to where to change this.
Feb 01 17:45:36 <davej_> Some discussion in this topic: http://forum.civicrm.org/index.php/topic,6895.msg40581.html#msg40581
Feb 01 17:46:09 <dgg> ok - so i guess this is either a bug or an undeveloped feature - since Case activities are the only ones that use the is_deleted flag
Feb 01 17:46:45 <dgg> Going forward with the 'move to trash' / undelete work that we are doing for 3.2, most objects will have that - so this will get 'fixed' for sure then
Feb 01 17:47:17 <dgg> but i don't see a problem (at least on initial thinking) about fixing the BAO now (for 3.1.2)
Feb 01 17:47:23 <davej_> Right. Currently the CRM_Activity_BAO_Activity code basically has no mention of is_deleted apart from the delete/restore functions.
Feb 01 17:48:06 <dgg> however, we'll need to verify that this doesn't affect viewing 'deleted' activities in manage case selector
Feb 01 17:48:11 <dgg> (i don't think so)
Feb 01 17:48:25 <davej_> Right, I was wary of touching a BAO function as this is liable to be called all over the place!
Feb 01 17:50:02 <dgg> let me do some quick snooping - and if looks safe - u can make the change, verify that viewing deleted activiites works (manage case) and file and issue with the patch
Feb 01 17:51:40 <davej_> OK, so would just need to add is_deleted = 0 alongside is_test = 0 in getActivitySQLClause I guess.
Feb 01 17:52:40 <davej_> I've alos noticed that activities reports don't check is_deleted, have patched these for our installation.
Feb 01 18:04:07 <dgg> davej_: so I'm pretty sure the only place we allow users to see deleted activities is in Manage Case - Activities selector
Feb 01 18:04:27 <dgg> this grid uses CRM_Case_BAO_Case::getActivities
Feb 01 18:04:35 <dgg> and not the BAO you want to change
Feb 01 18:04:44 <dgg> so i think add is_deleted=0 is fine
Feb 01 18:04:58 <dgg> HOWEVER, you should also add is_current_revision=1
Feb 01 18:05:10 <dgg> since case activities are revisioned on edit (always insert)
Feb 01 18:05:29 <dgg> you can check that query in that function (above)
Feb 01 18:06:21 <davej_> Thanks dgg, I'll test it and file an issue.
Feb 01 18:06:30 <dgg> cool
Feb 01 18:06:51 <davej_> What about activities reports - mention those in same issue?
Feb 01 18:24:54 <dgg> oops - missed that davej_ yes include them