From 6fb7361552139dfc47d3361fedff0b3cbb520678 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Fri, 28 Jan 2022 10:49:52 -0500 Subject: [PATCH] Extracting listing routes (#16338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fundamental goal in this refactor is to make it easier to disable the Listings feature set. By extracting a routes file (via the `draw` method [see implementation details][1]), we can more easily disable those routes _en masse_. This "crease in the code" could mean that we wouldn't need to update the controller action logic. An update of that logic might mean going into each controller action related to listing and adding a conditional for render 404 when listing is disabled. Now, on to why this should work (not I did not open the application and click around, but instead went with a "let the framework help me" mindset). I believe that this should work; though there's a caveat†. Here's why I believe this will work for extracting the listing routes: Before making changes I ran the following: ```sh $ rails routes | sort > original-routes.txt ``` Let's break that down: - `rails routes` :: renders the routing table for Rails; this is how Rails converts routes to the magic of stuff like `article_path(:id)` - `| sort` :: to sort the results - `> original-routes.txt` :: to write the results into a file. Then I made the changes and ran the following: ```sh $ rails routes | sort > updated-listing-routes.txt ``` And finally, I ran: ```sh $ diff updated-routes.txt original-routes.txt ``` The results were as follows: ```sh 500c500 < api_organization_listings GET (/locale/:locale)/api/organizations/:organization_username/listings(.:format) api/v0/organizations#listings {:locale=>nil, :format=>"json", :to=>"organizations#listings"} --- > api_organization_listings GET (/locale/:locale)/api/organizations/:organization_username/listings(.:format) api/v0/organizations#listings {:locale=>nil, :format=>"json"} ``` Note the only difference is that the above two lines, one has `:to=>"organizations#listings"` and the other does not. However, the `./spec/requests/api/v0/organizations_spec.rb` exercises the route; so I assume a hiccup/foible in the route reporting. There was no difference between the before and after files. † Caveat: if for some reason the order of the routes mattered, this will have messed that up. I don't think that is the case, but we shall see. Related to forem/rfcs#291 and #16335 --- config/routes.rb | 18 ++------------- config/routes/admin.rb | 3 --- config/routes/listing.rb | 27 ++++++++++++++++++++++ spec/requests/api/v0/organizations_spec.rb | 6 +++++ 4 files changed, 35 insertions(+), 19 deletions(-) create mode 100644 config/routes/listing.rb diff --git a/config/routes.rb b/config/routes.rb index 22dfb60b0..1dffa2f81 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -26,6 +26,7 @@ Rails.application.routes.draw do get "/locale/:locale", to: "stories#index" draw :admin + draw :listing namespace :stories, defaults: { format: "json" } do resource :feed, only: [:show] do @@ -36,8 +37,7 @@ Rails.application.routes.draw do end namespace :api, defaults: { format: "json" } do - scope module: :v0, - constraints: ApiConstraints.new(version: 0, default: true) do + scope module: :v0, constraints: ApiConstraints.new(version: 0, default: true) do resources :articles, only: %i[index show create update] do collection do get "me(/:status)", to: "articles#me", as: :me, constraints: { status: /published|unpublished|all/ } @@ -65,8 +65,6 @@ Rails.application.routes.draw do end resources :readinglist, only: [:index] - resources :listings, only: %i[index show create update] - get "/listings/category/:category", to: "listings#index", as: :listings_category get "/analytics/totals", to: "analytics#totals" get "/analytics/historical", to: "analytics#historical" get "/analytics/past_day", to: "analytics#past_day" @@ -83,7 +81,6 @@ Rails.application.routes.draw do resources :profile_images, only: %i[show], param: :username resources :organizations, only: [:show], param: :username do resources :users, only: [:index], to: "organizations#users" - resources :listings, only: [:index], to: "organizations#listings" resources :articles, only: [:index], to: "organizations#articles" end resource :instance, only: %i[show] @@ -156,7 +153,6 @@ Rails.application.routes.draw do resources :tag_adjustments, only: %i[create destroy] resources :rating_votes, only: [:create] resources :page_views, only: %i[create update] - resources :listings, only: %i[index new create edit update destroy dashboard] resources :credits, only: %i[index new create] do get "purchase", on: :collection, to: "credits#new" end @@ -193,17 +189,9 @@ Rails.application.routes.draw do get "/verify_email_ownership", to: "email_authorizations#verify", as: :verify_email_authorizations get "/search/tags", to: "search#tags" - get "/search/listings", to: "search#listings" get "/search/usernames", to: "search#usernames" get "/search/feed_content", to: "search#feed_content" get "/search/reactions", to: "search#reactions" - get "/listings/dashboard", to: "listings#dashboard" - get "/listings/:category", to: "listings#index", as: :listing_category - get "/listings/:category/:slug", to: "listings#index", as: :listing_slug - get "/listings/:category/:slug/:view", to: "listings#index", - constraints: { view: /moderate/ } - get "/listings/:category/:slug/delete_confirm", to: "listings#delete_confirm" - delete "/listings/:category/:slug", to: "listings#destroy" get "/notifications/:filter", to: "notifications#index", as: :notifications_filter get "/notifications/:filter/:org_id", to: "notifications#index", as: :notifications_filter_org get "/notification_subscriptions/:notifiable_type/:notifiable_id", to: "notification_subscriptions#show" @@ -221,7 +209,6 @@ Rails.application.routes.draw do get "/social_previews/user/:id", to: "social_previews#user", as: :user_social_preview get "/social_previews/organization/:id", to: "social_previews#organization", as: :organization_social_preview get "/social_previews/tag/:id", to: "social_previews#tag", as: :tag_social_preview - get "/social_previews/listing/:id", to: "social_previews#listing", as: :listing_social_preview get "/social_previews/comment/:id", to: "social_previews#comment", as: :comment_social_preview get "/async_info/base_data", controller: "async_info#base_data", defaults: { format: :json } @@ -264,7 +251,6 @@ Rails.application.routes.draw do # These routes are required by links in the sites and will most likely to be replaced by a db page get "/about", to: "pages#about" - get "/about-listings", to: "pages#about_listings" get "/security", to: "pages#bounty" get "/community-moderation", to: "pages#community_moderation" get "/faq", to: "pages#faq" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 9ac0eedfa..e2c72c863 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -156,9 +156,6 @@ namespace :admin do scope :apps do resources :consumer_apps, only: %i[index new create edit update destroy] - resources :listings, only: %i[index edit update destroy] - resources :listing_categories, only: %i[index edit update new create - destroy], path: "listings/categories" resources :welcome, only: %i[index create] end end diff --git a/config/routes/listing.rb b/config/routes/listing.rb new file mode 100644 index 000000000..5c0c9e5f7 --- /dev/null +++ b/config/routes/listing.rb @@ -0,0 +1,27 @@ +namespace :admin do + scope :apps do + resources :listings, only: %i[index edit update destroy] + resources :listing_categories, only: %i[index edit update new create + destroy], path: "listings/categories" + end +end + +namespace :api, defaults: { format: "json" } do + scope module: :v0, constraints: ApiConstraints.new(version: 0, default: true) do + resources :listings, only: %i[index show create update] + get "/listings/category/:category", to: "listings#index", as: :listings_category + get "/organizations/:organization_username/listings", to: "organizations#listings", + as: :organization_listings + end +end +resources :listings, only: %i[index new create edit update destroy dashboard] + +get "/search/listings", to: "search#listings" +get "/listings/dashboard", to: "listings#dashboard" +get "/listings/:category", to: "listings#index", as: :listing_category +get "/listings/:category/:slug", to: "listings#index", as: :listing_slug +get "/listings/:category/:slug/:view", to: "listings#index", constraints: { view: /moderate/ } +get "/listings/:category/:slug/delete_confirm", to: "listings#delete_confirm" +delete "/listings/:category/:slug", to: "listings#destroy" +get "/social_previews/listing/:id", to: "social_previews#listing", as: :listing_social_preview +get "/about-listings", to: "pages#about_listings" diff --git a/spec/requests/api/v0/organizations_spec.rb b/spec/requests/api/v0/organizations_spec.rb index 63dbd3be1..de8a2504c 100644 --- a/spec/requests/api/v0/organizations_spec.rb +++ b/spec/requests/api/v0/organizations_spec.rb @@ -82,6 +82,12 @@ RSpec.describe "Api::V0::Organizations", type: :request do expect(response).to have_http_status(:not_found) end + it "returns success for when orgnaization username exists" do + create(:listing, user: org_user, organization: organization) + get "/api/organizations/#{organization.username}/listings" + expect(response).to have_http_status(:success) + end + it "supports pagination" do create(:listing, user: org_user, organization: organization)