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

[REF] Add caching to function to determine if activity type is permitted. #13329

Merged
merged 1 commit into from
Dec 21, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

This reduces the number of queries that run when calling Activity.get api

Before

Currently the Activity.get api gets a number of queries and then checks the permission for each one (this is something I'd like to change) and in each check it runs an sql query to determine if the contact can access the activity type of that activity. There are other places (forms, pages) where permissions are determined on multiple activities by running multiple queries

After

A list of accessible activity types is cached in a php static variable. The query only runs once

Technical Details

This is being cached in a php static variable rather than being queried multiple times.

It would be nice if this could be cached in Redis in a future iteration

We would want to be able to flush this whenever one of the following
happens

 1) an activity type is created, updated, deleted
 2) a component is enabled or disabled
 3) a CMS permission change takes place

It seems like it would be good for functions that do caching to register themselves
against actions that may affect them.

Comments

This adds caching & re-usability to the activity type checking
@civibot
Copy link

civibot bot commented Dec 20, 2018

(Standard links)

@civibot civibot bot added the master label Dec 20, 2018
@colemanw
Copy link
Member

The function getPermittedActivityTypes looks like it would be amenable to a test :)

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Dec 21, 2018

@colemanw it's pretty well covered by api tests which were added in conjunction with an earlier step in the refactoring

@colemanw colemanw merged commit 1370f4b into civicrm:master Dec 21, 2018
@colemanw colemanw deleted the activity_extract branch December 21, 2018 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants