From 6c22bad77a12e316c2d917562642107687ef3960 Mon Sep 17 00:00:00 2001 From: James Cole Date: Wed, 17 Feb 2016 21:03:59 +0100 Subject: [PATCH] Revamped a part of the rule test code. - clean way of constructing triggers - processor can be constructed in a number of ways. - cleaner transaction matcher using collections instead of arrays --- app/Http/Controllers/RuleController.php | 27 ++++++-- app/Rules/Processor.php | 69 +++++++++++++++---- app/Rules/TransactionMatcher.php | 88 +++++++++++++------------ app/Rules/Triggers/AbstractTrigger.php | 32 +++++++-- app/Rules/Triggers/TriggerFactory.php | 45 +++++++++---- 5 files changed, 178 insertions(+), 83 deletions(-) diff --git a/app/Http/Controllers/RuleController.php b/app/Http/Controllers/RuleController.php index 43839ef11a..194d12e8fd 100644 --- a/app/Http/Controllers/RuleController.php +++ b/app/Http/Controllers/RuleController.php @@ -267,13 +267,23 @@ class RuleController extends Controller */ public function testTriggers(TestRuleFormRequest $request) { - $triggers = [ + // build trigger array from response TODO move to function: + $triggers = []; + $data = [ 'rule-triggers' => $request->get('rule-trigger'), 'rule-trigger-values' => $request->get('rule-trigger-value'), 'rule-trigger-stop' => $request->get('rule-trigger-stop'), ]; + foreach ($data['rule-triggers'] as $index => $triggerType) { + $triggers[] = [ + 'type' => $triggerType, + 'value' => $data['rule-trigger-values'][$index], + 'stopProcessing' => intval($data['rule-trigger-stop'][$index]) === 1 ? true : false, + ]; + } - if (count($triggers['rule-triggers']) == 0) { + + if (count($triggers) == 0) { return Response::json(['html' => '', 'warning' => trans('firefly.warning_no_valid_triggers')]); } @@ -281,12 +291,15 @@ class RuleController extends Controller $range = Config::get('firefly.test-triggers.range'); $matcher = new TransactionMatcher; - + $matcher->setLimit($limit); + $matcher->setRange($range); + $matcher->setTriggers($triggers); + $matchingTransactions = $matcher->findMatchingTransactions(); // Dispatch the actual work to a matched object - $matchingTransactions - = (new TransactionMatcher($triggers)) - ->setTransactionLimit($range) - ->findMatchingTransactions($limit); + // $matchingTransactions + // = (new TransactionMatcher($triggers)) + // ->setTransactionLimit($range) + // ->findMatchingTransactions($limit); // Warn the user if only a subset of transactions is returned if (count($matchingTransactions) == $limit) { diff --git a/app/Rules/Processor.php b/app/Rules/Processor.php index 5d0ce78811..b575e59e28 100644 --- a/app/Rules/Processor.php +++ b/app/Rules/Processor.php @@ -16,8 +16,8 @@ use FireflyIII\Models\RuleTrigger; use FireflyIII\Models\TransactionJournal; use FireflyIII\Rules\Actions\ActionFactory; use FireflyIII\Rules\Actions\ActionInterface; +use FireflyIII\Rules\Triggers\AbstractTrigger; use FireflyIII\Rules\Triggers\TriggerFactory; -use FireflyIII\Rules\Triggers\TriggerInterface; use Illuminate\Support\Collection; use Log; @@ -43,7 +43,8 @@ class Processor */ private function __construct() { - + $this->triggers = new Collection; + $this->actions = new Collection; } /** @@ -53,16 +54,55 @@ class Processor */ public static function make(Rule $rule) { - $self = new self; - $self->rule = $rule; - $self->triggers = $rule->ruleTriggers()->orderBy('order', 'ASC')->get(); - $self->actions = $rule->ruleActions()->orderBy('order', 'ASC')->get(); + $self = new self; + $self->rule = $rule; + + $triggerSet = $rule->ruleTriggers()->orderBy('order', 'ASC')->get(); + /** @var RuleTrigger $trigger */ + foreach ($triggerSet as $trigger) { + $self->triggers->push(TriggerFactory::getTrigger($trigger)); + } + $self->actions = $rule->ruleActions()->orderBy('order', 'ASC')->get(); return $self; } + /** + * @param string $triggerName + * @param string $triggerValue + * + * @return Processor + */ + public static function makeFromString(string $triggerName, string $triggerValue) + { + $self = new self; + $trigger = TriggerFactory::makeTriggerFromStrings($triggerName, $triggerValue, false); + $self->triggers->push($trigger); + + return $self; + } + + /** + * @param array $triggers + * + * @return Processor + */ + public static function makeFromStringArray(array $triggers) + { + $self = new self; + foreach ($triggers as $entry) { + $trigger = TriggerFactory::makeTriggerFromStrings($entry['type'], $entry['value'], $entry['stopProcessing']); + $self->triggers->push($trigger); + } + + return $self; + } + + /** * @param TransactionJournal $journal + * + * @return bool */ public function handleTransactionJournal(TransactionJournal $journal) { @@ -70,10 +110,15 @@ class Processor // get all triggers: $triggered = $this->triggered(); if ($triggered) { - Log::debug('Rule #' . $this->rule->id . ' was triggered. Now process each action.'); - $this->actions(); + if ($this->actions->count() > 0) { + $this->actions(); + } + + return true; } + return false; + } /** @@ -112,13 +157,11 @@ class Processor foreach ($this->triggers as $trigger) { $foundTriggers++; - /** @var TriggerInterface $triggerObject */ - $triggerObject = TriggerFactory::getTrigger($trigger); - // no need to keep pushing the journal around! - if ($triggerObject->triggered($this->journal)) { + /** @var AbstractTrigger $trigger */ + if ($trigger->triggered($this->journal)) { $hitTriggers++; } - if ($trigger->stop_processing) { + if ($trigger->stopProcessing) { break; } diff --git a/app/Rules/TransactionMatcher.php b/app/Rules/TransactionMatcher.php index 9570c871c1..2d66ff9e41 100644 --- a/app/Rules/TransactionMatcher.php +++ b/app/Rules/TransactionMatcher.php @@ -11,8 +11,10 @@ declare(strict_types = 1); namespace FireflyIII\Rules; use FireflyIII\Models\Rule; +use FireflyIII\Models\TransactionJournal; use FireflyIII\Models\TransactionType; use FireflyIII\Repositories\Journal\JournalRepositoryInterface; +use Illuminate\Support\Collection; /** * Class TransactionMatcher is used to find a list of @@ -22,11 +24,10 @@ use FireflyIII\Repositories\Journal\JournalRepositoryInterface; */ class TransactionMatcher { - /** @var int Maximum number of transaction to search in (for performance reasons) * */ - private $range = 200; /** @var int */ private $limit = 10; - + /** @var int Maximum number of transaction to search in (for performance reasons) * */ + private $range = 200; /** @var array */ private $transactionTypes = [TransactionType::DEPOSIT, TransactionType::WITHDRAWAL, TransactionType::TRANSFER]; /** @var array List of triggers to match */ @@ -43,25 +44,18 @@ class TransactionMatcher { /** @var JournalRepositoryInterface $repository */ $repository = app('FireflyIII\Repositories\Journal\JournalRepositoryInterface'); - - // We don't know the number of transaction to fetch from the database, in - // order to return the proper number of matching transactions. Since we don't want - // to fetch all transactions (as the first transactions already match, or the last - // transactions are irrelevant), we will fetch data in pages. - - // The optimal pagesize is somewhere between the maximum number of results to be returned - // and the maximum number of transactions to consider. - $pagesize = min($this->range / 2, $maxResults * 2); + $pagesize = min($this->range / 2, $this->limit * 2); // Variables used within the loop $numTransactionsProcessed = 0; $page = 1; - $matchingTransactions = []; + $matchingTransactions = new Collection(); // Flags to indicate the end of the loop $reachedEndOfList = false; $foundEnoughTransactions = false; $searchedEnoughTransactions = false; + $processor = Processor::makeFromStringArray($this->triggers); // Start a loop to fetch batches of transactions. The loop will finish if: // - all transactions have been fetched from the database @@ -69,51 +63,44 @@ class TransactionMatcher // - the maximum number of transactions to search in have been searched do { // Fetch a batch of transactions from the database - $offset = $page > 0 ? ($page - 1) * $pagesize : 0; - $transactions = $repository->getJournalsOfTypes($this->transactionTypes, $offset, $page, $pagesize)->getCollection()->all(); + $offset = $page > 0 ? ($page - 1) * $pagesize : 0; + $set = $repository->getCollectionOfTypes($this->transactionTypes, $offset, $pagesize); - // Filter transactions that match the rule - $matchingTransactions += array_filter( - $transactions, function ($transaction) { - $processor = new Processor(new Rule, $transaction); - - return $processor->isTriggeredBy($this->triggers); - } + // Filter transactions that match the given triggers. + $filtered = $set->filter( + function (TransactionJournal $journal) use ($processor) { + return $processor->handleTransactionJournal($journal); + } ); + // merge: + $matchingTransactions = $matchingTransactions->merge($filtered); + +// $matchingTransactions += array_filter( +// $set, function ($transaction) { +// $processor = new Processor(new Rule, $transaction); +// +// return $processor->isTriggeredBy($this->triggers); +// } +// ); + // Update counters $page++; - $numTransactionsProcessed += count($transactions); + $numTransactionsProcessed += count($set); // Check for conditions to finish the loop - $reachedEndOfList = (count($transactions) < $pagesize); - $foundEnoughTransactions = (count($matchingTransactions) >= $maxResults); + $reachedEndOfList = (count($set) < $pagesize); + $foundEnoughTransactions = (count($matchingTransactions) >= $this->limit); $searchedEnoughTransactions = ($numTransactionsProcessed >= $this->range); } while (!$reachedEndOfList && !$foundEnoughTransactions && !$searchedEnoughTransactions); // If the list of matchingTransactions is larger than the maximum number of results // (e.g. if a large percentage of the transactions match), truncate the list - $matchingTransactions = array_slice($matchingTransactions, 0, $maxResults); + $matchingTransactions = $matchingTransactions->slice(0, $this->limit); return $matchingTransactions; } - /** - * @return int - */ - public function getRange() - { - return $this->range; - } - - /** - * @param int $range - */ - public function setRange($range) - { - $this->range = $range; - } - /** * @return int */ @@ -130,6 +117,22 @@ class TransactionMatcher $this->limit = $limit; } + /** + * @return int + */ + public function getRange() + { + return $this->range; + } + + /** + * @param int $range + */ + public function setRange($range) + { + $this->range = $range; + } + /** * @return array */ @@ -147,5 +150,4 @@ class TransactionMatcher } - } diff --git a/app/Rules/Triggers/AbstractTrigger.php b/app/Rules/Triggers/AbstractTrigger.php index 5263073946..a2d0f65217 100644 --- a/app/Rules/Triggers/AbstractTrigger.php +++ b/app/Rules/Triggers/AbstractTrigger.php @@ -21,6 +21,8 @@ use FireflyIII\Models\TransactionJournal; */ class AbstractTrigger { + /** @var bool */ + public $stopProcessing; /** @var string */ protected $checkValue; /** @var TransactionJournal */ @@ -38,6 +40,20 @@ class AbstractTrigger } + /** + * @param string $triggerValue + * @param bool $stopProcessing + * + * @return static + */ + public static function makeFromStrings(string $triggerValue, bool $stopProcessing) + { + $self = new static; + $self->triggerValue = $triggerValue; + $self->stopProcessing = $stopProcessing; + return $self; + } + /** * @param RuleTrigger $trigger * @@ -45,9 +61,10 @@ class AbstractTrigger */ public static function makeFromTrigger(RuleTrigger $trigger) { - $self = new self; - $self->trigger = $trigger; - $self->triggerValue = $trigger->trigger_value; + $self = new static; + $self->trigger = $trigger; + $self->triggerValue = $trigger->trigger_value; + $self->stopProcessing = $trigger->stop_processing; return $self; } @@ -58,10 +75,11 @@ class AbstractTrigger */ public static function makeFromTriggerAndJournal(RuleTrigger $trigger, TransactionJournal $journal) { - $self = new self; - $self->trigger = $trigger; - $self->triggerValue = $trigger->trigger_value; - $self->journal = $journal; + $self = new static; + $self->trigger = $trigger; + $self->triggerValue = $trigger->trigger_value; + $self->stopProcessing = $trigger->stop_processing; + $self->journal = $journal; } /** diff --git a/app/Rules/Triggers/TriggerFactory.php b/app/Rules/Triggers/TriggerFactory.php index 01d19963d9..d104070e06 100644 --- a/app/Rules/Triggers/TriggerFactory.php +++ b/app/Rules/Triggers/TriggerFactory.php @@ -38,9 +38,40 @@ class TriggerFactory $class = self::getTriggerClass($triggerType); $obj = $class::makeFromTriggerValue($trigger->trigger_value); + // this is a massive HACK. TODO. + $obj->databaseObject = $trigger; + return $obj; } + /** + * @param string $triggerType + * @param string $triggerValue + * + * @return AbstractTrigger + * @throws FireflyException + */ + public static function makeTriggerFromStrings(string $triggerType, string $triggerValue, bool $stopProcessing) + { + /** @var AbstractTrigger $class */ + $class = self::getTriggerClass($triggerType); + $obj = $class::makeFromStrings($triggerValue, $stopProcessing); + + return $obj; + } + + /** + * Returns a map with triggertypes, mapped to the class representing that type + */ + protected static function getTriggerTypes() + { + if (!self::$triggerTypes) { + self::$triggerTypes = Domain::getRuleTriggers(); + } + + return self::$triggerTypes; + } + /** * Returns the class name to be used for triggers with the given name * @@ -49,7 +80,7 @@ class TriggerFactory * @return TriggerInterface|string * @throws FireflyException */ - public static function getTriggerClass(string $triggerType): string + private static function getTriggerClass(string $triggerType): string { $triggerTypes = self::getTriggerTypes(); @@ -64,16 +95,4 @@ class TriggerFactory return $class; } - - /** - * Returns a map with triggertypes, mapped to the class representing that type - */ - protected static function getTriggerTypes() - { - if (!self::$triggerTypes) { - self::$triggerTypes = Domain::getRuleTriggers(); - } - - return self::$triggerTypes; - } }