Merge pull request #16147 from hennevogel/refactoring/notification-sc…

…opes

Refactor Notification scopes

src/api/app/components/notification_action_bar_component.rb

@@ -1,18 +1,18 @@
 # frozen_string_literal: true
 
 class NotificationActionBarComponent < ApplicationComponent
-  attr_accessor :type, :update_path, :show_read_all_button
+  attr_accessor :state, :update_path, :show_read_all_button
 
-  def initialize(type:, update_path:, show_read_all_button: false)
+  def initialize(state:, update_path:, show_read_all_button: false)
     super
 
-    @type = type
+    @state = state
     @update_path = add_params(update_path)
     @show_read_all_button = show_read_all_button
   end
 
   def button_text(all: false)
-    text = type == 'read' ? 'Unread' : 'Read'
+    text = state == 'read' ? 'Unread' : 'Read'
     if all
       "Mark all as '#{text}'"
     else

src/api/app/components/notification_filter_component.html.haml

@@ -1,33 +1,33 @@
 .list-group.list-group-flush.my-2
   = render NotificationFilterLinkComponent.new(text: 'Unread', amount: @count['unread'],
-                                               filter_item: { type: 'unread' }, selected_filter: @selected_filter)
-  = render NotificationFilterLinkComponent.new(text: 'Read', filter_item: { type: 'read' },
+                                               filter_item: { state: 'unread' }, selected_filter: @selected_filter)
+  = render NotificationFilterLinkComponent.new(text: 'Read', filter_item: { state: 'read' },
                                                selected_filter: @selected_filter)
 .list-group.list-group-flush.mt-5.mb-2
   %h5.ms-3 Filter
   = render NotificationFilterLinkComponent.new(text: 'Comments', amount: @count['Comment'], icon: 'fas fa-comments',
-                                               filter_item: { type: 'comments' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'comments' }, selected_filter: @selected_filter)
   = render NotificationFilterLinkComponent.new(text: 'Requests', amount: @count['BsRequest'], icon: 'fas fa-code-pull-request',
-                                               filter_item: { type: 'requests' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'requests' }, selected_filter: @selected_filter)
   = render NotificationFilterLinkComponent.new(text: 'Incoming Requests', amount: @count['incoming_requests'], icon: 'fas fa-code-pull-request',
-                                               filter_item: { type: 'incoming_requests' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'incoming_requests' }, selected_filter: @selected_filter)
   = render NotificationFilterLinkComponent.new(text: 'Outgoing Requests', amount: @count['outgoing_requests'], icon: 'fas fa-code-pull-request',
-                                               filter_item: { type: 'outgoing_requests' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'outgoing_requests' }, selected_filter: @selected_filter)
   = render NotificationFilterLinkComponent.new(text: 'Roles Granted', amount: @count['relationships_created'], icon: 'fas fa-user-tag',
-                                               filter_item: { type: 'relationships_created' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'relationships_created' }, selected_filter: @selected_filter)
   = render NotificationFilterLinkComponent.new(text: 'Roles Revoked', amount: @count['relationships_deleted'], icon: 'fas fa-user-tag',
-                                               filter_item: { type: 'relationships_deleted' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'relationships_deleted' }, selected_filter: @selected_filter)
   = render NotificationFilterLinkComponent.new(text: 'Build Failures', amount: @count['build_failures'], icon: "fas fa-xmark text-danger",
-                                               filter_item: { type: 'build_failures' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'build_failures' }, selected_filter: @selected_filter)
   - if ReportPolicy.new(@user, Report).notify?
     = render NotificationFilterLinkComponent.new(text: 'Reports', amount: @count['reports'], icon: 'fas fa-flag',
-                                                filter_item: { type: 'reports' }, selected_filter: @selected_filter,
+                                                filter_item: { kind: 'reports' }, selected_filter: @selected_filter,
                                                 )
   = render NotificationFilterLinkComponent.new(text: 'Workflow Runs', amount: @count['workflow_runs'], icon: 'fas fa-book-open',
-                                               filter_item: { type: 'workflow_runs' }, selected_filter: @selected_filter)
+                                               filter_item: { kind: 'workflow_runs' }, selected_filter: @selected_filter)
   - if Flipper.enabled?(:content_moderation, @user)
     = render NotificationFilterLinkComponent.new(text: 'Appealed Decisions', amount: @count['appealed_decisions'], icon: 'fas fa-hand',
-                                                 filter_item: { type: 'appealed_decisions' }, selected_filter: @selected_filter)
+                                                 filter_item: { kind: 'appealed_decisions' }, selected_filter: @selected_filter)
 
 - unless @projects_for_filter.empty?
   .list-group.list-group-flush.mt-5.mb-2

src/api/app/components/notification_filter_component.rb

@@ -14,8 +14,8 @@ def initialize(selected_filter:, user:, projects_for_filter: ProjectsForFilterFi
   def notifications_count
     notifications = User.session.notifications.for_web
     counted_notifications = notifications.unread.group(:notifiable_type).count
-    counted_notifications['incoming_requests'] = notifications.unread.for_incoming_requests.count
-    counted_notifications['outgoing_requests'] = notifications.unread.for_outgoing_requests.count
+    counted_notifications['incoming_requests'] = notifications.unread.for_incoming_requests(User.session!).count
+    counted_notifications['outgoing_requests'] = notifications.unread.for_outgoing_requests(User.session!).count
     counted_notifications['relationships_created'] = notifications.unread.for_relationships_created.count
     counted_notifications['relationships_deleted'] = notifications.unread.for_relationships_deleted.count
     counted_notifications['build_failures'] = notifications.unread.for_failed_builds.count

src/api/app/components/notification_filter_link_component.rb

@@ -24,10 +24,10 @@ def notification_filter_matches?
       @filter_item[:project] == @selected_filter[:project]
     elsif @selected_filter[:group].present?
       @filter_item[:group] == @selected_filter[:group]
-    elsif @selected_filter[:type].present?
-      @filter_item[:type] == @selected_filter[:type]
+    elsif @selected_filter[:kind].present?
+      @filter_item[:kind] == @selected_filter[:kind]
     else
-      @filter_item[:type] == 'unread'
+      @filter_item[:state] == 'unread'
     end
   end
 

src/api/app/components/notification_mark_button_component.rb

@@ -19,7 +19,8 @@ def icon
   end
 
   def update_path
-    my_notifications_path(notification_ids: [@notification.id], type: @selected_filter[:type],
+    my_notifications_path(notification_ids: [@notification.id], kind: @selected_filter[:kind],
+                          state: @selected_filter[:state],
                           project: @selected_filter[:project], group: @selected_filter[:group],
                           page: @page, show_more: @show_more)
   end

src/api/app/controllers/concerns/webui/notifications_filter.rb

@@ -0,0 +1,33 @@
+module Webui::NotificationsFilter
+  extend ActiveSupport::Concern
+
+  # It's just a case...
+  # rubocop:disable Metrics/CyclomaticComplexity
+  def filter_notifications_by_type(notifications, filter_type)
+    case filter_type
+    when 'comments'
+      notifications.for_comments
+    when 'requests'
+      notifications.for_requests
+    when 'incoming_requests'
+      notifications.for_incoming_requests(User.session!)
+    when 'outgoing_requests'
+      notifications.for_outgoing_requests(User.session!)
+    when 'relationships_created'
+      notifications.for_relationships_created
+    when 'relationships_deleted'
+      notifications.for_relationships_deleted
+    when 'build_failures'
+      notifications.for_failed_builds
+    when 'reports'
+      notifications.for_reports
+    when 'workflow_runs'
+      notifications.for_workflow_runs
+    when 'appealed_decisions'
+      notifications.for_appealed_decisions
+    else # all
+      notifications
+    end
+  end
+  # rubocop:enable Metrics/CyclomaticComplexity
+end

src/api/app/controllers/person/notifications_controller.rb

@@ -1,53 +1,72 @@
 module Person
   class NotificationsController < ApplicationController
     include Person::Errors
+    include Webui::NotificationsFilter
 
-    MAX_PER_PAGE = 300
-    ALLOWED_FILTERS = %w[read comments requests unread incoming_requests outgoing_requests relationships_created relationships_deleted
-                         build_failures reports workflow_runs appealed_decisions].freeze
+    ALLOWED_FILTERS = %w[all comments requests incoming_requests outgoing_requests relationships_created relationships_deleted build_failures
+                         reports reviews workflow_runs appealed_decisions].freeze
+    ALLOWED_STATES = %w[unread read].freeze
 
-    before_action :check_filter_type, except: [:update]
+    before_action :set_filter_type, only: :index
+    before_action :set_filter_state, only: :index
+    before_action :set_notifications, only: :index
+    before_action :set_notification, only: :update
 
     # GET /my/notifications
     def index
-      @notifications = paginated_notifications
       @notifications_count = @notifications.count
+      @paged_notifications = @notifications.page(params[:page])
+
+      params[:page] = @paged_notifications.total_pages if @paged_notifications.out_of_range?
+      params[:show_maximum] ? show_maximum(@notifications) : @paged_notifications
     end
 
     def update
-      notification = authorize Notification.find(params[:id])
-
-      notification.toggle(:delivered).save!
+      authorize @notification
 
-      render_ok
+      if @notification.toggle(:delivered).save
+        render_ok
+      else
+        render_error(message: @notification.errors.full_messages.to_sentence, status: 400)
+      end
     end
 
     private
 
-    def show_maximum(notifications)
-      total = notifications.size
-      notifications.page(params[:page]).per([total, MAX_PER_PAGE].min)
+    def set_notifications
+      @notifications = User.session!.notifications
+      @notifications = @notifications.for_project_name(params[:project]) if params[:project]
+      @notifications = @notifications.for_group_title(params[:group]) if params[:group]
+      @notifications = filter_notifications_by_type(@notifications, @filter_type)
+      @notifications = filter_notifications_by_state(@notifications, @filter_state)
+    end
+
+    def set_notification
+      @notification = User.session!.notifications.find(params[:id])
     end
 
-    def fetch_notifications
-      notifications = policy_scope(Notification)
-      filtered_notifications = if params[:project]
-                                 notifications.unread.for_project_name(params[:project])
-                               else
-                                 notifications
-                               end
-      filtered_notifications.for_notifiable_type(@filter_type)
+    def filter_notifications_by_state(notifications, filter_state)
+      case filter_state
+      when 'read'
+        notifications.read
+      else
+        notifications.unread
+      end
     end
 
-    def paginated_notifications
-      notifications = fetch_notifications
-      params[:page] = notifications.page(params[:page]).total_pages if notifications.page(params[:page]).out_of_range?
-      params[:show_maximum] ? show_maximum(notifications) : notifications.page(params[:page])
+    def show_maximum(notifications)
+      total = notifications.size
+      notifications.page(params[:page]).per([total, Notification::MAX_PER_PAGE].min)
     end
 
-    def check_filter_type
-      @filter_type = params[:notifications_type]
+    def set_filter_type
+      @filter_type = params[:kind] || 'all'
       raise FilterNotSupportedError if @filter_type.present? && ALLOWED_FILTERS.exclude?(@filter_type)
     end
+
+    def set_filter_state
+      @filter_state = params[:state] || 'unread'
+      raise FilterNotSupportedError if @filter_state.present? && ALLOWED_STATES.exclude?(@filter_state)
+    end
   end
 end

src/api/app/controllers/webui/users/notifications_controller.rb

@@ -1,66 +1,76 @@
 class Webui::Users::NotificationsController < Webui::WebuiController
-  VALID_NOTIFICATION_TYPES = %w[read reviews comments requests unread incoming_requests outgoing_requests relationships_created relationships_deleted
-                                build_failures reports workflow_runs appealed_decisions].freeze
+  include Webui::NotificationsFilter
+
+  ALLOWED_FILTERS = %w[all comments requests incoming_requests outgoing_requests relationships_created relationships_deleted build_failures
+                       reports reviews workflow_runs appealed_decisions].freeze
+  ALLOWED_STATES = %w[unread read].freeze
 
   before_action :require_login
-  before_action :check_param_type, :check_param_project, only: :index
+  before_action :set_filter_type, :set_filter_state
+  before_action :set_notifications
+  before_action :set_notifications_to_be_updated, only: [:update]
+  before_action :set_show_read_all_button
+  before_action :set_selected_filter
+  before_action :paginate_notifications
 
   def index
-    @notifications = paginated_notifications
-    @show_read_all_button = show_read_all_button?
     @filtered_project = Project.find_by(name: params[:project])
-    @selected_filter = selected_filter
     @current_user = User.session
   end
 
   def update
-    notifications = if params[:update_all]
-                      fetch_notifications
-                    else
-                      fetch_notifications.where(id: params[:notification_ids])
-                    end
     # rubocop:disable Rails/SkipsModelValidations
-    read_count = notifications.where(delivered: false).update_all('delivered = !delivered')
-    unread_count = notifications.where(delivered: true).update_all('delivered = !delivered')
+    @read_count = Notification.where(id: @undelivered_notification_ids).update_all('delivered = !delivered')
+    @unread_count = Notification.where(id: @delivered_notification_ids).update_all('delivered = !delivered')
     # rubocop:enable Rails/SkipsModelValidations
 
-    if read_count.zero? && unread_count.zero?
-      flash.now[:error] = "Couldn't update the notifications"
-    else
-      send_notifications_information_rabbitmq(read_count, unread_count)
-    end
-
     respond_to do |format|
       format.html { redirect_to my_notifications_path }
       format.js do
         render partial: 'update', locals: {
-          notifications: paginated_notifications,
-          selected_filter: selected_filter,
-          show_read_all_button: show_read_all_button?,
+          notifications: @notifications,
+          selected_filter: @selected_filter,
+          show_read_all_button: @show_read_all_button,
           user: User.session
         }
       end
+      send_notifications_information_rabbitmq(@read_count, @unread_count)
     end
   end
 
   private
 
-  def selected_filter
-    { type: params[:type], project: params[:project], group: params[:group] }
+  def set_filter_type
+    @filter_type = params[:kind] || 'all'
+    raise FilterNotSupportedError if @filter_type.present? && ALLOWED_FILTERS.exclude?(@filter_type)
+  end
+
+  def set_filter_state
+    @filter_state = params[:state] || 'unread'
+    raise FilterNotSupportedError if @filter_state.present? && ALLOWED_STATES.exclude?(@filter_state)
+  end
+
+  def set_notifications
+    @notifications = User.session!.notifications.for_web.includes(notifiable: [{ commentable: [{ comments: :user }, :project, :bs_request_actions] }, :bs_request_actions, :reviews])
+    @notifications = @notifications.for_project_name(params[:project]) if params[:project].present?
+    @notifications = @notifications.for_group_title(params[:group]) if params[:group].present?
+    @notifications = filter_notifications_by_type(@notifications, @filter_type)
+    @notifications = filter_notifications_by_state(@notifications, @filter_state)
   end
 
-  def check_param_type
-    return if params[:type].nil? || VALID_NOTIFICATION_TYPES.include?(params[:type])
+  def set_notifications_to_be_updated
+    return unless params[:notification_ids]
 
-    flash[:error] = 'Filter not valid.'
-    redirect_to my_notifications_path
+    @undelivered_notification_ids = @notifications.where(id: params[:notification_ids]).where(delivered: false).map(&:id)
+    @delivered_notification_ids = @notifications.where(id: params[:notification_ids]).where(delivered: true).map(&:id)
   end
 
-  def check_param_project
-    return unless params[:project] == ''
+  def set_show_read_all_button
+    @show_read_all_button = @notifications.count > Notification::MAX_PER_PAGE
+  end
 
-    flash[:error] = 'Filter not valid.'
-    redirect_to my_notifications_path
+  def set_selected_filter
+    @selected_filter = { kind: @filter_type, state: @filter_state, project: params[:project], group: params[:group] }
   end
 
   def show_more(notifications)
@@ -69,30 +79,22 @@ def show_more(notifications)
     notifications.page(params[:page]).per([total, Notification::MAX_PER_PAGE].min)
   end
 
-  def fetch_notifications
-    notifications = User.session!.notifications.for_web.includes(notifiable: [{ commentable: [{ comments: :user }, :project, :bs_request_actions] }, :bs_request_actions, :reviews])
-
-    if params[:project]
-      notifications.unread.for_project_name(params[:project])
-    elsif params[:group]
-      notifications.unread.for_group_title(params[:group])
+  def filter_notifications_by_state(notifications, filter_state)
+    case filter_state
+    when 'read'
+      notifications.read
     else
-      notifications.for_notifiable_type(params[:type])
+      notifications.unread
     end
   end
 
-  def paginated_notifications
-    notifications = fetch_notifications
-    params[:page] = notifications.page(params[:page]).total_pages if notifications.page(params[:page]).out_of_range?
-    params[:show_more] ? show_more(notifications) : notifications.page(params[:page])
-  end
-
-  def show_read_all_button?
-    fetch_notifications.count > Notification::MAX_PER_PAGE
-  end
-
   def send_notifications_information_rabbitmq(read_count, unread_count)
     RabbitmqBus.send_to_bus('metrics', "notification,action=read value=#{read_count}") if read_count.positive?
     RabbitmqBus.send_to_bus('metrics', "notification,action=unread value=#{unread_count}") if unread_count.positive?
   end
+
+  def paginate_notifications
+    @notifications = params[:show_more] ? show_more(@notifications) : @notifications.page(params[:page])
+    params[:page] = @notifications.total_pages if @notifications.out_of_range?
+  end
 end