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] switch from (undeclared) class property to local variable. #13583

Merged
merged 2 commits into from
Feb 13, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Minor code cleanup

Before

undeclared class property used, extraneous config call

After

local var used, extraneous call removed

Technical Details

We either needed to declare the variable on the class & make it a local variable. Switching
to a local var reflects the fact it is never accessed from outside thie function & improves readability.

grepping for _userOptions returns nothing after this.

I also removed an extraneous config singleton call. I can't see a strong case that either of
these changes will affect the intermittent fails but ... maybe?

Comments

@seamuslee001 @mattwire either of you OK to check this - should be pretty simple as it's just preliminary to my next step (using the api to get the contribution rows rather than Selector object - in order to simplify some performance fixes & hopefully deal to the intermittent test fail)

We either needed to declare the variable on the class & make it a local variable. Switching
to a local var reflects the fact it is never accessed from outside thie function & improves readability.

grepping for _userOptions returns nothing after this.

I also removed an extraneous config singleton call. I can't see a strong case that either of
these changes will affect the intermittent fails but ... maybe?
@civibot
Copy link

civibot bot commented Feb 13, 2019

(Standard links)

@civibot civibot bot added the master label Feb 13, 2019
Copy link
Contributor

@mattwire mattwire left a comment

Choose a reason for hiding this comment

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

I've reviewed the code and given this a brief test - ok to merge

$check = CRM_Core_Permission::check('access Contact Dashboard');

if (!$check) {
if (!CRM_Core_Permission::check('access Contact Dashboard')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ok


$session = CRM_Core_Session::singleton();
$userID = $session->get('userID');
$userID = CRM_Core_Session::singleton()->getLoggedInContactID();
Copy link
Contributor

Choose a reason for hiding this comment

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

ok


if (!empty($this->_userOptions['Groups'])) {
if (!empty($dashboardOptions['Groups'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is only used within this function. So ok with this if it helps set the scene for further refactoring

@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire

@eileenmcnaughton eileenmcnaughton merged commit 9aa4d23 into civicrm:master Feb 13, 2019
@eileenmcnaughton eileenmcnaughton deleted the user_dash branch February 13, 2019 17:51
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