FIX: Category.find_by_slug

FIX: Category.find_by_slug

find_by_slug should ensure that the parent actually exists when its looking for a parent.

diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb
index 6f60b09cff..a548e6c815 100644
--- a/app/controllers/categories_controller.rb
+++ b/app/controllers/categories_controller.rb
@@ -206,6 +206,9 @@ class CategoriesController < ApplicationController
   def find_by_slug
     params.require(:category_slug)
     @category = Category.find_by_slug(params[:category_slug], params[:parent_category_slug])
+
+    raise Discourse::NotFound unless @category.present?
+
     if !guardian.can_see?(@category)
       if SiteSetting.detailed_404 && group = @category.access_category_via_group
         raise Discourse::InvalidAccess.new(
diff --git a/app/models/category.rb b/app/models/category.rb
index c0f66dbea9..a903c8f366 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -709,7 +709,8 @@ class Category < ActiveRecord::Base
 
   def self.find_by_slug(category_slug, parent_category_slug = nil)
     if parent_category_slug
-      parent_category_id = self.where(slug: parent_category_slug, parent_category_id: nil).pluck(:id).first
+      parent_category_id = self.where(slug: parent_category_slug, parent_category_id: nil).select(:id)
+
       self.where(slug: category_slug, parent_category_id: parent_category_id).first
     else
       self.where(slug: category_slug, parent_category_id: nil).first
diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb
index 35a58303df..225b87eeab 100644
--- a/spec/models/category_spec.rb
+++ b/spec/models/category_spec.rb
@@ -742,12 +742,36 @@ describe Category do
   end
 
   describe "find_by_slug" do
-    it "finds with category and sub category" do
-      category = Fabricate(:category_with_definition, slug: 'awesome-category')
-      sub_category = Fabricate(:category_with_definition, parent_category_id: category.id, slug: 'awesome-sub-category')
+    fab!(:category) do
+      Fabricate(:category_with_definition, slug: 'awesome-category')
+    end
 
+    fab!(:subcategory) do
+      Fabricate(
+        :category_with_definition,
+        parent_category_id: category.id,
+        slug: 'awesome-sub-category'
+      )
+    end
+
+    it "finds a category that exists" do
       expect(Category.find_by_slug('awesome-category')).to eq(category)
-      expect(Category.find_by_slug('awesome-sub-category', 'awesome-category')).to eq(sub_category)
+    end
+
+    it "finds a subcategory that exists" do
+      expect(Category.find_by_slug('awesome-sub-category', 'awesome-category')).to eq(subcategory)
+    end
+
+    it "produces nil if the parent doesn't exist" do
+      expect(Category.find_by_slug('awesome-sub-category', 'no-such-category')).to eq(nil)
+    end
+
+    it "produces nil if the parent doesn't exist and the requested category is a root category" do
+      expect(Category.find_by_slug('awesome-category', 'no-such-category')).to eq(nil)
+    end
+
+    it "produces nil if the subcategory doesn't exist" do
+      expect(Category.find_by_slug('no-such-category', 'awesome-category')).to eq(nil)
     end
   end

GitHub sha: 5f5b232c

2 Likes