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

Enotice fix on userDashboard with contributions, replace deprecated functions with api4v calls #24861

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 31, 2022

Overview

Enotice fix on userDashboard with contributions

  • occurs when debugging is enabled & contact dashboard is viewed for a contact who is the target of soft credits

Before

image

After

Notice cleared

image

Technical Details

I wound up with some scope creep here as the notice turned out to be in the soft credits secion not the main section. This turned out to be accessing a function that was already recommended for deprecation & only accessed by tests. I switched to moving the test to check the dashboard rendered rather than the deprecated function.

As I was wrapping up discovered the ContributionRecur section used 3 different forms of deprecated function - including a require_once on the api utils file and the potentially bug-inducing

$recurStatus = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'label');

accessing the wrong option group....

So in the end as I was deep in adding the tests I fixed all except one of the deprecated function uses

Comments

The tpl variables are also changed here a little - mostly around the honorRows which is not that accurate & is renamed to soft_credit_contributions & standardised with the contributions array. This should be surfaced in the release notes

@civibot
Copy link

civibot bot commented Oct 31, 2022

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

CRM_Contact_Page_View_UserDashBoardTest::testDashboardContentContributions
Undefined array key "id"

/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/default/files/civicrm/templates_c/en_US/%%D5/D5B/D5B2D140%%UserDashboard.tpl.php:35
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1914
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/default/files/civicrm/templates_c/en_US/%%5B/5B5/5B5E9910%%UserDashBoard.tpl.php:31
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1914
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/default/files/civicrm/templates_c/en_US/%%F7/F77/F77C7890%%CMSPrint.tpl.php:67
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1914
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/default/files/civicrm/templates_c/en_US/%%85/854/854F26FC%%unittests.tpl.php:6
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1273
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/CRM/Core/Smarty.php:189
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/CRM/Core/Page.php:254
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/CRM/Contact/Page/View/UserDashBoard.php:195
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php:259
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/tests/phpunit/CRM/Contact/Page/View/UserDashBoardTest.php:159
/home/jenkins/bknix-dfl/build/core-24861-1vbop/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:265
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:1721

Passing locally & I can't figure out what line it is :-( (smarty compiled is scrunched)

@yashodha
Copy link
Contributor

yashodha commented Nov 2, 2022

@eileenmcnaughton in Your Contributions section, the contributions are still not sorted by received date descending (look at the after screenshot)

image

You might to want fix that as well while you are at it.

@colemanw
Copy link
Member

colemanw commented Nov 2, 2022

General note, just to maks sure this is on your radar:
The biggest difference between using the DAO vs the API to display data on a page is that the DAO would fetch strings that are pre-escaped (< and > characters are html-escaped in the db) and the API does not.

@eileenmcnaughton
Copy link
Contributor Author

@yashodha - @colemanw & @seamuslee001 agreed your search order change made sense so I've included it. I had to adjust a test as a result

@colemanw I added escaping.

'cs' => $this->getUserChecksum(),
'cid' => $row['contact_id'],
]),
];
}
}
unset($row);
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's just for clarity when you have a variable bleed out of a loop & it is a reference there is a bit of a risk of doing something odd with it

'amount_level',
'contact_id,',
'source',
'balance_amount',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting this is ok because it is a computed field

->setSqlRenderer([__CLASS__, 'calculateBalance']);

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@colemanw @yashodha this is passing now - I think all comments are addressed

@yashodha
Copy link
Contributor

yashodha commented Feb 7, 2023

@eileenmcnaughton you might want to squash the commits

Escape option-possible values

Adjust test for change in order
@eileenmcnaughton
Copy link
Contributor Author

@yashodha done

@yashodha
Copy link
Contributor

yashodha commented Feb 8, 2023

@eileenmcnaughton merging this PR

@yashodha yashodha merged commit 1d5c596 into civicrm:master Feb 8, 2023
@eileenmcnaughton eileenmcnaughton deleted the dash branch February 8, 2023 07:51
@eileenmcnaughton
Copy link
Contributor Author

Yay - thanks @yashodha

yashodha referenced this pull request in cividesk/civicrm-core Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants