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

Don't lose search criteria for smart group built with search builder #23052

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

konadave
Copy link
Contributor

Overview

Consider a smart group with yes/no criteria, e.g. Do Not Email and Deceased. If you go to 'Edit' the search criteria and it rebuilds the search builder the final values (yes/no) are lost and go back to 'Select'

screenshot_1_1635221149

I'm not sure if it's all yes/no fields or what the cause is but it seems to store the value correctly until you go back to edit and then they are lost - this could cause someone to then update the smart group criteria without realising they've lost values

@civibot
Copy link

civibot bot commented Mar 28, 2022

(Standard links)

@civibot civibot bot added the master label Mar 28, 2022
@seamuslee001
Copy link
Contributor

@konadave is this something only happening recently or has it been ongoing?

@konadave
Copy link
Contributor Author

konadave commented Mar 29, 2022

@seamuslee001 a different developer originally wrote this code for the client approx five months ago. I'm working with said client now and have been asked to submit this PR. I did confirm that it is still an issue on a vanilla 5.47.2 before submitting, and this fixed.

@seamuslee001
Copy link
Contributor

@konadave ok thanks, then I think master is fine, was just trying to figure out if it was in the category of a recent regression or not

@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We created a smart group with the condition do not email, deceased and country. We played with different conditions, such as = and /= (is not equa) and different value yes and no. We could replicate this issue in dmaster and not in the test environment of this PR.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@colemanw or @eileenmcnaughton can one of you merge this PR?

@jaapjansma jaapjansma added the merge ready PR will be merged after a few days if there are no objections label Apr 4, 2022
@eileenmcnaughton
Copy link
Contributor

Thanks for testing this @jaapjansma

@eileenmcnaughton eileenmcnaughton merged commit 61e00c4 into civicrm:master Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants