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

Convert bcc field to use an entity reference. #17064

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 13, 2020

Overview

This changes the bcc field on all email fields to use an entity reference fields. This gives significant code simplification as well as extending filtering on the field

Note I've restricted to the bcc field for now as having it different makes it easier to do comparitive
testing. The other fields will follow once this is merged

Before

Custom js & php code for this use case - same as the cc field.

Screen Shot 2020-04-13 at 4 31 06 PM

After

Entity reference box
Screen Shot 2020-04-13 at 4 30 21 PM

Technical Details

@mfb - I did some testing on this & in my tests it performs well - it uses 2 queries not a union (if required ) but avoids the OR

@colemanw can you try this out? Note I've ONLY done the bcc field so far & as yet tags & groups don't filter

Comments

@civibot
Copy link

civibot bot commented Apr 13, 2020

(Standard links)

@civibot civibot bot added the master label Apr 13, 2020
@eileenmcnaughton eileenmcnaughton force-pushed the email3 branch 4 times, most recently from 6314f49 to 9f42134 Compare April 13, 2020 04:27
@colemanw
Copy link
Member

This is cool. I can't believe how crazy the old widget was - using "contact id + email string" as the "id" field. Using "email.id" is so much saner.

One nitpick: the "description" field includes a redundant copy of the sort name.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I'm glad you picked up that nitpick - because I wasn't sure what was best there - I think originally I had display name & sort name (rather than both being sort) - but I'm not sure if it should just be sort

@eileenmcnaughton
Copy link
Contributor Author

Looks like when I changed label_field to sort_name that created a duplication

'contact_id.sort_name',

@colemanw
Copy link
Member

I think just sort_name as the display value and email as the description is fine.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I think you had some thought about how I could add groups & tags to the filter on email?

Note I've restricted to the bcc field for now as having  it different makes it easier to do comparitive
testing. The other fields will follow once this is merged
@eileenmcnaughton
Copy link
Contributor Author

@colemanw I've updated the label & added some comments as well. The groups & tags thing is more of a follow up

@eileenmcnaughton
Copy link
Contributor Author

@colemanw where do you think we are on this? I'd be happy to leave it as just the bcc field for this rc & encourage larger sites to do further testing (eg. @lucianov88 @mfb @pfigel ) before switching 'to' & 'cc' over.

Also I'd like to discuss extending to filtering on tags & groups

@colemanw
Copy link
Member

This is working well, code looks good & passes tests. I'd be happy to merge a PR for the other email fields too, and we can test them all in this RC.

@colemanw colemanw merged commit 35f6155 into civicrm:master Apr 21, 2020
@colemanw colemanw deleted the email3 branch April 21, 2020 00:14
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