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

fix created_date/modified_date relative date filter searches #16287

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

MegaphoneJon
Copy link
Contributor

Overview

There are two problems when using relative date filters that don't define both a begin and end date. E.g. "To end of previous year" or "From start of current week", as opposed to those that define a begin AND end date - e.g. "This month".

Before

SQL would be generated incorrectly, resulting in a syntax error.
After fixing this, I also noticed that there was a copy/paste error - when a start date but no end date is defined, the end date (always NULL) is used as the start date.

After

No syntax error in SQL.

Technical Details

The if statement that has the comment Special handling for contact table as it has a known alias in advanced search. was previously only invoked when both start and end dates are defined. I pulled it out to run a bit sooner, and also to run on the $secondWhere variable.

@civibot
Copy link

civibot bot commented Jan 13, 2020

(Standard links)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@seamuslee001 seamuslee001 changed the base branch from master to 5.22 January 14, 2020 03:11
@civibot civibot bot removed the master label Jan 14, 2020
@seamuslee001
Copy link
Contributor

(CiviCRM Review Template WORD-1.2)

  • General standards
    • (r-explain) Pass
    • (r-user) Pass
    • (r-doc)
    • (r-run) Pass I was able to replicate the issue and confirmed that this fixed. I also tested the Event Active On date which is where the second where would likely come into things and confirmed that still works.
  • Developer standards

@eileenmcnaughton eileenmcnaughton merged commit 5698e34 into civicrm:5.22 Jan 14, 2020
@MegaphoneJon
Copy link
Contributor Author

Since this was a 5.20 regression, and it can break in unexpected ways (if used in a smart group), I think this makes sense for 5.21. Happy to submit a PR, also fine if that's not how it works, it's in my patch set regardless.

@eileenmcnaughton
Copy link
Contributor

@MegaphoneJon that would be appreciated

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.

None yet

3 participants