Skip to content

Fixed SQL for "exclude" parameter#1

Merged
jpdevries merged 1 commit intomodxcms:developfrom
jrotering:develop
Oct 2, 2013
Merged

Fixed SQL for "exclude" parameter#1
jpdevries merged 1 commit intomodxcms:developfrom
jrotering:develop

Conversation

@jrotering
Copy link

IDs listed in the 'exclude' parameter were being put in a "NOT IN" clause, but not being removed from the "IN" clause. As a result those resources were not being properly excluded at least some of the time (not sure if this behavior was consistent across all environments).

I don't know if something similar may need to be done in simplesearchdriversolr.class.php.

IDs listed in the 'exclude' parameter were being put in a "NOT IN" clause, but not being removed from the "IN" clause. As a result those resources were not being properly excluded at least some of the time (not sure if this behavior was consistent across all environments).

I don't know if something similar may need to be done in simplesearchdriversolr.class.php.
jpdevries added a commit that referenced this pull request Oct 2, 2013
Fixed SQL for "exclude" parameter
@jpdevries jpdevries merged commit f19ddfd into modxcms:develop Oct 2, 2013
@renekopcem
Copy link

I don't think this should be merged. We should do clean up of the commented code inside.

Choose a reason for hiding this comment

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

@jrotering what is the #$c for?

Copy link
Author

Choose a reason for hiding this comment

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

It is the line that used to build the "NOT IN" clause, commented out. I expected someone to remove 165-166 before merging this, but thought there was a remote possibility that the "NOT IN" was important in ways I didn't understand, so I left it as commented-out.

@jpdevries
Copy link

ok so this PR just comments out one line, and adds a comment. I can remove both of those lines can be removed before merging with production. @jrotering are there any specific tests that come to mind to ensure this works correctly and also doesn't introduce any new issues?

matdave pushed a commit that referenced this pull request Jul 25, 2022
Adding option for boosting based on tvs
matdave pushed a commit that referenced this pull request Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants