Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-15713 - Case ajax fixes #4719

Closed
wants to merge 1 commit into from
Closed

Conversation

colemanw
Copy link
Member

No description provided.

@eileenmcnaughton
Copy link
Contributor

test this please

return FALSE;
}

$filterCases = CRM_Case_BAO_Case::getCases(FALSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't fully get this at first, but it seems smart -- getCases(TRUE) initially seemed more correct (because https://github.com/civicrm/civicrm-core/blob/master/CRM/Case/BAO/Case.php#L736 will ignore $allCases if the user lacks permission). However, combining L3073 and L3082 should produce the same results with better performance -- because there's no need to execute getCases(...) when we know that the user has permission 'access all cases and activities'.

I'm not sure sure it's proper to use getCases(...$type=='upcoming' [default-value]...); this seems to be saying that one only has permission to access cases with upcoming activities. As the general rule, I don't see how dates on activities would affect access-control to the overall case. $type=='any' seems more appropriate to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: getCases() includes a filter on case_status (which excludes closed cases). It seems OK to block write access on closed cases, but I don't think we'd want to block read access on closed cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm drafting an extra change.

@eileenmcnaughton
Copy link
Contributor

Tim - can you PR your extra change against 4.4 too as I ported Coleman's fix since time was running out. Or let me know if you think the pre-change version is fine for the LTS (not as elegant but does the trick is fine by me :-)

@totten
Copy link
Member

totten commented Dec 17, 2014

@eileenmcnaughton , 4.4 PR: #4730

@colemanw colemanw closed this Dec 17, 2014
@colemanw colemanw deleted the CRM-15713 branch December 30, 2014 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants