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

Afform - reset managed entities when deleting a dashlet #22957

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

colemanw
Copy link
Member

Overview

Fixes dev/core#3122

Before

Deleting an afform does not delete the related dashlet

After

Now it does

Technical Details

It wasn't as simple as adding a cache clear in the revert function, because if multiple afforms were being reverted at once, that would be very slow clearing the cache each time. So I refactored the BasicBatchAction to allow the cache to be cleared after the whole batch is processed.

@civibot
Copy link

civibot bot commented Mar 16, 2022

(Standard links)

@civibot civibot bot added the master label Mar 16, 2022
@@ -667,11 +667,11 @@ protected function loadDeclarations(): void {

protected function loadManagedEntityActions(): void {
$managedEntities = Managed::get(FALSE)->addSelect('*')->execute();
$this->managedActions = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

Unit test revealed this variable was leaky. The purpose of this function is to rebuild this array, but it needs to be reset first due to the singleton pattern.

@colemanw
Copy link
Member Author

@MegaphoneJon can you confirm this fixes your issue?

@MegaphoneJon
Copy link
Contributor

This doesn't fix the issue, but for what seems like an unrelated-ish regression. Deleting the Search Kit search no longer deletes the associated Form Builder form.

@colemanw
Copy link
Member Author

@MegaphoneJon not according to the unit tests.This is still passing:

SavedSearch::delete(FALSE)
->addWhere('name', '=', 'TestSearchToDelete')
->execute();
$this->assertCount(0, Afform::get(FALSE)->addWhere('search_displays', 'CONTAINS', 'TestSearchToDelete.TestDisplayToDelete')->execute());
$this->assertCount(0, Afform::get(FALSE)->addWhere('name', 'CONTAINS', 'TestAfformToDelete')->execute());

@colemanw
Copy link
Member Author

@MegaphoneJon actually I just took a closer look and you're right. The unit test wasn't catching this problem with deleting an afform using a "default display": #22997

So if we merge that PR as well as this one, it should all be working well according to the tests.

@colemanw
Copy link
Member Author

@MegaphoneJon now that #22997 is merged are you good with merging this one too?

@MegaphoneJon
Copy link
Contributor

I have no objection to this being merged - but it also seems as though #22997 was the bug that was generating the error for end users.

When this PR is applied I can see that the entry in civicrm_dashboard is deleted immediately when the Search Kit search is deleted. However, when the patch isn't applied, the dashboard isn't broken - it simply doesn't show the dashlet, and when cache is cleared, the civicrm_dashboard record is removed.

@colemanw colemanw merged commit b3346a2 into civicrm:master Mar 22, 2022
@colemanw colemanw deleted the afformClearCache branch March 22, 2022 16:57
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