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

Handle smart groups created in previous Civi's which include relation_permission #12803

Merged
merged 1 commit into from
Sep 12, 2018

Conversation

highfalutin
Copy link
Contributor

Overview

Fixes https://lab.civicrm.org/dev/core/issues/390: Smart groups created before 5.5.0 using "relation_permission" break on upgrade to 5.5.0

Before

  • In Civi 5.4, create a smart group based on a relationship search.
  • Upgrade to 5.5, clear cache and try to open the group.
  • Error: DB Error: syntax error

After

  • In Civi 5.4, create a smart group based on a relationship search.
  • Upgrade to 5.5, clear cache and try to open the group.
  • Group opens as expected.

Technical Details

When you create a Smart Group for a relationship search, the form values which are saved include relation_permission. Prior to 5.5.0, the operator was "=" and the value was scalar. As of 5.5.0, the operator is "IN" and the value is an array.

@seamuslee001
Copy link
Contributor

It would be great if we could get a unit test on this. @highfalutin are you able to do a second PR against the now 5.6 branch please as well

@highfalutin
Copy link
Contributor Author

@seamuslee001 I'd love to write a test but I'm not sure how too, as this involves an upgrade.

@seamuslee001
Copy link
Contributor

@highfalutin what i would be thinking is something like this https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/CRM/Contact/BAO/QueryTest.php#L391 but of course for your specific field

@highfalutin
Copy link
Contributor Author

5.6 PR: #12804.

@totten totten added the 5.5 label Sep 12, 2018
@totten
Copy link
Member

totten commented Sep 12, 2018

jenkins, test this please

@totten
Copy link
Member

totten commented Sep 12, 2018

(CiviCRM Review Template WORD-1.1)

  • (r-explain) Pass
  • (r-test) Pass: The failures in JobProcessMailingTest are a general/pre-existing issue affecting test-runs since the recent server rebuild.
  • (r-code) Pass: @eileenmcnaughton gave a general sign-off on the code in the 5.6 variant. (That PR is currently blocked on a test re-run.)
  • (r-doc) Pass: I added a release-note in v5.5.1 - Bump number. Draft release notes. #12805.
  • (r-maint) Soft Pass: We need to get the regression fix shipped. We can assess maintainability improvement as part of after-action analysis.
  • (r-run) Undecided: I'm doing this now. If I subsequently merge it, you take that as a sign it worked out. ;) Pass: Tested with the procedure described by Noah. That seems like a good procedure. Additionally, to hedge against regressions, I created some other smart-groups (purely in 5.5; with and without relationships) and did some cache-flushes. With the patch, all those cases worked as expected.
  • (r-user) Pass
  • (r-tech) Pass: Makes the inputs more forgiving

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.

3 participants