-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
CRM-20816 - Expose CiviCase settings through "Settings" framework #10609
Conversation
…ings.xml" CiviCase was originally developed before the creation of the "Settings" framework (aka `$civicrm_settings`, `Settings API`, `civicrm_settings`, `*.setting.php`). At the time, it stored some settings in a special file named "Settings.xml". Today, this file is an anomaly which supports fewer dataflows. With this revision: * For backward compatibility, the default setting is `auto`, which reads the setting from XML. * For maximum compliance with the Settings framework, the value from `Civi::settings()->get()` takes precedence (if defined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good Tim, Like the idea behind it @totten
@totten when you say "When |
INSERT INTO civicrm_navigation | ||
(domain_id, url, label, name, permission, permission_operator, parent_id, is_active, has_separator, weight) | ||
VALUES | ||
({$domainID}, 'civicrm/admin/setting/case?reset=1', '{ts escape="sql" skip="true"}CiviCase Settings{/ts}', 'CiviCase Settings', NULL, 'AND', @civicaseAdminId, '1', NULL, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@totten should you also update the weight of the other case navigation items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I wondered about that and tested -- as you might expect/hope, the two items with weight==1
float to the top next to each other. The current code seems to work.
Going the other way, it didn't seem like a good idea to manually set the weight of each of the other items -- because they could have been manually rearranged/supplemented/removed, and changing them seemed edgy.
But on second thought... if it really matters... I guess this particular situation could be addressed by incrementing all the weight
s. Ex:
UPDATE civicrm_navigation
SET weight = weight+1
WHERE domain_id = {$domainID} AND parent_id = @civicaseAdminId;
INSERT INTO civicrm_navigation ...;
"What happens if that old xml file is removed?" @colemanw I discovered while working on this that the current defaults actually stem from reading Answering your question depends on which old XML is removed:
Note, though, that I wasn't keen to actually remove
But... I wouldn't mind putting in a time-bomb where it stops reading these settings from XML when version changes to 5.0... |
Sounds good. Let's add it to the 5.0 list. |
CRM/Case/Info.php
Outdated
*/ | ||
public static function getRedactOptions() { | ||
return array( | ||
'auto' => ts('Auto'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think "Auto" will make sense to a user here? I've seen the help text in the template and settings/Case.setting.php
file. I'm not so familiar with the context, but just thought something like "default" would be more standard here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we could call it "Magic", and it would make just as much sense. ;)
But... yes, "Default" does feel a bit more standard. Updating.
CRM/Case/XMLProcessor/Process.php
Outdated
@@ -634,6 +634,10 @@ public function getListeners($caseType) { | |||
* @return int | |||
*/ | |||
public function getRedactActivityEmail() { | |||
$setting = Civi::settings()->get('civicaseRedactActivityEmail'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for these three method is the same, I wonder would a private function be helpful to remove the duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, sometimes a little duplication is better. It seems like a placing a bet for where you think it'll go.
In this case... the functions are not quite identical. One guards explicitly against the condition where $xml
is unavailable, and the others don't. This situation feels like a mistake -- the guard seems sensible for all three. Which is the kind of problem you'd expect from too much duplication... so I agree on consolidating...
…ultipleClients The second instance of the word "case" seems redundant. Nip it in the bud.
Made a few small updates (with commit msgs) and edited the description accordingly. |
== Overview ==
CiviCase was originally developed before the creation of the "Settings"framework (aka
$civicrm_settings
,Setting API
,civicrm_settings
,*.setting.php
). At the time, it stored some settings in a special file named "Settings.xml".== Before ==
The following settings could only be customized by creating or editing a file like
CRM/Case/xml/configuration.sample/Settings.xml
:RedactActivityEmail
AllowMultipleCaseClients
NaturalActivityTypeSort
== After ==
The settings can be customized multiple ways, e.g.
civicrm.settings.php
or in theSetting
API, set values for:civicaseRedactActivityEmail
civicaseAllowMultipleClients
civicaseNaturalActivityTypeSort
The settings default to the value
default
. Whendefault
is set, the setting will be loaded with the legacy XML logic.