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#337 Add in upgrade script to set search by range to be false… #13384

Merged
merged 1 commit into from
Jan 2, 2019

Conversation

seamuslee001
Copy link
Contributor

… for Fields of money, float, int with widget types radio and select which were how they were effectively implemented originally

Overview

This adds in an upgrade sql step and also a post upgrade message as per @eileenmcnaughton 's comments here #13314

ping @eileenmcnaughton @totten @colemanw

… for Fields of money, float, int with widget types radio and select which were how they were effectively implementeted originally
@civibot
Copy link

civibot bot commented Jan 2, 2019

(Standard links)

@civibot civibot bot added the master label Jan 2, 2019
@seamuslee001 seamuslee001 changed the base branch from master to 5.9 January 2, 2019 21:31
@civibot civibot bot added 5.9 and removed master labels Jan 2, 2019
@eileenmcnaughton
Copy link
Contributor

Note that I also merged the related PR - #13314 & a release notes update will be needed to cover them

@seamuslee001
Copy link
Contributor Author

Merging as per the tag

@seamuslee001 seamuslee001 merged commit 4bf8a3c into civicrm:5.9 Jan 2, 2019
@seamuslee001 seamuslee001 deleted the dev_core_337 branch January 2, 2019 23:19
@@ -61,7 +61,7 @@ public function setPostUpgradeMessage(&$postUpgradeMessage, $rev) {
2 => ts('Email on Hold'),
);
$postUpgradeMessage .= '<p>' . ts('If the setting "%1" is enabled, you should update any smart groups based on the "%2" field.', $args) . '</p>' .
'<p>' . ts('If you were previously on version 5.8 and altered the WYSIWYG editor setting, you should visit the <a %1>Display Preferences</a> page and re-save the WYSIWYG editor setting.', array(1 => 'href="' . CRM_Utils_System::url('civicrm/admin/setting/preferences/display', 'reset=1') . '"')) . '</p>';
'<p>' . ts('If you were previously on version 5.8 and altered the WYSIWYG editor setting, you should visit the <a %1>Display Preferences</a> page and re-save the WYSIWYG editor setting.', array(1 => 'href="' . CRM_Utils_System::url('civicrm/admin/setting/preferences/display', 'reset=1') . '"')) . '</p><p>' . ts('If you have any custom fields of type Money, Int or Float, The search by range checkbox has been unchecked for these fields. If you wish to search by range on those fields you must re-set the checkbox') . '</p>';
Copy link
Member

@totten totten Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have any custom fields of type Money, Int or Float, The search by range checkbox has been unchecked for these fields. If you wish to search by range on those fields you must re-set the checkbox.

@seamuslee001 When I first read this text, I was pretty alarmed, so then I read more of the the background discussion -- and maybe it was false alarm and just a wording issue? Restating the situation to check my comprehension: This does not apply to all Money|Int|Float fields -- rather, it specifically applies to Radio|Select. To understand why this was done, it helps to consider the visual from the "New Custom Field" screen:

image

In older versions (<=5.8), for fields with (Money|Int|Float)+(Radio|Select), you would see the first option but not the second. In v5.9, you would see both. Consequently:

  1. Pre-existing fields (from <=5.8) don't have a sensible value of is_search_range. The option wasn't displayed in the UI before. so there was no specific intent.
  2. The behavior in old-versions depends on the specific old-version -- either it was crashy before, or it had a select/radio-type UX (not a search-range UX).
  3. Without the patch, pre-existing fields (from <=5.8) effectively change their behavior (becoming a search-range UX).
  4. With the patch, pre-existing fields (from <=5.8) effectively keep their behavior (select/radio-type UX).
  5. If you were using an interim version (5.9-alpha or 5.9-beta?) , then the upgrade script may override the policy -- these are the people who need to update the setting.

(NOTE: Italics indicate some edited text.)

How about this language:

CiviCRM v5.9+ adds a new search preference for certain custom-fields ("Money", "Integer", or "Float" data displayed as "Select" or "Radio" fields). For continuity, any old fields have been set to continue the old user-experience. However, you may want to review these settings. (You should especially review if this site used v5.9-alpha or v5.9-beta.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure happy with that @totten

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.

3 participants