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

dev/core#2676 SearchKit - Add LocBlock entity to display Event location data #20746

Merged
merged 2 commits into from
Jul 7, 2021
Merged

dev/core#2676 SearchKit - Add LocBlock entity to display Event location data #20746

merged 2 commits into from
Jul 7, 2021
+42 −19

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 1, 2021 •
edited
Loading

Overview

Allows Event location data (address, email, phone, im) to be searched on and displayed in SearchKit.

See https://lab.civicrm.org/dev/core/-/issues/2676

colemanw added 2 commits July 1, 2021 17:14
@colemanw
Cleanup labels in LocBlock entity and expose to SearchKit
77af2b9
@colemanw
SearchKit - Improve join labels
577810c 
This fixes the order of joins, fixes a bug where the label fields were getting overwritten by
implicit joins before they could be used for explicit joins,
and uses the label field for explicit joins instead of just the entity name.
@civibot
Copy link

civibot bot commented Jul 1, 2021

(Standard links)

@civibot civibot bot added the master label Jul 1, 2021
@colemanw colemanw changed the title SearchKit - Add LocBlock entity to display Event location data dev/core#2676 SearchKit - Add LocBlock entity to display Event location data Jul 1, 2021
@seamuslee001
Copy link
Contributor

Code looks right to me but will leave this open for @michaelmcandrew to review as well

@colemanw
Copy link
Member Author

colemanw commented Jul 6, 2021

Ping @kurund

@michaelmcandrew
Copy link
Contributor

@colemanw - I do not really know much at all about this part of the codebase so can't really be of much help. Nothing stands out in any case.

@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

@michaelmcandrew I think what Seamus was saying is that he reviewed the code but hasn't run it. If you can do so then his half and your half together would make a complete review and this can be merged.

@michaelmcandrew
Copy link
Contributor

Got it - @kurund - could you test this as part of the relevant Omega issue.

@eileenmcnaughton
Copy link
Contributor

@colemanw I just pulled this down as I thought it would be easy to verify but I'm seeing not seeing it

image

@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

@eileenmcnaughton see http://shouldiblamecaching.com

@eileenmcnaughton
Copy link
Contributor

@colemanw yeah that was it

So this works but I hit a usability issue - as soon as I added 'address' it added a conditional on the location type. It took me a while to realise that was why no results were coming back. I doesn't seem to happen on email (And if we WERE to add some default conditional it would be is_primary not location type = billing)

image

@colemanw colemanw mentioned this pull request Jul 7, 2021
Merged
@colemanw
Copy link
Member Author

colemanw commented Jul 7, 2021

@eileenmcnaughton ok I've created #20803

@eileenmcnaughton
Copy link
Contributor

OK - with that out of scope of this PR I'm happy to merge this one

@eileenmcnaughton eileenmcnaughton merged commit 1eadd61 into civicrm:master Jul 7, 2021
@eileenmcnaughton eileenmcnaughton deleted the locBlockFixes branch July 7, 2021 23:04
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.
4 participants
@colemanw @seamuslee001 @michaelmcandrew @eileenmcnaughton