From fe676f334aa07489092b4d6af61d643ecb95aa27 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Tue, 5 Oct 2021 12:12:31 -0600 Subject: [PATCH] FEATURE: Return subcategories on categories endpoint (#14492) * FEATURE: Return subcategories on categories endpoint When using the API subcategories will now be returned nested inside of each category response under the `subcategory_list` param. We already return all the subcategory ids under the `subcategory_ids` param, but you then would have to make multiple separate API calls to fetch each of those subcategories. This way you can get **ALL** of the categories along with their subcategories in a single API response. The UI will not be affected by this change because you need to pass in the `include_subcategories=true` param in order for subcategories to be returned. In a follow up PR I'll add the API scoping for fetching categories so that a readonly API key can be used for the `/categories.json` endpoint. This endpoint should be used instead of the `/site.json` endpoint for fetching a sites categories and subcategories. * Update PR based on feedback - Have spec check for specific subcategory - Move comparison check out of loop - Only populate subcategory list if option present - Remove empty array initialization - Update api spec to allow null response * More PR updates based on feedback - Use a category serializer for the subcategory_list - Don't include the subcategory_list param if empty - For the spec check for the subcategory by id - Fix spec to account for param not present when empty --- app/controllers/categories_controller.rb | 3 +- app/models/category.rb | 2 +- app/models/category_list.rb | 20 ++++++-- .../category_detailed_serializer.rb | 6 +++ .../schemas/json/category_list_response.json | 9 ++++ spec/requests/categories_controller_spec.rb | 49 +++++++++++++++++++ 6 files changed, 83 insertions(+), 6 deletions(-) diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 2931dbc42ec..4e7337eaf0d 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -26,7 +26,8 @@ class CategoriesController < ApplicationController category_options = { is_homepage: current_homepage == "categories", parent_category_id: params[:parent_category_id], - include_topics: include_topics(parent_category) + include_topics: include_topics(parent_category), + include_subcategories: params[:include_subcategories] == "true" } @category_list = CategoryList.new(guardian, category_options) diff --git a/app/models/category.rb b/app/models/category.rb index 07b890e0644..630a74c425a 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -139,7 +139,7 @@ class Category < ActiveRecord::Base # permission is just used by serialization # we may consider wrapping this in another spot - attr_accessor :displayable_topics, :permission, :subcategory_ids, :notification_level, :has_children + attr_accessor :displayable_topics, :permission, :subcategory_ids, :subcategory_list, :notification_level, :has_children # Allows us to skip creating the category definition topic in tests. attr_accessor :skip_category_definition diff --git a/app/models/category_list.rb b/app/models/category_list.rb index 4144f79b423..655edfa194e 100644 --- a/app/models/category_list.rb +++ b/app/models/category_list.rb @@ -100,6 +100,8 @@ class CategoryList @categories = @categories.to_a + include_subcategories = @options[:include_subcategories] == true + notification_levels = CategoryUser.notification_levels_for(@guardian.user) default_notification_level = CategoryUser.default_notification_level @@ -111,16 +113,26 @@ class CategoryList end if @options[:parent_category_id].blank? - subcategories = {} + subcategory_ids = {} + subcategory_list = {} to_delete = Set.new @categories.each do |c| if c.parent_category_id.present? - subcategories[c.parent_category_id] ||= [] - subcategories[c.parent_category_id] << c.id + subcategory_ids[c.parent_category_id] ||= [] + subcategory_ids[c.parent_category_id] << c.id + if include_subcategories + subcategory_list[c.parent_category_id] ||= [] + subcategory_list[c.parent_category_id] << c + end to_delete << c end end - @categories.each { |c| c.subcategory_ids = subcategories[c.id] || [] } + @categories.each do |c| + c.subcategory_ids = subcategory_ids[c.id] || [] + if include_subcategories + c.subcategory_list = subcategory_list[c.id] || [] + end + end @categories.delete_if { |c| to_delete.include?(c) } end diff --git a/app/serializers/category_detailed_serializer.rb b/app/serializers/category_detailed_serializer.rb index 6b1e5083b20..1349cd44aa8 100644 --- a/app/serializers/category_detailed_serializer.rb +++ b/app/serializers/category_detailed_serializer.rb @@ -14,10 +14,16 @@ class CategoryDetailedSerializer < BasicCategorySerializer has_many :displayable_topics, serializer: ListableTopicSerializer, embed: :objects, key: :topics + has_many :subcategory_list, serializer: CategoryDetailedSerializer, embed: :objects, key: :subcategory_list + def include_displayable_topics? displayable_topics.present? end + def include_subcategory_list? + subcategory_list.present? + end + def is_uncategorized object.id == SiteSetting.uncategorized_category_id end diff --git a/spec/requests/api/schemas/json/category_list_response.json b/spec/requests/api/schemas/json/category_list_response.json index 1aa3c8f0648..fc3c424a4b4 100644 --- a/spec/requests/api/schemas/json/category_list_response.json +++ b/spec/requests/api/schemas/json/category_list_response.json @@ -141,6 +141,15 @@ ] }, + "subcategory_list": { + "type": [ + "array", + "null" + ], + "items": [ + + ] + }, "uploaded_logo": { "type": [ "string", diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 2cfed06d614..2067279cf13 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -68,6 +68,55 @@ describe CategoriesController do ) end + it 'does not returns subcatgories without permission' do + subcategory = Fabricate(:category, user: admin, parent_category: category) + subcategory.set_permissions(admins: :full) + subcategory.save! + + sign_in(user) + + get "/categories.json?include_subcategories=true" + + expect(response.status).to eq(200) + + category_list = response.parsed_body["category_list"] + + subcategories_for_category = category_list["categories"][1]["subcategory_list"] + expect(subcategories_for_category).to eq(nil) + end + + it 'returns the right subcategory response with permission' do + subcategory = Fabricate(:category, user: admin, parent_category: category) + + sign_in(user) + + get "/categories.json?include_subcategories=true" + + expect(response.status).to eq(200) + + category_list = response.parsed_body["category_list"] + + subcategories_for_category = category_list["categories"][1]["subcategory_list"] + expect(subcategories_for_category.count).to eq(1) + expect(subcategories_for_category.first["parent_category_id"]).to eq(category.id) + expect(subcategories_for_category.first["id"]).to eq(subcategory.id) + end + + it 'does not return subcategories without query param' do + subcategory = Fabricate(:category, user: admin, parent_category: category) + + sign_in(user) + + get "/categories.json" + + expect(response.status).to eq(200) + + category_list = response.parsed_body["category_list"] + + subcategories_for_category = category_list["categories"][1]["subcategory_list"] + expect(subcategories_for_category).to eq(nil) + end + it 'does not show uncategorized unless allow_uncategorized_topics' do SiteSetting.desktop_category_page_style = "categories_boxes_with_topics"