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

dev/core#1297 If start/end date is empty, results are empty #15415

Merged
merged 1 commit into from
Nov 2, 2019

Conversation

VangelisP
Copy link
Contributor

Overview

If user has selected the 'Choose date range' date filtering option and he/she doesn't populate the end date field, the end date value becomes 0 and not a valid MySQL date, thus returning 0 results.
Related issue: https://lab.civicrm.org/dev/core/issues/1297

Before

No results are coming back

After

Results are coming back

Technical Details

If end date is empty, the clause produces contrib.receive_date <= '0'

Expected behaviour: contrib.receive_date <= <current_date & time>

Similarly, if start date is empty, we need to have valid date format.

Comments

@civibot
Copy link

civibot bot commented Oct 7, 2019

(Standard links)

@civibot civibot bot added the master label Oct 7, 2019
@eileenmcnaughton eileenmcnaughton changed the title If start/end date is empty, results are empty dev/core#1297 If start/end date is empty, results are empty Oct 7, 2019
@jitendrapurohit
Copy link
Contributor

Currently(after this patch), if I select Choose date range and select date values in both start and end date fields, the result is not correct. @VangelisP Can you update this PR to fix this problem? I can then proceed with further testing of this PR.

Thanks.

@VangelisP
Copy link
Contributor Author

I just realized that there appears to be another issue affecting this search: Relative dates are not being converted to absolute.

The$ formvalue being sent to CRM/Contact/BAO/Query.php has receive_date_relative and due to the fact that it's in the whitelist : https://lab.civicrm.org/dev/core/blob/master/CRM/Contact/BAO/Query.php#L1590 , it doesn't alter the relative to absolute dates.

Which means that regardless the relative date that you put, results are always the same.

I suppose I would need to create a separate issue for that bug.

@jitendrapurohit
Copy link
Contributor

@VangelisP I think if you can remove the complete for loop and replace it with the below code, you should get the date field working correctly on the aggregate search form

list($relativeFrom, $relativeTo) = CRM_Utils_Date::getFromTo($dateParams['receive_date_relative'], $dateParams['receive_date_low'], $dateParams['receive_date_high']);
if ($relativeFrom) {
  $clauses[] = "contrib.receive_date >= '{$relativeFrom}'";
}
if ($relativeTo) {
  $clauses[] = "contrib.receive_date <= '{$relativeTo}'";
}

Can you update this PR if the above snippet works for you?

Thanks.

@VangelisP
Copy link
Contributor Author

Hi @jitendrapurohit , I've used your code example and enriched it.

Now it works with relative/absolute dates as also with specified times or even empty date filters as well.

@yashodha
Copy link
Contributor

@VangelisP
Can you please squash all your commits?

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit is this good to merge?

@jitendrapurohit
Copy link
Contributor

@eileenmcnaughton yes, I've re-tested the changes and this looks good to be merged.

Thanks @VangelisP

@eileenmcnaughton
Copy link
Contributor

Merging based on @jitendrapurohit review

@VangelisP Can you commit to re-testing this during the rc - there have been a few moving parts in 5.20 that touch on dates in searches & it would be good to get some final testing on it

@eileenmcnaughton eileenmcnaughton merged commit 33b5008 into civicrm:master Nov 2, 2019
@VangelisP
Copy link
Contributor Author

Sure @eileenmcnaughton

@VangelisP VangelisP deleted the dev_issue#1297 branch November 2, 2019 08:20
@VangelisP
Copy link
Contributor Author

As a follow-up as asked, @eileenmcnaughton I just re-did a test with 5.20.beta1 and this is working perfectly.

@eileenmcnaughton
Copy link
Contributor

@VangelisP - thanks!

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.

4 participants