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#876) Allow url to set IS NULL/ NOT NULL in report for opera… #14052

Merged
merged 1 commit into from
Apr 20, 2019

Conversation

yashodha
Copy link
Contributor

@yashodha yashodha commented Apr 15, 2019

…tions

Overview

This depends on #14028
Now that we allow IS NULL/ NOT NULL for filters like campaign, we should also allow the params to be passed via url.

Before

Operator not respected

op_before

After

Operator is respected
op_after

Comments

Filters can be passes via url for is null/is not null for fields like campaigns, etc

@civibot
Copy link

civibot bot commented Apr 15, 2019

(Standard links)

@civibot civibot bot added the master label Apr 15, 2019
@demeritcowboy
Copy link
Contributor

demeritcowboy commented Apr 16, 2019

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] COMMENTS It works but I get a warning Notice: Undefined index: campaign_id_value in CRM_Report_Utils_Report::getPreviewCriteriaQueryParams() (line 524 of /home/jenkins/bknix-dfl/build/core-14052-2ari/sites/all/modules/civicrm/CRM/Report/Utils/Report.php) but I get this for anything I try to put in the url so I'm not sure it's related. It doesn't happen when I use the dropdown widget. If I try to use nnll for a date field in the url I get a slightly different warning, but again I don't think it's related to this change since I see similar on dmaster.demo. Notice: Undefined offset: 1 in CRM_Utils_Date::getFromTo() (line 875 of /home/jenkins/bknix-dfl/build/core-14052-2ari/sites/all/modules/civicrm/CRM/Utils/Date.php).
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] Undecided
    • [r-test] PASS

@yashodha
Copy link
Contributor Author

@demeritcowboy : as the notices are not related, will handle in separate PR. Thanks!

@mattwire
Copy link
Contributor

@eileenmcnaughton This feels like it needs a quick look from you. It's been reviewed by @demeritcowboy but just want to check that you agree with the url params

@pradpnayak
Copy link
Contributor

I think this is GOOD TO MERGE as the options matches with the drop the dropdown. I also QA'd with both options on Contribution and Contact report and it works fine.

@yashodha
Copy link
Contributor Author

@pradpnayak thanks for testing!

@colemanw colemanw merged commit 1f6f882 into civicrm:master Apr 20, 2019
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.

5 participants