diff --git a/.rubocop.yml b/.rubocop.yml index fd2cf0919..e5fde5a60 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -683,6 +683,11 @@ RSpec/ExampleLength: Max: 15 StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExampleLength +RSpec/MultipleMemoizedHelpers: + Description: Checks if example groups contain too many `let` and `subject` calls. + Enabled: false + StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers + RSpec/MultipleExpectations: Description: Checks if examples contain too many `expect` calls. Enabled: true diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 15fadbf50..a796b5c91 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -6,19 +6,19 @@ require: # This configuration was generated by # `rubocop --auto-gen-config` -# on 2020-07-29 11:14:03 UTC using RuboCop version 0.88.0. +# on 2020-08-24 06:18:00 UTC using RuboCop version 0.89.1. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 6 +# Offense count: 3 Performance/OpenStruct: Exclude: - 'spec/models/github_repo_spec.rb' - 'spec/requests/github_repos_spec.rb' -# Offense count: 28 +# Offense count: 13 # Configuration parameters: Include. # Include: app/models/**/*.rb Rails/HasManyOrHasOneDependent: diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index f5cb81706..7672ed070 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -293,7 +293,7 @@ Devise.setup do |config| # Add a new OmniAuth provider. Check the wiki for more information on setting # up on your models and hooks. - # Fun fact, if this is reordered to have Twitter first, it doesn't work for some reason. + # Fun fact, if this is reordered to have Twitter first, it doesn't work for some reason. config.omniauth :github, setup: GITHUB_OMNIUATH_SETUP config.omniauth :twitter, setup: TWITTER_OMNIAUTH_SETUP diff --git a/lib/tasks/temporary/env_variables.rake b/lib/tasks/temporary/env_variables.rake index 003622788..45fe6aa93 100644 --- a/lib/tasks/temporary/env_variables.rake +++ b/lib/tasks/temporary/env_variables.rake @@ -6,8 +6,7 @@ task create_dot_env_file: :environment do sample_app_yml = YAML.load_file("config/sample_application.yml") # read existing lines - application_yml = File.open("config/application.yml", 'r').read - + application_yml = File.open("config/application.yml", "r").read File.open(".env", "w") do |env_file| # add default values only if they are not already present in the @@ -16,7 +15,7 @@ task create_dot_env_file: :environment do env_file.write("export #{variable}=#{value}\n") unless application_yml.include?(variable.to_s) end - File.open("config/application.yml", 'r') do |file| + File.open("config/application.yml", "r") do |file| file.each_line do |line| new_line = line.gsub(": ", "=") unless new_line.blank? || new_line.starts_with?("#") diff --git a/package.json b/package.json index efa5e8bdc..f752ced1a 100644 --- a/package.json +++ b/package.json @@ -53,6 +53,9 @@ "Gemfile": [ "bundle exec rubocop --require rubocop-rspec --auto-correct" ], + "*.rake": [ + "bundle exec rubocop --require rubocop-rspec --auto-correct" + ], "app/**/*.html.erb": [ "bundle exec erblint --autocorrect" ], diff --git a/spec/requests/listings_spec.rb b/spec/requests/listings_spec.rb index 3a8542788..e7c226df2 100644 --- a/spec/requests/listings_spec.rb +++ b/spec/requests/listings_spec.rb @@ -1,17 +1,16 @@ require "rails_helper" RSpec.describe "/listings", type: :request do - let(:edu_category) do - create(:listing_category, cost: 1) - end let(:user) { create(:user) } + let(:organization) { create(:organization) } + let(:edu_category) { create(:listing_category, cost: 1) } let(:listing_params) do { listing: { title: "something", body_markdown: "something else", listing_category_id: edu_category.id, - tag_list: "", + tag_list: "ruby, rails, go", contact_via_connect: true } } @@ -144,7 +143,7 @@ RSpec.describe "/listings", type: :request do describe "POST /listings" do before do sign_in user - create_list(:credit, 25, user: user) + create_list(:credit, 20, user: user) end let(:cfp_category) { create(:listing_category, :cfp) } @@ -199,30 +198,56 @@ RSpec.describe "/listings", type: :request do expect(response).to redirect_to "/listings" end + it "creates proper listing if credits are available" do + post "/listings", params: listing_params + expect(Listing.last.processed_html).to include(listing_params[:listing][:body_markdown]) + end + it "properly deducts the amount of credits" do expect do post "/listings", params: listing_params end.to change { user.credits.spent.size }.by(edu_category.cost) end - it "creates a listing draft under the org" do + it "adds tags" do + post "/listings", params: listing_params + + tag = listing_params[:listing][:tag_list].split(", ").first + expect(Listing.last.cached_tag_list).to include(tag) + end + + it "creates the listing under the user" do + post "/listings", params: listing_params + expect(Listing.last.user_id).to eq(user.id) + end + + it "creates the listing for the user if no organization_id is selected" do + create(:organization_membership, user_id: user.id, organization_id: organization.id) + + post "/listings", params: listing_params + + expect(Listing.last.organization_id).to be(nil) + expect(Listing.last.user_id).to eq(user.id) + end + + it "creates a listing draft under the org for an org admin" do org_admin = create(:user, :org_admin) org_id = org_admin.organizations.first.id Credit.create(organization_id: org_id) draft_params[:listing][:organization_id] = org_id sign_in org_admin post "/listings", params: draft_params - expect(Listing.first.organization_id).to eq org_id + expect(Listing.first.organization_id).to eq(org_id) end - it "creates a listing under the org" do + it "creates a listing under the org for an org admin" do org_admin = create(:user, :org_admin) org_id = org_admin.organizations.first.id Credit.create(organization_id: org_id) listing_params[:listing][:organization_id] = org_id sign_in org_admin post "/listings", params: listing_params - expect(Listing.first.organization_id).to eq org_id + expect(Listing.first.organization_id).to eq(org_id) end it "does not create a listing draft for an org not belonging to the user" do @@ -237,6 +262,22 @@ RSpec.describe "/listings", type: :request do expect { post "/listings", params: listing_params }.to raise_error(Pundit::NotAuthorizedError) end + it "creates the listing for the organization" do + create(:organization_membership, user_id: user.id, organization_id: organization.id) + listing_params[:listing][:organization_id] = organization.id + post "/listings", params: listing_params + expect(Listing.last.organization_id).to eq(organization.id) + end + + it "does not create an org listing if a different org member doesn't belong to the org" do + another_org = create(:organization) + create(:organization_membership, user_id: user.id, organization_id: another_org.id) + listing_params[:listing][:organization_id] = organization.id + expect do + post "/listings", params: listing_params + end.to raise_error Pundit::NotAuthorizedError + end + it "assigns the spent credits to the listing" do post "/listings", params: listing_params spent_credit = user.credits.spent.last @@ -269,6 +310,29 @@ RSpec.describe "/listings", type: :request do end.to raise_error("SUSPENDED") end end + + context "when rate limiting" do + let(:rate_limiter) { RateLimitChecker.new(user) } + + before do + allow(RateLimitChecker).to receive(:new).and_return(rate_limiter) + sign_in user + end + + it "increments rate limit for listing_creation" do + allow(rate_limiter).to receive(:track_limit_by_action) + post "/listings", params: listing_params + + expect(rate_limiter).to have_received(:track_limit_by_action).with(:listing_creation) + end + + it "returns a 429 status when rate limit is reached" do + allow(rate_limiter).to receive(:limit_by_action).and_return(true) + post "/listings", params: listing_params + + expect(response.status).to eq(429) + end + end end describe "PUT /listings/:id" do @@ -316,7 +380,7 @@ RSpec.describe "/listings", type: :request do expect do put "/listings/#{listing.id}", params: params end.to change(user.credits.spent, :size).by(cost) - expect(listing.reload.bumped_at >= previous_bumped_at).to eq(true) + expect(listing.reload.bumped_at >= previous_bumped_at).to be(true) end it "bumps the org listing using org credits before user credits" do @@ -327,7 +391,7 @@ RSpec.describe "/listings", type: :request do expect do put "/listings/#{org_listing.id}", params: params end.to change(organization.credits.spent, :size).by(cost) - expect(org_listing.reload.bumped_at >= previous_bumped_at).to eq(true) + expect(org_listing.reload.bumped_at >= previous_bumped_at).to be(true) end it "bumps the org listing using user credits if org credits insufficient and user credits are" do @@ -337,7 +401,7 @@ RSpec.describe "/listings", type: :request do expect do put "/listings/#{org_listing.id}", params: params end.to change(user.credits.spent, :size).by(cost) - expect(org_listing.reload.bumped_at >= previous_bumped_at).to eq(true) + expect(org_listing.reload.bumped_at >= previous_bumped_at).to be(true) end end @@ -404,16 +468,64 @@ RSpec.describe "/listings", type: :request do it "unpublishes a published listing" do put "/listings/#{listing.id}", params: params - expect(listing.reload.published).to eq(false) + expect(listing.reload.published).to be(false) end end context "when an update is attempted" do + it "updates body_markdown" do + put "/listings/#{listing.id}", params: { listing: { body_markdown: "hello new markdown" } } + expect(listing.reload.body_markdown).to eq("hello new markdown") + end + + it "does not update body_markdown if not bumped/created recently" do + listing.update_column(:bumped_at, 50.hours.ago) + + put "/listings/#{listing.id}", params: { listing: { body_markdown: "hello new markdown" } } + expect(listing.reload.body_markdown).not_to eq("hello new markdown") + end + it "does not update with an empty body markdown" do put "/listings/#{listing.id}", params: { listing: { body_markdown: "" } } expect(response.body).to include(CGI.escapeHTML("can't be blank")) expect(listing.reload.body_markdown).not_to be_empty end + + it "updates other fields" do + put "/listings/#{listing.id}", params: { + listing: { body_markdown: "hello new markdown", title: "New title!", tag_list: "new, tags, hey" } + } + + listing.reload + expect(listing.title).to eq("New title!") + expect(listing.cached_tag_list).to include("hey") + end + + it "does not update other fields if not bumped/created recently" do + listing.update_column(:bumped_at, 50.hours.ago) + + put "/listings/#{listing.id}", params: { + listing: { body_markdown: "hello new markdown", title: "New title!", tag_list: "new, tags, hey" } + } + + listing.reload + expect(listing.title).not_to eq("New title!") + expect(listing.cached_tag_list).not_to include("hey") + end + end + end + + describe "GET /listings/edit" do + let(:listing) { create(:listing, user: user) } + + before do + create_list(:credit, 20, user: user) + sign_in user + end + + it "has page content" do + get "/listings/#{listing.id}/edit" + expect(response.body).to include("You can bump your listing") end end @@ -517,166 +629,3 @@ RSpec.describe "/listings", type: :request do end end end - -# TODO: [@forem/oss] We used to have 2 request spec files, listing_spec.rb -# and classified_listing_spec.rb. This context contains the specs of the former, -# but we should eventually unify them into one set to remove some redundancy. -context "when running the specs that were previously in another file" do - let(:user) { create(:user) } - let(:organization) { create(:organization) } - let(:cl_category) { create(:listing_category, cost: 1) } - - let(:params) do - { - listing: { - title: "Hey", - listing_category_id: cl_category.id, - body_markdown: "hey hey my my", - tag_list: "ruby, rails, go" - } - } - end - - describe "POST /listings" do - before do - create_list(:credit, 20, user_id: user.id) - sign_in user - end - - it "creates proper listing if credits are available" do - post "/listings", params: params - expect(Listing.last.processed_html).to include("hey my") - end - - it "spends credits" do - num_credits = Credit.where(spent: true).size - post "/listings", params: params - expect(Credit.where(spent: true).size).to be > num_credits - end - - it "adds tags" do - post "/listings", params: params - expect(Listing.last.cached_tag_list).to include("rails") - end - - it "creates the listing under the user" do - post "/listings", params: params - expect(Listing.last.user_id).to eq user.id - end - - it "creates the listing for the user if no organization_id is selected" do - create(:organization_membership, user_id: user.id, organization_id: organization.id) - post "/listings", params: params - expect(Listing.last.organization_id).to eq nil - expect(Listing.last.user_id).to eq user.id - end - - it "creates the listing for the organization" do - create(:organization_membership, user_id: user.id, organization_id: organization.id) - params[:listing][:organization_id] = organization.id - post "/listings", params: params - expect(Listing.last.organization_id).to eq(organization.id) - end - - it "does not create an org listing if a different org member doesn't belong to the org" do - another_org = create(:organization) - create(:organization_membership, user_id: user.id, organization_id: another_org.id) - params[:listing][:organization_id] = organization.id - expect do - post "/listings", params: params - end.to raise_error Pundit::NotAuthorizedError - end - - context "when rate limiting" do - let(:rate_limiter) { RateLimitChecker.new(user) } - - before do - allow(RateLimitChecker).to receive(:new).and_return(rate_limiter) - sign_in user - end - - it "increments rate limit for listing_creation" do - allow(rate_limiter).to receive(:track_limit_by_action) - post "/listings", params: params - - expect(rate_limiter).to have_received(:track_limit_by_action).with(:listing_creation) - end - - it "returns a 429 status when rate limit is reached" do - allow(rate_limiter).to receive(:limit_by_action).and_return(true) - post "/listings", params: params - - expect(response.status).to eq(429) - end - end - end - - describe "GET /listings/edit" do - let(:listing) { create(:listing, user_id: user.id) } - - before do - create_list(:credit, 20, user_id: user.id) - sign_in user - end - - it "has page content" do - get "/listings/#{listing.id}/edit" - expect(response.body).to include("You can bump your listing") - end - end - - describe "PUT /listings/:id" do - let(:listing) { create(:listing, user_id: user.id) } - - before do - create_list(:credit, 20, user_id: user.id) - sign_in user - end - - it "updates bumped_at if action is bump" do - put "/listings/#{listing.id}", params: { - listing: { action: "bump" } - } - expect(Listing.last.bumped_at).to be > 10.seconds.ago - end - - it "updates publish if action is unpublish" do - put "/listings/#{listing.id}", params: { - listing: { action: "unpublish" } - } - expect(Listing.last.published).to eq(false) - end - - it "updates body_markdown" do - put "/listings/#{listing.id}", params: { - listing: { body_markdown: "hello new markdown" } - } - expect(Listing.last.body_markdown).to eq("hello new markdown") - end - - it "does not update body_markdown if not bumped/created recently" do - listing.update_column(:bumped_at, 50.hours.ago) - put "/listings/#{listing.id}", params: { - listing: { body_markdown: "hello new markdown" } - } - expect(Listing.last.body_markdown).not_to eq("hello new markdown") - end - - it "updates other fields" do - put "/listings/#{listing.id}", params: { - listing: { body_markdown: "hello new markdown", title: "New title!", tag_list: "new, tags, hey" } - } - expect(Listing.last.title).to eq("New title!") - expect(Listing.last.cached_tag_list).to include("hey") - end - - it "does not update other fields" do - listing.update_column(:bumped_at, 50.hours.ago) - put "/listings/#{listing.id}", params: { - listing: { body_markdown: "hello new markdown", title: "New title!", tag_list: "new, tags, hey" } - } - expect(Listing.last.title).not_to eq("New title!") - expect(Listing.last.cached_tag_list).not_to include("hey") - end - end -end