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.

Sign up for GitHub

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

Jump to bottom

dev/core#926 Fixes bug on searching for removed members of smartgroups #14182

Closed
wants to merge 1 commit into from
Closed

dev/core#926 Fixes bug on searching for removed members of smartgroups #14182

wants to merge 1 commit into from

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 2, 2019 •
edited
Loading

Overview

Fixes a bug whereby it's not possible to search for contacts who have been removed from a smart group (although it works with regular groups)

Before

Screenshot 2019-05-02 17 12 39

After

Screenshot 2019-05-02 19 05 25

Technical Details

This is an imperfect fix - code is still hard to read & I feel like there is still a logic gap in the code (unchanged) but it

  1. adds testing
  2. does some code cleanup - which will probably be merged in dev/core#926 [ref] adds a test & does a preliminary extraction #14181 & rebased out of this PR
  3. improves in code comments (with doc blocks)
  4. fixes a replicable bug in a pretty safe way - in some ways the least important of these things

Comments

https://lab.civicrm.org/dev/core/issues/926

@civibot
Copy link

civibot bot commented May 2, 2019

(Standard links)

@civibot civibot bot added the master label May 2, 2019
@eileenmcnaughton
Copy link
Contributor Author

Test fails relate - that at least tells me the code has good coverage

mattwire
mattwire reviewed May 2, 2019
View reviewed changes
CRM/Contact/BAO/Query.php Outdated Show resolved Hide resolved
@eileenmcnaughton eileenmcnaughton force-pushed the group_search_fix branch 2 times, most recently from 29b01c0 to 319c9a0 Compare May 4, 2019 05:03
@mattwire
Copy link
Contributor

mattwire commented Jun 1, 2019

test this please

@eileenmcnaughton
Copy link
Contributor Author

sadly re-testing won't help - the fix needs work. I think I know how now....

@eileenmcnaughton eileenmcnaughton force-pushed the group_search_fix branch 2 times, most recently from dc4d717 to 7ea7295 Compare June 3, 2019 01:13
@eileenmcnaughton
Copy link
Contributor Author

I think this is fixed now .- I think this is a regression from 5.10 security patches although I haven't confirmed that - will try to get merged before rc is cut on that basis

@seamuslee001
Copy link
Contributor

@eileenmcnaughton are those screenshots the correct way around?

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 they both look wrong don't they! I think that might have been from the earlier attempt that I re-wrote

@eileenmcnaughton
Copy link
Contributor Author

test this please

@Stoob
Copy link
Contributor

Stoob commented Jun 14, 2019

I'm having trouble testing this PR because the I cannot remove contacts from a smart group without the attached syntax error.
remove.pdf

@eileenmcnaughton eileenmcnaughton mentioned this pull request Jun 23, 2019
Merged
@eileenmcnaughton
Copy link
Contributor Author

#14618 incorporates this so I will close this if that is merged

@eileenmcnaughton
Copy link
Contributor Author

Also note we should re-test per @Stoob's comments once merged - I think that might already be resolved but unsure

@seamuslee001
Copy link
Contributor

@eileenmcnaughton i have just re-tested the removal and confirm it still fails as per @Stoob 's comment

backtrace is

    [to_string] => [db_error: message="DB Error: syntax error" code=-2 mode=callback callback=CRM_Core_Error::exceptionHandler prefix="" info="SELECT *
FROM civicrm_contact
WHERE (id IN ())
 [nativecode=1064 ** You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))' at line 3]"]
)


Jun 26 08:02:55  [info] $backTrace = #0 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/Error.php(952): CRM_Core_Error::backtrace("backTrace", TRUE)
#1 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(922): CRM_Core_Error::exceptionHandler(Object(DB_Error))
#2 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB.php(985): PEAR_Error->__construct("DB Error: syntax error", -2, 16, (Array:2), "SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n [nativecode=1064 ** You ha...")
#3 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(575): DB_Error->__construct(-2, 16, (Array:2), "SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n [nativecode=1064 ** You ha...")
#4 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/vendor/pear/pear-core-minimal/src/PEAR.php(223): PEAR->_raiseError(Object(DB_mysqli), NULL, -2, 16, (Array:2), "SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n [nativecode=1064 ** You ha...", "DB_Error", TRUE)
#5 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB/common.php(1907): PEAR->__call("raiseError", (Array:7))
#6 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB/mysqli.php(933): DB_common->raiseError(-2, NULL, NULL, "SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n [nativecode=1064 ** You ha...", "1064 ** You have an error in your SQL syntax; check the manual that correspon...")
#7 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB/mysqli.php(403): DB_mysqli->mysqliRaiseError()
#8 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB/common.php(1216): DB_mysqli->simpleQuery("SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n")
#9 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB/DataObject.php(2415): DB_common->query("SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n")
#10 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/DB/DataObject.php(1607): DB_DataObject->_query("SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n")
#11 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/DAO.php(439): DB_DataObject->query("SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n")
#12 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/DAO.php(1414): CRM_Core_DAO->query("SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n", TRUE)
#13 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Utils/SQL/Select.php(517): CRM_Core_DAO::executeQuery("SELECT *\nFROM civicrm_contact\nWHERE (id IN ())\n", (Array:0), TRUE, NULL, FALSE, TRUE, FALSE)
#14 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Contact/Form/Task.php(491): CRM_Utils_SQL_Select->execute()
#15 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Contact/Form/Task.php(272): CRM_Contact_Form_Task::getSelectedContactNames()
#16 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Contact/Form/Task.php(91): CRM_Contact_Form_Task::preProcessCommon(Object(CRM_Contact_Form_Task_RemoveFromGroup))
#17 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/Form.php(590): CRM_Contact_Form_Task->preProcess()
#18 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/QuickForm/Action/Display.php(92): CRM_Core_Form->buildForm()
#19 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/HTML/QuickForm/Controller.php(203): CRM_Core_QuickForm_Action_Display->perform(Object(CRM_Contact_Form_Task_RemoveFromGroup), "display")
#20 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/packages/HTML/QuickForm/Page.php(103): HTML_QuickForm_Controller->handle(Object(CRM_Contact_Form_Task_RemoveFromGroup), "display")
#21 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/Controller.php(351): HTML_QuickForm_Page->handle("display")
#22 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/Invoke.php(311): CRM_Core_Controller->run((Array:3), (Array:0))
#23 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/Invoke.php(85): CRM_Core_Invoke::runItem((Array:14))
#24 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/CRM/Core/Invoke.php(52): CRM_Core_Invoke::_invoke((Array:3))
#25 /home/seamus/buildkit/build/47-test/sites/all/modules/civicrm/drupal/civicrm.module(444): CRM_Core_Invoke::invoke((Array:3))
#26 /home/seamus/buildkit/build/47-test/includes/menu.inc(527): civicrm_invoke("group", "search")
#27 /home/seamus/buildkit/build/47-test/index.php(21): menu_execute_active_handler()

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 so this affects the rc? Let's get a new regression gitlab opened & close this

@seamuslee001
Copy link
Contributor

Closing this as another PR has merged this stuff tracking the other bug here https://lab.civicrm.org/dev/core/issues/1074

@eileenmcnaughton eileenmcnaughton deleted the group_search_fix branch June 25, 2019 22:17
@eileenmcnaughton
Copy link
Contributor Author

thansk @seamuslee001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwire mattwire mattwire left review comments

@seamuslee001 seamuslee001 seamuslee001 left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eileenmcnaughton @mattwire @seamuslee001 @Stoob