Merge bb8d1f2 into 6b7ff09

app/helpers/hart_formatter.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+# When running in production mode use the default `Formatter`, which outputs all information
+# When running outside of production, e.g. in development, use `SimpleFormatter`,
+# which only displays the message, suppressing PID and timestamp
+# https://www.rubydoc.info/github/rails/rails/ActiveSupport/Logger/SimpleFormatter
+base_logger = Rails.env == 'production' ? ActiveSupport::Logger::Formatter : ActiveSupport::Logger::SimpleFormatter
+
+class HartFormatter < base_logger
+  def call(severity, timestamp, progname, msg)
+    if severity == 'ERROR'
+      User.admin.each do |admin|
+        admin.notify 'An Error occured!', msg.to_s, type: :error
+      end
+    end
+
+    super(severity, timestamp, progname, msg)
+  end
+end

app/models/notification.rb

@@ -2,5 +2,21 @@
 
 class Notification < ApplicationRecord
   belongs_to :user
+  enum notification_type: { default: 0, error: 1 }
   default_scope { order(read: :asc, created_at: :desc) }
+
+  def duplicate_of(other)
+    # its fine to use instance_exec here because we know the other object is
+    # also of our class and we want state to be private
+    self.class == other.class &&
+      state == other.instance_exec { state } &&
+      (created_at.to_f - other.created_at.to_f).abs < 60 # seconds
+    # we consider two notifications to be a duplicate of each other if they have the same contents and were created within one minute of each other
+  end
+
+  private
+
+  def state
+    [title, message, read, user.id, link, notification_type]
+  end
 end

app/models/user.rb

@@ -20,6 +20,7 @@ class User < ApplicationRecord
   has_many :users_assigned_to_requests
   has_many :requests, through: :users_assigned_to_requests
   has_many :servers
+  has_many :notifications
   has_and_belongs_to_many :request_responsibilities, class_name: 'Request', join_table: 'requests_responsible_users'
   validates :first_name, presence: true
   validates :last_name, presence: true
@@ -51,16 +52,23 @@ def self.search(term, role)
   end
 
   # notifications
-  def notify(title, message, link = '')
-    notify_slack("*#{title}*\n#{message}\n#{link}")
-
-    NotificationMailer.with(user: self, title: '[HART] ' + title.to_s, message: (message + link).to_s).notify_email.deliver_now if email_notifications
-
-    notification = Notification.new title: title, message: message
-    notification.user_id = id
-    notification.read = false
-    notification.link = link
-    notification.save # saving might fail, but there is no useful way to handle the error.
+  def notify(title, message, link = '', type: :default)
+    # notifications are ordered by descending created_at order (see `models/notification.rb`)
+    # notifications with the newest ("largest") timestamps are first
+    last_notification = notifications.first
+    # Set the `created_at` attribute, so that it can be compared by the `duplicate_of`
+    notification = Notification.new title: title, message: message, notification_type: type, user_id: id, read: false, link: link, created_at: DateTime.current
+
+    if notification.duplicate_of last_notification
+      last_notification.update(count: last_notification.count + 1)
+    else
+      notify_slack("*#{title}*\n#{message}\n#{link}")
+      NotificationMailer.with(user: self, title: '[HART] ' + title.to_s, message: (message + link).to_s).notify_email.deliver_now if email_notifications
+      notification.save
+      # saving might fail, there is however no good way to handle this error.
+      # We cannot log the error, because when using the HartFormatter, logging errors creates new notifications,
+      # these might also fail to save, creating an endless recursion.
+    end
   end
 
   after_initialize :set_default_role, if: :new_record?

app/views/notifications/_list.html.erb

@@ -1,7 +1,7 @@
 <% unless @notifications.empty? %>
 
   <% @notifications.each do |notification| %>
-    <div class="card mb-3 <%= "bg-primary-light" if not notification.read %>">
+    <div class="card mb-3 <%= 'bg-primary-light' unless notification.read %>">
       <div class="card-body">
         <%# Delete link %>
         <%= link_to fa_icon('trash lg'), notification_path(notification),
@@ -18,6 +18,8 @@
         <%# Notification title %>
         <h5 class="card-title">
           <%= link_to notification.title, mark_as_read_and_redirect_notification_path(notification) %>
+          <%= ' (' + notification.count.to_s + ' times)' if notification.count > 1 %>
+          <%= fa_icon('exclamation-triangle') if notification.error? %>
           <br>
           <span class="small">
             <%= I18n.l notification.created_at, format: :short %> (<%= time_ago_in_words notification.created_at %> ago)

config/application.rb

@@ -3,6 +3,7 @@
 require_relative 'boot'
 
 require 'rails/all'
+require './app/helpers/hart_formatter'
 
 # Require the gems listed in Gemfile, including any gems
 # you've limited to :test, :development, or :production.
@@ -16,6 +17,10 @@ class Application < Rails::Application
     # Load files in lib
     config.eager_load_paths << Rails.root.join('lib')
 
+    # Use custom log formatter which creates Notification objects for admin users
+    # when an application error is logged, making errors visisble in the application
+    config.log_formatter = HartFormatter.new
+
     # Use normal JS (default), not coffeescript
     # https://guides.rubyonrails.org/configuring.html#configuring-generators
     config.generators.javascript_engine = :js

config/environments/production.rb

@@ -81,9 +81,6 @@
   # Send deprecation notices to registered listeners.
   config.active_support.deprecation = :notify
 
-  # Use default logging formatter so that PID and timestamp are not suppressed.
-  config.log_formatter = ::Logger::Formatter.new
-
   # Use master key
   config.require_master_key = true
 

db/migrate/20190327182939_add_type_to_notifications.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddTypeToNotifications < ActiveRecord::Migration[5.2]
+  def change
+    add_column :notifications, :notification_type, :integer, default: 0
+  end
+end

db/migrate/20190406192841_add_count_to_notifications.rb

@@ -0,0 +1,5 @@
+class AddCountToNotifications < ActiveRecord::Migration[5.2]
+  def change
+    add_column :notifications, :count, :integer, default: 1
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_03_18_170254) do
+ActiveRecord::Schema.define(version: 2019_04_06_192841) do
 
   create_table "app_settings", force: :cascade do |t|
     t.integer "singleton_guard"
@@ -56,6 +56,8 @@
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.string "link"
+    t.integer "notification_type", default: 0
+    t.integer "count", default: 1
     t.index ["user_id"], name: "index_notifications_on_user_id"
   end
 

spec/factories/notifications.rb

@@ -8,4 +8,14 @@
     read { false }
     link { 'https://example.com' }
   end
+
+  factory :error_notification, parent: :notification do
+    notification_type { :error }
+    title { 'An Error occured!' }
+  end
+
+  factory :read_notification, parent: :notification do
+    title { 'This notification is already read' }
+    read { true }
+  end
 end

spec/helpers/hart_formatter_spec.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe HartFormatter do
+
+  before do
+    # ensure we have at least one admin
+    FactoryBot.create :admin
+  end
+
+  it 'notifies admins when an error is logged' do
+    expect do
+      Rails.logger.error('My Error')
+    end.to change(Notification, :count).by(User.admin.size)
+  end
+
+  it 'does not notify admins when the log is not an error' do
+    expect do
+      Rails.logger.info('My Info')
+    end.not_to change(Notification, :count)
+  end
+
+  it 'does not notify admins multiple times when the same error occurs' do
+    expect do
+      3.times { Rails.logger.error('My Error') }
+    end.to change(Notification, :count).by(User.admin.size)
+  end
+
+  it 'sets the notification count correctly' do
+    3.times { Rails.logger.error('My Error') }
+    expect(Notification.all.first.count).to eq(3)
+  end
+end

spec/models/user_spec.rb

@@ -114,4 +114,45 @@
       expect(user2.user_id).to be > user.user_id
     end
   end
+
+  describe 'notifying the user' do
+    let(:user) { FactoryBot.create :user }
+
+    def notify_user
+      user.notify 'my notification', 'some message', 'https://www.google.com/'
+    end
+
+    it 'creates a notification object' do
+      expect { notify_user }.to change(Notification, :count).by(1)
+    end
+
+    it 'notifies slack' do
+      allow(user).to receive(:notify_slack)
+      notify_user
+      expect(user).to have_received(:notify_slack)
+    end
+
+    it 'does not create multiple duplicate notifications' do
+      3.times { notify_user }
+      expect(user.notifications.size).to eq(1)
+    end
+
+    it 'sets the count of duplicate notifications correctly' do
+      4.times { notify_user }
+      expect(user.notifications.first.count).to eq(4)
+    end
+
+    it 'does not notify slack for duplicate notifiations' do
+      allow(user).to receive(:notify_slack)
+      3.times { notify_user }
+      expect(user).to have_received(:notify_slack).once
+    end
+
+    it 'does create notifications if the duplicates are one minute apart' do
+      notify_user
+      new_time = Time.now + 61 # seconds
+      allow(Time).to receive(:now).and_return new_time
+      expect { notify_user }.to change(Notification, :count).by(1)
+    end
+  end
 end

spec/views/notifications/_list.html.erb_spec.rb

@@ -0,0 +1,75 @@
+
+require 'rails_helper'
+
+RSpec.describe 'notifications/_list.html.erb', type: :view do
+
+  before do
+    assign :notifications, notifications
+    render
+  end
+
+  describe 'list of notifications' do
+
+    let(:read_notification) { FactoryBot.create :read_notification }
+
+    let(:unread_notification) { FactoryBot.create :notification }
+
+    let(:notifications) do
+      [FactoryBot.create(:error_notification), read_notification, unread_notification]
+    end
+
+    it 'renders the title of each notification' do
+      notifications.each do |notification|
+        expect(rendered).to have_text(notification.title)
+      end
+    end
+
+    it 'renders the message of each notification' do
+      notifications.each do |notification|
+        expect(rendered).to have_text(notification.message)
+      end
+    end
+  end
+
+  context 'when the notification is an error' do
+    let(:notifications) do
+      [FactoryBot.create(:error_notification)]
+    end
+
+    it 'renders an error icon' do
+      assert_select 'i[class="fa fa-exclamation-triangle"]'
+    end
+  end
+
+  context 'when the nofification is normal' do
+    let(:notifications) do
+      [FactoryBot.create(:notification)]
+    end
+
+    it 'does not render an error icon' do
+      assert_select 'i[class="fa fa-exclamation-triangle"]', false
+    end
+  end
+
+  describe 'notification count' do
+    # we can declare :count later because let works using lazy initialization
+    # pro tip: if you want to use a non-lazy let, you can use let!
+    let(:notifications) { [FactoryBot.create(:notification, count: count)] }
+
+    context 'when the notification occurred multiple times' do
+      let(:count) { 3 }
+
+      it 'displays the count' do
+        expect(rendered).to have_text(count.to_s + ' times')
+      end
+    end
+
+    context 'when the notification occured once' do
+      let(:count) { 1 }
+
+      it 'does not display the count' do
+        expect(rendered).not_to have_text(count.to_s + ' times')
+      end
+    end
+  end
+end