From 1ac767902bddca120c3511265423db139ef75344 Mon Sep 17 00:00:00 2001 From: Andreas Nedbal Date: Sun, 14 Jan 2024 21:13:30 +0100 Subject: [PATCH] Add more generic implementation of inbox filtering --- app/controllers/inbox_controller.rb | 34 +++--------- app/models/inbox_filter.rb | 45 +++++++++++++++ spec/controllers/inbox_controller_spec.rb | 67 +++++++++++------------ 3 files changed, 83 insertions(+), 63 deletions(-) create mode 100644 app/models/inbox_filter.rb diff --git a/app/controllers/inbox_controller.rb b/app/controllers/inbox_controller.rb index 56b01f5a..e551b0bc 100644 --- a/app/controllers/inbox_controller.rb +++ b/app/controllers/inbox_controller.rb @@ -4,16 +4,8 @@ class InboxController < ApplicationController before_action :authenticate_user! def show - find_author find_inbox_entries - if @author_user && @inbox_count.zero? - # rubocop disabled because of a false positive - flash[:info] = t(".author.info", author: @author) # rubocop:disable Rails/ActionControllerFlashBeforeRender - redirect_to inbox_path(last_id: params[:last_id]) - return - end - @delete_id = find_delete_id @disabled = true if @inbox.empty? @@ -47,36 +39,24 @@ class InboxController < ApplicationController private - def find_author - return if params[:author].blank? - - @author = params[:author] - - @author_user = User.where("LOWER(screen_name) = ?", @author.downcase).first - flash.now[:error] = t(".author.error", author: @author) unless @author_user + def filter_params + params.slice(*InboxFilter::KEYS).permit(*InboxFilter::KEYS) end def find_inbox_entries - @inbox = current_user.cursored_inbox(last_id: params[:last_id]).then(&method(:filter_author_chain)) + filter = InboxFilter.new(current_user, filter_params) + @inbox = filter.cursored_results(last_id: params[:last_id]) @inbox_last_id = @inbox.map(&:id).min - @more_data_available = current_user.cursored_inbox(last_id: @inbox_last_id, size: 1).then(&method(:filter_author_chain)).count.positive? - @inbox_count = current_user.inboxes.then(&method(:filter_author_chain)).count + @more_data_available = filter.cursored_results(last_id: @inbox_last_id, size: 1).count.positive? + @inbox_count = filter.results.count end def find_delete_id - return "ib-delete-all-author" if @author_user && @inbox_count.positive? + return "ib-delete-all-author" if params[:author].present? && @inbox_count.positive? "ib-delete-all" end - def filter_author_chain(query) - return query unless @author_user - - query - .joins(:question) - .where(questions: { user: @author_user, author_is_anonymous: false }) - end - # rubocop:disable Rails/SkipsModelValidations def mark_inbox_entries_as_read # using .dup to not modify @inbox -- useful in tests diff --git a/app/models/inbox_filter.rb b/app/models/inbox_filter.rb new file mode 100644 index 00000000..2ae01974 --- /dev/null +++ b/app/models/inbox_filter.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +class InboxFilter + include CursorPaginatable + + define_cursor_paginator :cursored_results, :results + + KEYS = %i( + author + anonymous + ).freeze + + attr_reader :params, :user + + def initialize(user, params) + @user = user + @params = params + end + + def results + scope = @user.inboxes + .includes(:question, user: :profile) + .order(:created_at) + .reverse_order + + params.each do |key, value| + scope.merge!(scope_for(key, value)) if value.present? + end + + scope + end + + private + + def scope_for(key, value) + case key.to_s + when "author" + @user.inboxes.joins(question: [:user]) + .where(questions: { users: { screen_name: value }, author_is_anonymous: false }) + when "anonymous" + @user.inboxes.joins(:question) + .where(questions: { author_is_anonymous: true }) + end + end +end diff --git a/spec/controllers/inbox_controller_spec.rb b/spec/controllers/inbox_controller_spec.rb index a6e5917c..73553f33 100644 --- a/spec/controllers/inbox_controller_spec.rb +++ b/spec/controllers/inbox_controller_spec.rb @@ -144,39 +144,10 @@ describe InboxController, type: :controller do subject { get :show, params: { author: author_param } } - context "with a nonexisting screen name" do - let(:author_param) { "xXx420MegaGamer2003xXx" } - - it "sets the error flash" do - subject - expect(flash[:error]).to eq "No user with the name @xXx420MegaGamer2003xXx found, showing entries from all users instead!" - end - - include_examples "sets the expected ivars" do - let(:expected_assigns) do - { - inbox: [generic_inbox_entry2, generic_inbox_entry1], - inbox_last_id: generic_inbox_entry1.id, - more_data_available: false, - inbox_count: 2, - delete_id: "ib-delete-all", - disabled: nil - } - end - end - end - context "with an existing screen name" do let(:author_param) { other_user.screen_name } context "with no questions from the other user in the inbox" do - it { is_expected.to redirect_to inbox_path } - - it "sets the info flash" do - subject - expect(flash[:info]).to eq "No questions from @#{other_user.screen_name} found, showing entries from all users instead!" - end - include_examples "sets the expected ivars" do # these are the ivars set before the redirect happened let(:expected_assigns) do @@ -202,13 +173,6 @@ describe InboxController, type: :controller do ) end - it { is_expected.to redirect_to inbox_path } - - it "sets the info flash" do - subject - expect(flash[:info]).to eq "No questions from @#{other_user.screen_name} found, showing entries from all users instead!" - end - include_examples "sets the expected ivars" do # these are the ivars set before the redirect happened let(:expected_assigns) do @@ -259,6 +223,37 @@ describe InboxController, type: :controller do end end end + + context "when passed the anonymous param" do + let!(:other_user) { FactoryBot.create(:user) } + let!(:generic_inbox_entry) do + Inbox.create( + user:, + question: FactoryBot.create( + :question, + user: other_user, + author_is_anonymous: false + ) + ) + end + + let!(:inbox_entry_fillers) do + # 9 times => 1 entry less than default page size + 9.times.map { Inbox.create(user:, question: FactoryBot.create(:question, author_is_anonymous: true)) } + end + + subject { get :show, params: { anonymous: true } } + + include_examples "sets the expected ivars" do + let(:expected_assigns) do + { + inbox: [*inbox_entry_fillers.reverse], + more_data_available: false, + inbox_count: 9 + } + end + end + end end end