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

Fixes dev/core#2824 - Handle related Afforms when deleting Search Displays #21457

Merged
merged 5 commits into from
Sep 23, 2021

Conversation

colemanw
Copy link
Member

Overview

Improves delete handling and user feedback about Search Displays embedded in Afforms.

Before

  • Not a lot of info about afforms in the SearchKit UI.
  • Deleting a search display would not delete associated forms, causing broken forms and unexplained errors.
  • Broken link to edit Afform if the AfformAdmin extension was disabled.

After

  • The system now ensures that Afforms will be deleted when embedded SearchDisplays are deleted.
  • When AfformAdmin extension disabled, forms are not shown as links in SearchKit.
  • When deleting an entire SavedSearch, a confirmation will show all the related entities that will be deleted (SmartGroup, SearchDisplays and Afforms)
    Screenshot from 2021-09-13 14-30-05
  • Handy list of forms added to the SearchDisplay config screen, including a link to create a new one:
    Screenshot from 2021-09-13 14-32-13
  • Warning shown when deleting a Search Display:
    Screenshot from 2021-09-13 14-32-42

Technical Details

This adds a new "Extra" field to APIv4 Afform.get - search_displays which is a calculated field which returns the names of any search displays embedded on a form. This makes it easier to query the Afform api to see what displays are in use on which forms.

@civibot
Copy link

civibot bot commented Sep 13, 2021

(Standard links)

@civibot civibot bot added the master label Sep 13, 2021
@MegaphoneJon
Copy link
Contributor

Hi Coleman - happy to test later this week, but I'm thinking about Eileen's mention in the dev-digest that there's thinking underway about displaying multiple SearchKit displays in a single afform. Based on the screenshot above, the language suggests that any afform that contains an embedded display from a to-be-deleted search will be deleted. This may have unintended side effects on someone's "dashboard" afform. I also don't want to introduce scope creep here.

I suspect the correct behavior if "multiple displays on an afform" is implemented, would be to delete afforms that contain a single display, and warn about other afforms. And allow rendering of the "edit afform" screen that has a broken reference to a display. But I wouldn't block a merge of this against a non-existent potential feature.

@colemanw
Copy link
Member Author

Yea, we might change the behavior in the future. I have a vision for an Afform type "Layout" which would allow combining multiple other afforms into some sort of composition. When implementing that I'll need to work on some kind of surgical operation which will excise a search display or other afform from a parent layout when deleting the embedded item.

@eileenmcnaughton
Copy link
Contributor

@MegaphoneJon FYI - funding for the project mentioned is likely to emerge early next year

@MegaphoneJon
Copy link
Contributor

@eileenmcnaughton Glad to hear it. I have a client who wants to use SearchKit instead of Contribution Detail report but needs the totals of numeric columns. When I read that I thought that adding two SearchKit displays - one with a GROUP BY, one without - would meet that need.

@eileenmcnaughton
Copy link
Contributor

@MegaphoneJon yeah - so you can already do the thing where you have one report with group bys & then the rows in that link through to a detail report

I did document it but it looks like it hasn't published https://lab.civicrm.org/documentation/docs/user-en/-/commit/0cddeed4251da2c711cf7901232933892277e9fe#a20ca5b945e4ae0b5f08518de47ab495aa2550c0_115_115

@civicrm civicrm deleted a comment from eileenmcnaughton Sep 16, 2021
@civicrm civicrm deleted a comment from eileenmcnaughton Sep 16, 2021
@MegaphoneJon
Copy link
Contributor

OK - this looks good. There's just one thing I noticed, and I don't know if it's a big enough deal to address.

The "delete associated afforms" always works. However, the warning about which afforms will be deleted only considers afforms that existed when that (SearchKit) page was loaded. I would have expected this line to reload the afform list so as to present the complete list of afforms being deleted.

@colemanw
Copy link
Member Author

@MegaphoneJon are you talking about when deleting from the Listing screen (table of multiple saved searches) or from the screen where you edit a single saved search? The code you linked to was from the latter, but also on that screen is an auto-refresh-the-afforms function here: https://github.com/civicrm/civicrm-core/pull/21457/files#diff-75d84f75cabaad3ec790f7aacd36d726a052c253108f0926919035261814a75fR706

So on that screen the afform info should always be up-to-date. I don't think that's true of the listing though.

@MegaphoneJon
Copy link
Contributor

Ah, you're right. Yes, it's the listing that isn't refreshed.

@colemanw
Copy link
Member Author

Ok, given that this works let's merge it & then see about improving the listing.

@colemanw colemanw merged commit ab5dd70 into civicrm:master Sep 23, 2021
@colemanw colemanw deleted the searchDisplayAfformDelete branch September 23, 2021 19:19
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.

3 participants