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.

Sign up for GitHub

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

Jump to bottom

CiviGrant - Use SearchKit for contact summary tab #22467

Merged
merged 2 commits into from
Jan 13, 2022
Merged

CiviGrant - Use SearchKit for contact summary tab #22467

merged 2 commits into from
Jan 13, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 11, 2022 •
edited
Loading

Overview

Following-up on #22064, this switches the Grants tab on the contact summary page to use a Search Display.
It looks nearly identical before and after, but is now easily customizable via the SearchKit & Form-Builder GUI.

Before

Tab rendered with Smarty and old BAO_Query object.

After

Tab rendered with user-configurable search display.

@civibot
Copy link

civibot bot commented Jan 11, 2022

(Standard links)

@civibot civibot bot added the master label Jan 11, 2022
@colemanw colemanw force-pushed the civigrantSearchKitTab branch from 9159fc0 to 1cd7651 Compare January 11, 2022 02:24
colemanw
colemanw commented Jan 11, 2022
View reviewed changes
ext/civigrant/civigrant.php
* Add grants tab to contact summary screen.
*/
function civigrant_civicrm_tabSet($tabSetName, &$tabs, $context) {
if ($tabSetName === 'civicrm/contact/view' && !empty($context['contact_id'])) {
Copy link
Member Author

@colemanw colemanw Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@eileenmcnaughton I see the permission problem you noted in #22064 (comment) - this was adding the tab unconditionally without checking "access CiviGrant". This PR replaces rather than fixes that code :)

colemanw
colemanw commented Jan 11, 2022
View reviewed changes
ext/civigrant/ang/afsearchGrants.aff.json
"title": "Grants",
"contact_summary": "tab",
"server_route": "",
"permission": "access CiviGrant"
Copy link
Member Author

Choose a reason for hiding this comment

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

Permission check here

@eileenmcnaughton
Copy link
Contributor

This would potentially force a change on sites with customisations - I just posted on product mtce to check people are comfortable with this - feels safe-ish for civigrant due to low usage but seeing what people think

@colemanw
Copy link
Member Author

"Force" is a strong word. Sites with code customizations would have to update their code to re-implement the tabset hook and delete the search display.

@JoeMurray
Copy link
Contributor

@colemanw probably redundant question: how does this play with the refactoring of civigrant into core extension? It just works if the extension is enabled and doesn't cause an error if it isn't, right?

@monishdeb pls test next week, and review against our various grant extensions.

@eileenmcnaughton Is the concern that all of the customizations using form hooks would break when the form is dispensed with? I think Release Notes are likely good enough for this uncommonly used functionality as we have most of the main extensions in this area.

FWIW JMA are starting a large contract on grants that will likely result in significant enhancements in the coming 12 - 24 months.

@colemanw
Copy link
Member Author

@JoeMurray the refactoring of Component => Extension is complete :) #22064 has been merged.

With that done, it's now possible to package Afforms and Search Displays in the new CiviGrant extension, and you are correct, they will only appear when the extension is enabled, and will not cause errors when the extension is disabled.

@eileenmcnaughton
Copy link
Contributor

@JoeMurray yes - that is correct. I think some people have customised various screens and those customisations would break. I think for grant that is heavy mitigated by the relatively low usage it gets

@JoeMurray also, as an aside is this extension still in use https://github.com/JMAConsulting/biz.jmaconsulting.grantapplications/blob/1da4aee5bd616237398bd0a2803ed8a813cf7603/CRM/Grant/Form/Grant/Confirm.php#L724 (the linked line is to a function that we want to remove from core)

@eileenmcnaughton
Copy link
Contributor

Test fail is odd

Fatal error: Uncaught Exception: API error: API (SearchDisplay, create) does not exist (join the API team and implement it!) on SearchDisplay.create in /home/jenkins/bknix-dfl/build/core-22467-5b2ld/web/sites/all/modules/civicrm/CRM/Core/ManagedEntities.php on line 624

Exception: API error: API (SearchDisplay, create) does not exist (join the API team and implement it!) on SearchDisplay.create in /home/jenkins/bknix-dfl/build/core-22467-5b2ld/web/sites/all/modules/civicrm/CRM/Core/ManagedEntities.php on line 624

@colemanw
Copy link
Member Author

Just to qualify this statement:

some people have customised various screens and those customisations would break

The only screen this PR affects is the Grants tab on the Contact Summary screen. If anyone has customized that screen, they would either need to re-implement the hook being removed in this PR to bring the "classic" tab back, or re-do their customization in the SearchKit UI.
I'd further qualify this by saying that contact summary tabs are notoriously difficult to customize, so most people who wish to do so get deterred. That combined with low CiviGrant usage makes it a very small number. However, using SearchKit for those tabs makes them vastly more customizeable.

@colemanw
Copy link
Member Author

Hmm, test fail suggests that search_kit and afform are missing.

@colemanw
Copy link
Member Author

Retest this please

@eileenmcnaughton
Copy link
Contributor

Parsing schema description /home/jenkins/bknix-dfl/build/core-22467-5drgk/web/sites/all/modules/civicrm/xml/schema/Schema.xml
Extracting database information
Extracting table information
Installing core224675drgktest_29ob9 schema

Fatal error: Uncaught Exception: API error: API (SearchDisplay, create) does not exist (join the API team and implement it!) on SearchDisplay.create in /home/jenkins/bknix-dfl/build/core-22467-5drgk/web/sites/all/modules/civicrm/CRM/Core/ManagedEntities.php on line 624

Exception: API error: API (SearchDisplay, create) does not exist (join the API team and implement it!) on SearchDisplay.create in /home/jenkins/bknix-dfl/build/core-22467-5drgk/web/sites/all/modules/civicrm/CRM/Core/ManagedEntities.php on line 624

Specifically the civigrant tests are failing like this

@colemanw colemanw mentioned this pull request Jan 12, 2022
Merged
@colemanw
Copy link
Member Author

Specifically the civigrant tests are failing like this

@eileenmcnaughton I think it would be nice if we updated \Civi\Test\CiviEnvBuilder::installMe() to include the current extension and its dependencies. But for now I got it passing by enabling them explicitly.

@eileenmcnaughton
Copy link
Contributor

@colemanw OK - let's merge this & I will include in the dev list & if there is a bigger concern we can work with anyone who hits it to get the hook option working for them

@eileenmcnaughton eileenmcnaughton merged commit f03499d into civicrm:master Jan 13, 2022
@eileenmcnaughton eileenmcnaughton deleted the civigrantSearchKitTab branch January 13, 2022 00:42
@colemanw colemanw mentioned this pull request Jan 21, 2022
Closed
@colemanw colemanw mentioned this pull request Aug 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colemanw @eileenmcnaughton @JoeMurray