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] CRM_Utils_Recent - Use hook listener to delete items #21204

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Aug 21, 2021

Overview

Simplifies a bunch of delete functions by de-coupling BAOs from the Recent Items menu/sidebar.

Before

BAO functions responsible for deleting their own items from the Recent menu/sidebar.

After

Recent items delete themselves.

Technical Details

This is toward dev/core#2757

Comments

All that's missing is Note which has a weird del() function. That's addressed in #21208

@civibot
Copy link

civibot bot commented Aug 21, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@colemanw I just skimmed through this & obviously my take was that it really should have a test - but I know you've done a lot of PRs in this space just recently so wondering if you feel that you have otherwise covered it (or perhaps there already is a test)

@colemanw
Copy link
Member Author

I don't think there are any tests at all for CRM_Utils_Recent!

@colemanw
Copy link
Member Author

@eileenmcnaughton and now there is one :)

@eileenmcnaughton
Copy link
Contributor

PHP Fatal error: Cannot declare class api\v4\Action\ContactIsDeletedTest, because the name is already in use in /home/jenkins/bknix-dfl/build/core-21204-7ayeg/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Action/ContactIsDeletedTest.php on line 81
PHP Stack trace:

Use array_filter() instead of brittle for() loops.
@@ -217,15 +217,6 @@ public static function deleteActivity(&$params, $moveToTrash = FALSE) {
self::logActivityAction($activity, $logMsg);
}

// delete the recently created Activity
Copy link
Contributor

Choose a reason for hiding this comment

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

I just r-run tested this one & it worked (although the recent items block does require a screen refresh)

@@ -217,12 +217,6 @@ public static function deleteCase($caseId, $moveToTrash = FALSE) {

CRM_Utils_Hook::post('delete', 'Case', $caseId, $case);

// remove case from recent items.
Copy link
Contributor

Choose a reason for hiding this comment

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

r-run ++

@@ -1063,7 +1063,7 @@ public static function deleteContact($id, $restore = FALSE, $skipUndelete = FALS
}

//delete the contact id from recently view
CRM_Utils_Recent::delContact($id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - tested all good - note that when soft deleting it gets a strike through & is removed on hard-delete

@@ -116,13 +116,6 @@ public static function discard($id) {
$transaction->commit();

CRM_Utils_Hook::post('delete', 'Group', $id, $group);

// delete the recently created Group
$groupRecent = [
Copy link
Contributor

Choose a reason for hiding this comment

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

r-run = OK

@@ -259,13 +259,6 @@ public static function del($id) {

$grant->find();

// delete the recently created Grant
$grantRecent = [
Copy link
Contributor

Choose a reason for hiding this comment

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

r-run = ok

@eileenmcnaughton
Copy link
Contributor

I did r-run on a handful of entities here and also stepped through the code. I thought it was great that you covered permissions in the test because I didn't think of that when doing r-run.

It would be great to also CREATE the r-run entities in the post function but obviously that is scope creep!

@eileenmcnaughton eileenmcnaughton merged commit 60df685 into civicrm:master Aug 23, 2021
@eileenmcnaughton eileenmcnaughton deleted the delRecent branch August 23, 2021 06:53
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.

2 participants