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

Move from_email_address options to new DomainEmailAddresses table #31909

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 29, 2025

Overview

Moves the 'from_email_address' option group into its own table: Introducing the new DomainEmailAddress entity. 🎉

Migration and Backward Compatibility Notes

This adds an apiWrapper for backward-compatibility, to allow DomainEmailAddress records to be accessed as if it were an option group. When accessing it via the OptionValue api, the new email and name columns are concatenated to produce familiar value of "Name" <[email protected]>, and id is returned as value. Here's what is and is not supported:

Function Works Notes
Api3 get $params must contain 'option_group_id' => 'from_email_address'
Api3 create $params must contain 'option_group_id' => 'from_email_address'
Api3 update (aka. create + an id) works if passed id of an existing domainEmailAddress and a correctly formatted email header as value
Api3 delete 🔴 delete action only gets passed an id which is not enough info for the adapter
Api3 chaining get chained with create/update/delete will all work
Api4 get WHERE must contain 'option_group_id:name', '=', 'from_email_address'
Api4 update WHERE must contain 'option_group_id:name', '=', 'from_email_address'
Api4 create $values must contain 'option_group_id:name' => 'from_email_address'
Api4 delete WHERE must contain 'option_group_id:name', '=', 'from_email_address'
Api4 chaining 🔴 Couldn't find any uses in universe so didn't add support for this
CRM_Core_BAO_Domain::getNameAndEmail CRM_Core_BAO_Domain::getFromEmail No signature change
CRM_Core_BAO_Email::getFromEmail() CRM_Core_BAO_Email::domainEmails() No signature change
CRM_Core_OptionGroup::values Supported params: $labelColumnName, $keyColumnName, $onlyActive, $condition
CRM_Core_OptionGroup::getDefaultValue Returns id of default email address for current domain
Other core functions 🔴 CRM_Core_OptionGroup::valuesByID, CRM_Core_Pseudoconstant::get, etc.
SQL Queries, etc. 🔴 The options no longer exist in the civicrm_option_value table.

New-to-Old column map

DomainEmailAddress OptionValue
id value
name label (in quotes)
email label (in brackets)
description description
is_active is_active
is_default is_default
domain_id domain_id

Before

The list of from email addresses was not well-placed in the OptionValue table because:

  1. It was the only domain-specific option, overcomplicating that table with domain handling just for this one option list (aside, there was actually one other: Grant Type, but probably wasn't actually being used for multidomain).
  2. Had to mush the display name and email address together, like "My Name" <[email protected]> which wasn't fun for users to type and involved a lot of splitting/unsplitting strings.

After

New table has just the fields needed for storing site email addresses. UI switched to SearchKit/FormBuilder:

image

Copy link

civibot bot commented Jan 29, 2025

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Jan 29, 2025
@totten
Copy link
Member

totten commented Jan 29, 2025

Personal feeling: yes, please, for the love of god, yes.


Big consideration is going to be technical interop for extensions and sysadmin scripts. (In my copy of universe, there are ~90 references to from_email_address in ~45 extensions. That's a mix of things, and some may not be OG-references. But a lot are) Maybe there's some clever mechanism to help smooth it over?

(Like... hook_optionValues? Or, anticipating a mix of reads+writes and BAO/APIv3/APIv4, it may merit a bidirectional sync -- eg an extension legacyfromaddress which uses hook_post to sync the tables. I don't really know.)


In the future, we may get some better options to address permissions and routing if it's a proper entity.


Small aside #bikeshed: in ezcMailAddress and Civi\WorkflowMessage\Traits\AddressingTrait, they refer to these properties as name,email not display_name,email. I think name is probably more common in email-land, so I would gently vote for that. Not a huge deal.

@highfalutin
Copy link
Contributor

I don't deal with from_email_address much, but the proposed schema looks good to me. What would be the best practice in Civi for maintaining a single is_default = 1 per domain?

@colemanw
Copy link
Member Author

@highfalutin What would be the best practice in Civi for maintaining a single is_default = 1 per domain?

I added a unique index to the table for that, and the business logic would include an implementation of hook_civicrm_pre to set all others in the domain to false whenever setting one to true.

@colemanw
Copy link
Member Author

@totten I think name is probably more common in email-land, so I would gently vote for that. Not a huge deal.

Well in Civi-land, name usually means "machine name" so I avoided it for that reason, but since there's precedent with other email entities, sure let's go with name.

@colemanw colemanw force-pushed the fromEmailAddress branch 2 times, most recently from a074321 to 8125154 Compare January 29, 2025 03:41
@totten
Copy link
Member

totten commented Jan 29, 2025

Well in Civi-land, name usually means "machine name" so I avoided it for that reason, but since there's precedent with other email entities, sure let's go with name.

Ah, good point. Terms are painted into a corner. Dealers choice.

@colemanw colemanw force-pushed the fromEmailAddress branch 5 times, most recently from 5658e96 to ba3bdc6 Compare January 30, 2025 19:26
colemanw and others added 4 commits January 30, 2025 20:45
Introduce a new entity type `DomainEmailAddress` as an alternative
to the 'from_email_address' option group.
Replaced legacy "from_email_address" option group references with the new DomainEmailAddress API.
Removed unnecessary domain filtering logic from OptionValue as this was the only option group it applied to.
@colemanw colemanw marked this pull request as ready for review January 31, 2025 02:51
@colemanw colemanw changed the title Add separate table for Domain Email Addresses Move from_email_address options to new DomainEmailAddresses table Jan 31, 2025
Wraps the OptionValue api (v3 and v4) to provide backward compatibility
for accessing DomainEmailAddress via the OptionValue api.
CRM_Core_OptionGroup::values and ::getDefaultValue are also supported.
Removes unused handling from the quickform
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.

4 participants