Revise the smart playlist rule

rebienkrdns · Nov 18, 2018 · d45948e · d45948e
1 parent 26caca3
commit d45948e
Show file tree

Hide file tree

Showing 17 changed files with 352 additions and 316 deletions.
diff --git a/app/Models/Rule.php b/app/Models/Rule.php
@@ -8,14 +8,6 @@
 
 class Rule
 {
-    private const LOGIC_OR = 'or';
-    private const LOGIC_AND = 'and';
-
-    private const VALID_LOGICS = [
-        self::LOGIC_AND,
-        self::LOGIC_OR,
-    ];
-
     public const OPERATOR_IS = 'is';
     public const OPERATOR_IS_NOT = 'isNot';
     public const OPERATOR_CONTAINS = 'contains';
@@ -43,17 +35,14 @@ class Rule
     ];
 
     private $operator;
-    private $logic;
     private $value;
     private $model;
     private $parameterFactory;
 
     private function __construct(array $config)
     {
-        $this->validateLogic($config['logic']);
         $this->validateOperator($config['operator']);
 
-        $this->logic = $config['logic'];
         $this->value = $config['value'];
         $this->model = $config['model'];
         $this->operator = $config['operator'];
@@ -83,31 +72,18 @@ public function build(Builder $query, ?string $model = null): Builder
         // If the model is something like 'artist.name' or 'interactions.play_count', we have a subquery to deal with.
         // We handle such a case with a recursive call which, in theory, should work with an unlimited level of nesting,
         // though in practice we only have one level max.
-        $subQueryLogic = self::LOGIC_AND ? 'whereHas' : 'orWhereHas';
-
-        return $query->$subQueryLogic($fragments[0], function (Builder $subQuery) use ($fragments): Builder {
+        return $query->whereHas($fragments[0], function (Builder $subQuery) use ($fragments): Builder {
             return $this->build($subQuery, $fragments[1]);
         });
     }
 
+    /**
+     * Resolve the logic of a (sub)query base on the configured operator.
+     * Basically, if the operator is "between," we use "whereBetween." Otherwise, it's "where." Simple.
+     */
     private function resolveLogic(): string
     {
-        if ($this->operator === self::OPERATOR_IS_BETWEEN) {
-            return $this->logic === self::LOGIC_AND ? 'whereBetween' : 'orWhereBetween';
-        }
-
-        return $this->logic === self::LOGIC_AND ? 'where' : 'orWhere';
-    }
-
-    private function validateLogic(string $logic): void
-    {
-        if (!in_array($logic, self::VALID_LOGICS, true)) {
-            throw new InvalidArgumentException(
-                sprintf(
-                    '%s is not a valid value for logic. Valid values are: %s', $logic, implode(', ', self::VALID_LOGICS)
-                )
-            );
-        }
+        return $this->operator === self::OPERATOR_IS_BETWEEN ? 'whereBetween' : 'where';
     }
 
     private function validateOperator(string $operator): void

diff --git a/app/Services/SmartPlaylistService.php b/app/Services/SmartPlaylistService.php
@@ -30,18 +30,22 @@ public function getSongs(Playlist $playlist): Collection
 
         $rules = $this->addRequiresUserRules($playlist->rules, $playlist->user);
 
-        return $this->buildQueryForRules($rules)->get();
+        return $this->buildQueryFromRules($rules)->get();
     }
 
-    public function buildQueryForRules(array $rules): Builder
+    public function buildQueryFromRules(array $rules): Builder
     {
-        return tap(Song::query(), static function (Builder $query) use ($rules): Builder {
-            foreach ($rules as $config) {
-                $query = Rule::create($config)->build($query);
-            }
+        $query = Song::query();
 
-            return $query;
+        collect($rules)->each(static function (array $ruleGroup) use ($query): void {
+            $query->orWhere(static function (Builder $subQuery) use ($ruleGroup): void {
+                foreach ($ruleGroup['rules'] as $config) {
+                    Rule::create($config)->build($subQuery);
+                }
+            });
         });
+
+        return $query;
     }
 
     /**
@@ -51,26 +55,29 @@ public function buildQueryForRules(array $rules): Builder
      *
      * @param string[] $rules
      */
-    private function addRequiresUserRules(array $rules, User $user): array
+    public function addRequiresUserRules(array $rules, User $user): array
     {
-        $additionalRules = [];
+        foreach ($rules as &$ruleGroup) {
+            $additionalRules = [];
 
-        foreach ($rules as $rule) {
-            foreach (self::RULE_REQUIRES_USER_PREFIXES as $modelPrefix) {
-                if (starts_with($rule['model'], $modelPrefix)) {
-                    $additionalRules[] = $this->createRequireUserRule($user, $modelPrefix);
+            foreach ($ruleGroup['rules'] as &$config) {
+                foreach (self::RULE_REQUIRES_USER_PREFIXES as $modelPrefix) {
+                    if (starts_with($config['model'], $modelPrefix)) {
+                        $additionalRules[] = $this->createRequireUserRule($user, $modelPrefix);
+                    }
                 }
             }
+
+            // make sure all those additional rules are unique.
+            $ruleGroup['rules'] = array_merge($ruleGroup['rules'], collect($additionalRules)->unique('model')->all());
         }
 
-        // make sure all those additional rules are unique.
-        return array_merge($rules, collect($additionalRules)->unique('model')->all());
+        return $rules;
     }
 
     private function createRequireUserRule(User $user, string $modelPrefix): array
     {
         return [
-            'logic' => 'and',
             'model' => $modelPrefix.'user_id',
             'operator' => 'is',
             'value' => [$user->id],