-
Notifications
You must be signed in to change notification settings - Fork 191
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
Feature: Update all form goal blocks/shortcodes to respect the new v3 fields/queries #7380
Feature: Update all form goal blocks/shortcodes to respect the new v3 fields/queries #7380
Conversation
|
||
return Money::ofMinor($results->total, give_get_option('currency'))->getAmount(); | ||
foreach($this->getForms() as $formId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alaca what do you think about updating the query to accept multiple form IDs? We could update the where
to be a whereIn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea. I added a new method forms
where you can pass an array of form IDs. See 35eb534
*/ | ||
$form_income = 0; | ||
|
||
if ($args['start_date'] === $args['end_date']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might suggest instantiating the query and setting the ->form()
value and then conditionally setting a between()
value, as opposed to duplicating the new DonationQuery
code.
On that note, I've considered adding a ->tap($callback)
method to the DonationQuery
class which would be great for this.
For example:
(new DonationQuery)
->form(1)
->tap(function($query) {
if($startDate && $endDate) {
$query->between($startDate, $endDate);
}
})
->sumIndendedAmount()
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored it a bit 7ea042e
If you could add the tap
method, that would be great.
Description
This PR updates all form goal blocks/shortcodes to respect the new v3 fields/queries.
[give_goal]
shortcode is updated and now it supports two new argumentsstart_date
andend_date
where the user can set a date range for the goal.Affects
Testing Instructions
Pre-review Checklist
@unreleased
tags included in DocBlocks