diff --git a/app/assets/javascripts/initializers/initNotifications.js b/app/assets/javascripts/initializers/initNotifications.js index fec5475da..3b7170802 100644 --- a/app/assets/javascripts/initializers/initNotifications.js +++ b/app/assets/javascripts/initializers/initNotifications.js @@ -1,4 +1,4 @@ -/* global checkUserLoggedIn, instantClick, InstantClick, sendHapticMessage */ +/* global checkUserLoggedIn, instantClick, InstantClick, sendHapticMessage, showModalAfterError */ function initNotifications() { fetchNotificationsCount(); @@ -94,6 +94,13 @@ function initReactions() { .then(function (response) { if (response.status === 200) { response.json().then(successCb); + } else { + showModalAfterError({ + response, + element: 'reaction', + action_ing: 'updating', + action_past: 'updated', + }); } }); }; diff --git a/app/assets/javascripts/initializers/initializeArticleReactions.js b/app/assets/javascripts/initializers/initializeArticleReactions.js index 144b8482a..78209a822 100644 --- a/app/assets/javascripts/initializers/initializeArticleReactions.js +++ b/app/assets/javascripts/initializers/initializeArticleReactions.js @@ -1,4 +1,4 @@ -/* global sendHapticMessage, showLoginModal */ +/* global sendHapticMessage, showLoginModal, showModalAfterError */ // Set reaction count to correct number function setReactionCount(reactionName, newCount) { @@ -88,10 +88,17 @@ function reactToArticle(articleId, reaction) { return response.json().then(() => { document.getElementById('reaction-butt-' + reaction).disabled = false; }); + } else { + toggleReaction(); + document.getElementById('reaction-butt-' + reaction).disabled = false; + showModalAfterError({ + response, + element: 'reaction', + action_ing: 'updating', + action_past: 'updated', + }); + return undefined; } - toggleReaction(); - document.getElementById('reaction-butt-' + reaction).disabled = false; - return undefined; }) .catch((error) => { toggleReaction(); diff --git a/app/assets/javascripts/initializers/initializeCommentsPage.js.erb b/app/assets/javascripts/initializers/initializeCommentsPage.js.erb index ebc99a8c2..1024dec15 100644 --- a/app/assets/javascripts/initializers/initializeCommentsPage.js.erb +++ b/app/assets/javascripts/initializers/initializeCommentsPage.js.erb @@ -148,6 +148,13 @@ function initializeCommentsPage() { thisButt.disabled = false; if (response.status === 200) { response.json().then(successCb); + } else { + showModalAfterError({ + response, + element: 'reaction', + action_ing: 'making', + action_past: 'made', + }); } }); }; @@ -297,15 +304,11 @@ function handleCommentSubmit(event) { }) } else { form.classList.remove('submitting'); - var responseStatus = response.status; - response.json().then(function parseError(errorReponse) { - if (responseStatus === 429) { - showRateLimitModal('made a comment', 'making another comment') - } else { - showUserAlertModal('Error posting comment', 'Your comment could not be posted due to an error: ' + errorReponse.error, 'OK') - } - }).catch(function parseError(error){ - showUserAlertModal('Error posting comment', 'Your comment could not be posted due to a server error', 'OK') + showModalAfterError({ + response, + element: 'comment', + action_ing: 'posting', + action_past: 'posted', }); return false; } @@ -528,6 +531,13 @@ function handleImageUpload(event, randomIdNumber) { messageContainer.style.top = "5px"; } ); + } else if (response.status === 429) { + showRateLimitModal({ + response, + element: 'image', + action_ing: 'uploading', + action_past: 'uploaded', + }); } else { response.json().then(function(responseBody) { var errorMessage = responseBody.error || 'Invalid file!'; diff --git a/app/assets/javascripts/utilities/showUserAlertModal.js b/app/assets/javascripts/utilities/showUserAlertModal.js index 00ba187cd..fe811a54c 100644 --- a/app/assets/javascripts/utilities/showUserAlertModal.js +++ b/app/assets/javascripts/utilities/showUserAlertModal.js @@ -30,24 +30,78 @@ function showUserAlertModal(title, text, confirm_text) { * Displays a user rate limit alert modal letting the user know what they did that exceeded a rate limit, * and gives them links to explain why they must wait * - * @function showUserAlertModal - * @param {string} action_text Description of the action taken by the user - * @param {string} next_action_text Description of the next action that can be taken + * @function showRateLimitModal + * @param {string} element Description of the element that throw the error + * @param {string} action_ing The -ing form of the action taken by the user + * @param {string} action_past The past tense of the action taken by the user + * @param {string} timeframe Description of the time that we need to wait * * @example * showRateLimitModal('Made a comment', 'comment again') */ -function showRateLimitModal(action_text, next_action_text) { - let rateLimitText = buildRateLimitText(action_text, next_action_text); +function showRateLimitModal({ + element, + action_ing, + action_past, + timeframe = 'a moment', +}) { + let rateLimitText = buildRateLimitText({ + element, + action_ing, + action_past, + timeframe, + }); let rateLimitLink = '/faq'; showUserAlertModal( - 'Wait a moment...', + `Wait ${timeframe}...`, rateLimitText, 'Got it', rateLimitLink, 'Why do I have to wait?', ); } +/** + * Displays the corresponding modal after an error. + * + * @function showModalAfterError + * @param {Object} response The response from the API + * @param {string} element Description of the element that throw the error + * @param {string} action_ing The -ing form of the action taken by the user + * @param {string} action_past The past tense of the action taken by the user + * @param {string} timeframe Description of the time that we need to wait + * + * @example + * showModalAfterError(response, 'made a comment', 'making another comment', 'a moment'); + */ +function showModalAfterError({ + response, + element, + action_ing, + action_past, + timeframe = 'a moment', +}) { + response + .json() + .then(function parseError(errorReponse) { + if (response.status === 429) { + showRateLimitModal({ element, action_ing, action_past, timeframe }); + } else { + showUserAlertModal( + `Error ${action_ing} ${element}`, + `Your ${element} could not be ${action_past} due to an error: ` + + errorReponse.error, + 'OK', + ); + } + }) + .catch(function parseError(error) { + showUserAlertModal( + `Error ${action_ing} ${element}`, + `Your ${element} could not be ${action_past} due to a server error`, + 'OK', + ); + }); +} /** * HTML template for modal @@ -61,17 +115,17 @@ function showRateLimitModal(action_text, next_action_text) { * @returns {string} HTML for the modal */ const getModalHtml = (text, confirm_text) => ` - -`; + + `; /** * Constructs wording for rate limit modals @@ -79,13 +133,15 @@ const getModalHtml = (text, confirm_text) => ` * @private * @function buildRateLimitText * - * @param {string} action_text Description of the action taken by the user - * @param {string} next_action_text Description of the next action that can be taken + * @param {string} element Description of the element that throw the error + * @param {string} action_ing The -ing form of the action taken by the user + * @param {string} action_past The past tense of the action taken by the user + * @param {string} timeframe Description of the time that we need to wait * * @returns {string} Formatted body text for a rate limit modal */ -function buildRateLimitText(action_text, next_action_text) { - return `Since you recently ${action_text}, you’ll need to wait a moment before ${next_action_text}.`; +function buildRateLimitText({ element, action_ing, action_past, timeframe }) { + return `Since you recently ${action_past} a ${element}, you’ll need to wait ${timeframe} before ${action_ing} another ${element}.`; } /** @@ -104,6 +160,8 @@ function buildModalDiv(text, confirm_text) { if (!modalDiv) { modalDiv = getModal(text, confirm_text); document.body.appendChild(modalDiv); + } else { + modalDiv.outerHTML = getModal(text, confirm_text).outerHTML; } return modalDiv; } diff --git a/app/controllers/concerns/listings_toolkit.rb b/app/controllers/concerns/listings_toolkit.rb index 5255cfa8f..6a93538e1 100644 --- a/app/controllers/concerns/listings_toolkit.rb +++ b/app/controllers/concerns/listings_toolkit.rb @@ -36,6 +36,16 @@ module ListingsToolkit @listing = Listing.find(params[:id]) end + def rate_limit? + begin + rate_limit!(:listing_creation) + rescue StandardError => e + @listing.errors.add(:listing_creation, e.message) + return true + end + false + end + def create @listing = Listing.new(listing_params) organization_id = @listing.organization_id @@ -51,7 +61,7 @@ module ListingsToolkit return end - unless @listing.valid? + if !@listing.valid? || rate_limit? @credits = current_user.credits.unspent process_unsuccessful_creation return diff --git a/app/controllers/feedback_messages_controller.rb b/app/controllers/feedback_messages_controller.rb index a83d744fb..ea683e5fd 100644 --- a/app/controllers/feedback_messages_controller.rb +++ b/app/controllers/feedback_messages_controller.rb @@ -6,13 +6,12 @@ class FeedbackMessagesController < ApplicationController def create flash.clear - rate_limit!(:feedback_message_creation) params = feedback_message_params.merge(reporter_id: current_user&.id) @feedback_message = FeedbackMessage.new(params) recaptcha_enabled = ReCaptcha::CheckEnabled.call(current_user) - if (!recaptcha_enabled || recaptcha_verified? || connect_feedback?) && @feedback_message.save + if (!recaptcha_enabled || recaptcha_verified? || connect_feedback?) && !rate_limit? && @feedback_message.save Slack::Messengers::Feedback.call( user: current_user, type: feedback_message_params[:feedback_type], @@ -65,4 +64,14 @@ class FeedbackMessagesController < ApplicationController allowed_params = %i[message feedback_type category reported_url offender_id] params.require(:feedback_message).permit(allowed_params) end + + def rate_limit? + begin + rate_limit!(:feedback_message_creation) + rescue StandardError => e + @feedback_message.errors.add(:feedback_message_creation, e.message) + return true + end + false + end end diff --git a/app/controllers/listings_controller.rb b/app/controllers/listings_controller.rb index 53b509f54..73f99e363 100644 --- a/app/controllers/listings_controller.rb +++ b/app/controllers/listings_controller.rb @@ -27,7 +27,6 @@ class ListingsController < ApplicationController # actions `create` and `update` are defined in the module `ListingsToolkit`, # we thus silence Rubocop lexical scope filter cop: https://rails.rubystyle.guide/#lexically-scoped-action-filter # rubocop:disable Rails/LexicallyScopedActionFilter - before_action :check_limit, only: [:create] before_action :set_listing, only: %i[edit update destroy] before_action :set_cache_control_headers, only: %i[index] before_action :raise_suspended, only: %i[new create update] @@ -142,10 +141,6 @@ class ListingsController < ApplicationController redirect_to "/listings/dashboard" end - def check_limit - rate_limit!(:listing_creation) - end - # This is a convenience method to query listings for use in the index view in # the index action and process_after_update method def listings_for_index_view diff --git a/app/javascript/packs/followButtons.js b/app/javascript/packs/followButtons.js index 7ce104554..cb5670b72 100644 --- a/app/javascript/packs/followButtons.js +++ b/app/javascript/packs/followButtons.js @@ -1,6 +1,6 @@ import { getInstantClick } from '../topNavigation/utilities'; -/* global showLoginModal userData */ +/* global showLoginModal userData showModalAfterError*/ /** * Sets the text content of the button to the correct 'Follow' state @@ -223,7 +223,19 @@ function handleFollowButtonClick({ target }) { formData.append('followable_type', className); formData.append('followable_id', id); formData.append('verb', verb); - getCsrfToken().then(sendFetch('follow-creation', formData)); + getCsrfToken() + .then(sendFetch('follow-creation', formData)) + .then((response) => { + if (response.status !== 200) { + showModalAfterError({ + response, + element: 'user', + action_ing: 'following', + action_past: 'followed', + timeframe: 'for a day', + }); + } + }); } } diff --git a/cypress/integration/seededFlows/articleFlows/commentOnArticle.spec.js b/cypress/integration/seededFlows/articleFlows/commentOnArticle.spec.js index c16cf0c91..201560fc5 100644 --- a/cypress/integration/seededFlows/articleFlows/commentOnArticle.spec.js +++ b/cypress/integration/seededFlows/articleFlows/commentOnArticle.spec.js @@ -479,7 +479,7 @@ describe('Comment on articles', () => { cy.findByRole('button', { name: /Close/ }).should('have.focus'); cy.findByRole('heading', { name: 'Wait a moment...' }).should('exist'); cy.findByText( - 'Since you recently made a comment, you’ll need to wait a moment before making another comment.', + 'Since you recently posted a comment, you’ll need to wait a moment before posting another comment.', ); cy.findByRole('button', { name: 'Got it' }).click(); }); diff --git a/spec/requests/feedback_messages_spec.rb b/spec/requests/feedback_messages_spec.rb index d3418b1ab..6cc2b68c1 100644 --- a/spec/requests/feedback_messages_spec.rb +++ b/spec/requests/feedback_messages_spec.rb @@ -108,8 +108,8 @@ RSpec.describe "feedback_messages", type: :request do allow(RateLimitChecker).to receive(:new) { limiter } allow(limiter).to receive(:limit_by_action).and_return(true) - post "/feedback_messages", params: valid_abuse_report_params, headers: headers - expect(response.status).to eq(429) + post "/feedback_messages.json", params: valid_abuse_report_params, headers: headers + expect(response.parsed_body["status"]).to eq("bad_request") end end diff --git a/spec/requests/listings_spec.rb b/spec/requests/listings_spec.rb index f7e0f270e..f8d91d7c9 100644 --- a/spec/requests/listings_spec.rb +++ b/spec/requests/listings_spec.rb @@ -359,8 +359,7 @@ RSpec.describe "/listings", type: :request do 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) + expect(response.body).to include("Rate limit reached") end end end diff --git a/spec/system/articles/user_creates_an_article_spec.rb b/spec/system/articles/user_creates_an_article_spec.rb index 0fae51c7a..3a57c8f16 100644 --- a/spec/system/articles/user_creates_an_article_spec.rb +++ b/spec/system/articles/user_creates_an_article_spec.rb @@ -77,4 +77,24 @@ RSpec.describe "Creating an article with the editor", type: :system do expect_runkit_tag_to_be_active end end + + context "when user creates too many articles" do + let(:rate_limit_checker) { RateLimitChecker.new(user) } + + before do + # avoid hitting new user rate limit check + allow(user).to receive(:created_at).and_return(1.week.ago) + allow(RateLimitChecker).to receive(:new).and_return(rate_limit_checker) + allow(rate_limit_checker).to receive(:limit_by_action) + .with(:published_article_creation) + .and_return(true) + end + + it "displays a rate limit warning", :flaky, js: true do + visit new_path + fill_in "article_body_markdown", with: template + click_button "Save changes" + expect(page).to have_text("Rate limit reached") + end + end end diff --git a/spec/system/articles/user_edits_an_article_spec.rb b/spec/system/articles/user_edits_an_article_spec.rb index f466bf8ff..53072fc96 100644 --- a/spec/system/articles/user_edits_an_article_spec.rb +++ b/spec/system/articles/user_edits_an_article_spec.rb @@ -36,4 +36,24 @@ RSpec.describe "Editing with an editor", type: :system, js: true do click_button("Save changes") expect(page).to have_text("Unpublished Post.") end + + context "when user edits too many articles" do + let(:rate_limit_checker) { RateLimitChecker.new(user) } + + before do + # avoid hitting new user rate limit check + allow(user).to receive(:created_at).and_return(1.week.ago) + allow(RateLimitChecker).to receive(:new).and_return(rate_limit_checker) + allow(rate_limit_checker).to receive(:limit_by_action) + .with(:article_update) + .and_return(true) + end + + it "displays a rate limit warning", :flaky, js: true do + visit "/#{user.username}/#{article.slug}/edit" + fill_in "article_body_markdown", with: template.gsub("Suspendisse", "Yooo") + click_button "Save changes" + expect(page).to have_text("Rate limit reached") + end + end end diff --git a/spec/system/feedback_message_spec.rb b/spec/system/feedback_message_spec.rb index 7caf32450..062d288ba 100644 --- a/spec/system/feedback_message_spec.rb +++ b/spec/system/feedback_message_spec.rb @@ -2,6 +2,8 @@ require "rails_helper" RSpec.describe "Feedback report by chat channel messages", type: :system do let(:user) { create(:user) } + let(:message) { Faker::Lorem.paragraph } + let(:url) { Faker::Lorem.sentence } context "when user create a report abuse feedback message" do before do @@ -21,4 +23,27 @@ RSpec.describe "Feedback report by chat channel messages", type: :system do end.to change(FeedbackMessage, :count).by(1) end end + + context "when user creates too many report abuse feedback messages" do + let(:rate_limit_checker) { RateLimitChecker.new(user) } + + before do + # avoid hitting new user rate limit check + allow(user).to receive(:created_at).and_return(1.week.ago) + allow(RateLimitChecker).to receive(:new).and_return(rate_limit_checker) + allow(rate_limit_checker).to receive(:limit_by_action) + .with(:feedback_message_creation) + .and_return(true) + end + + it "displays a rate limit warning", :flaky, js: true do + visit report_abuse_path + choose("Other") + fill_in "feedback_message_message", with: message + fill_in "feedback_message_reported_url", with: url + click_button "Send Feedback" + expect(page).to have_current_path("/feedback_messages") + expect(page).to have_text("Rate limit reached") + end + end end