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

dev/core#2509 - Search kit display link for grant view is wrong #20060

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

demeritcowboy
Copy link
Contributor

Overview

Came up during https://lab.civicrm.org/dev/core/-/issues/2509.

If you create a search kit search on grants and make a display table and make one of the fields a link to Grant View, it's the wrong link.

Note also that unless the search includes a contact-related column the generated link has the literal [contact_id] in it which fatals when visited, and the link requires a contact id. This PR doesn't address that, but good to know for review purposes.

Before

Missing civicrm

After

Civified

Technical Details

Comments

@civibot
Copy link

civibot bot commented Apr 14, 2021

(Standard links)

@civibot civibot bot added the master label Apr 14, 2021
@colemanw
Copy link
Member

colemanw commented Apr 14, 2021

I'll merge this as the "before" version was clearly wrong.

The issue of [contact_id] is a bit complex and there are 2 sides to it:

SearchKit side: In the current design, links can pull data from the search but they can't "push" search params to ensure they get the data they need. The current compromise is that "id" always gets pushed onto the params.

Civi side: Ideally, a link to view/edit/delete any entity would require only the id. Maybe some cases (like e.g., cases) are more complex, but in this instance I see no reason why grants need the extra contact_id argument, they can probably just look it up.

@colemanw colemanw merged commit 79cf8fe into civicrm:master Apr 14, 2021
@demeritcowboy
Copy link
Contributor Author

Thanks!

@demeritcowboy demeritcowboy deleted the grantview branch April 14, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants