Merge 71fd00f into d5b3f24

app/assets/stylesheets/application.scss

@@ -99,7 +99,7 @@ body {
   width: auto;
 }
 
-.unread {
+.bg-primary-light {
   background-color: #e6f3ff;
 }
 

app/assets/stylesheets/notifications.scss

@@ -2,21 +2,6 @@
 // They will automatically be included in application.css.
 // You can use Sass (SCSS) here: http://sass-lang.com/
 
-.unread {
-  background-color: #e6f3ff;
-}
-
-.big-icon {
-  font-size: 20px;
-}
-
-.icon-link {
-  margin-left: 5px;
-  &:hover {
-    text-decoration: none;
-  }
-}
-
 .notification-bell {
   #notification-alert {
     margin-top: -0.4em;

app/controllers/dashboard_controller.rb

@@ -12,7 +12,7 @@ def index
     redirect_to '/users/sign_in' if current_user.nil?
     initialize_vm_categories
     filter_vm_categories current_user unless current_user.admin?
-    @notifications = Notification.where(user: current_user).slice(0, number_of_notifications)
+    @notifications = Notification.where(user: current_user, read: false).slice(0, number_of_notifications)
   end
 
   private

app/controllers/notifications_controller.rb

@@ -1,66 +1,40 @@
 # frozen_string_literal: true
 
 class NotificationsController < ApplicationController
-  before_action :set_notification, only: %i[show edit update destroy mark_as_read destroy_and_redirect]
+  before_action :set_notification, only: %i[destroy mark_as_read mark_as_read_and_redirect]
 
   # GET /notifications
-  # GET /notifications.json
   def index
     @notifications = Notification.where(user: current_user)
   end
 
-  # GET /notifications/new
-  def new
-    @notification = Notification.new
-  end
-
-  # POST /notifications
-  # POST /notifications.json
-  def create
-    @notification = Notification.new(notification_params)
-    @notification.user_id = current_user.id
-    @notification.read = false
-
-    respond_to do |format|
-      if @notification.save
-        format.html { redirect_to notifications_path }
-      else
-        format.html { render :new }
-      end
-    end
-  end
-
   # DELETE /notifications/1
-  # DELETE /notifications/1.json
   def destroy
-    @notification.destroy
-    respond_to do |format|
-      format.html { redirect_back fallback_location: notifications_url }
-      format.json { head :no_content }
+    if @notification.destroy
+      notice = "Notification '#{@notification.title}' was successfully deleted."
+    else
+      alert = "Error while deleting notification '#{@notification.title}'."
     end
-  end
-
-  def destroy_and_redirect
-    link = @notification.link
-    @notification.destroy
-    respond_to do |format|
-      if link.nil? || link.empty?
-        format.html { redirect_back fallback_location: notifications_url }
-      else
-        format.html { redirect_to link }
-      end
-      format.json { head :no_content }
+    redirect_back fallback_location: notifications_path, notice: notice, alert: alert
+  end
+
+  # GET /notifications/1/mark_as_read_and_redirect
+  def mark_as_read_and_redirect
+    @notification.update(read: true) unless @notification.read
+    if @notification.link.present?
+      redirect_to @notification.link
+    else
+      redirect_back fallback_location: notifications_path
     end
   end
-
+
+  # GET /notifications/1/mark_as_read
   def mark_as_read
     @notification.read = true
-    respond_to do |format|
-      if @notification.save
-        format.html { redirect_back fallback_location: notifications_path }
-      else
-        format.html { redirect_back fallback_location: notifications_path, alert: 'Something went wrong.' }
-      end
+    if @notification.save
+      redirect_back fallback_location: notifications_path
+    else
+      redirect_back fallback_location: notifications_path, alert: 'Error while marking notifcation as read.'
     end
   end
 

app/views/dashboard/index.html.erb

@@ -17,13 +17,11 @@
 
   <div class="col-4">
     <div class="card mb-4">
-      <%= link_to notifications_path, method: :get, class: "dashboard-header-link" do %>
-        <div class="card-header bg-primary text-white">
-          <div class="btn-group">
-            <h3><i class="fa fa-bell"></i> Notifications</h3>
-          </div>
-        </div>
-      <% end %>
+      <div class="card-header bg-primary">
+        <%= link_to notifications_path, class: "dashboard-header-link text-white" do %>
+          <h3><%= fa_icon('bell') %> Notifications</h3>
+        <% end %>
+      </div>
       <div class="card-body">
         <%= render 'notifications/list' %>
       </div>

app/views/notifications/_list.html.erb

@@ -1,25 +1,33 @@
 <% unless @notifications.empty? %>
 
   <% @notifications.each do |notification| %>
-    <% class_names = notification.read ? "card mb-3 notification" : "card unread mb-3 notification" %>
-    <div class="<%= class_names %>" style="width:100%">
+    <div class="card mb-3 <%= "bg-primary-light" if not notification.read %>">
       <div class="card-body">
-        <%= link_to destroy_and_redirect_notification_path(notification), {controller: notification, action: 'destroy_and_redirect', method: :delete} do %>
-          <h5 class="card-title">
-            <%= notification.title %>
-          </h5>
-          <%= simple_format(notification.message).html_safe%>
-        <% end %>
-
-        <div class="pull-right">
-          <%= link_to notification, method: :delete, data: { confirm: 'Are you sure you want to delete this notification?' }, class: 'icon-link delete'  do %>
-            <%= fa_icon('trash', class: 'big-icon') %>
-          <% end %>
-        </div>
+        <%# Delete link %>
+        <%= link_to fa_icon('trash lg'), notification_path(notification),
+          method: :delete,
+          title: 'Delete',
+          data: { toggle: 'tooltip', placement: 'top',
+                  confirm: 'Are you sure you want to delete this notification?' },
+          class: "float-right" %>
+        <%# Mark as read link %>
+        <%= link_to fa_icon('check lg'), mark_as_read_notification_path(notification),
+          title: 'Mark as read',
+          data: { toggle: 'tooltip', placement: 'top'},
+          class: "float-right mr-2" unless notification.read %>
+        <%# Notification title %>
+        <h5 class="card-title">
+          <%= link_to notification.title, mark_as_read_and_redirect_notification_path(notification) %>
+          <br>
+          <span class="small">
+            <%= I18n.l notification.created_at,format: :short%> (<%= time_ago_in_words notification.created_at %> ago)
+          </span>
+        </h5>
+        <%= simple_format(notification.message).html_safe %>
       </div>
     </div>
   <% end %>
 
 <% else %>
-  <p>You don't have any notifications at the moment.</p>
+  <div id="no-notifications" class="text-center">No unread notifications<div>
 <% end %>

app/views/notifications/index.html.erb

@@ -1,5 +1,3 @@
-<p id="notice"><%= notice %></p>
-
 <h1>Notifications</h1>
 
 <%= render 'list' %>

config/routes.rb

@@ -18,10 +18,10 @@
   resources :hosts, only: %i[index show], constraints: { id: /.*/ }
 
   # '/notifications/...'
-  resources :notifications, only: %i[index new create destroy] do
+  resources :notifications, only: %i[index destroy] do
     member do
       get :mark_as_read
-      delete :destroy_and_redirect
+      get :mark_as_read_and_redirect
     end
     get :has_any, on: :collection, to: 'notifications#any?'
   end

spec/controllers/notifications_controller_spec.rb

@@ -3,18 +3,8 @@
 require 'rails_helper'
 
 RSpec.describe NotificationsController, type: :controller do
-  let(:user) { FactoryBot.create :user }
-  let(:valid_attributes) do
-    {
-      title: 'title',
-      message: 'message',
-      link: 'https://www.google.com',
-      user: user
-    }
-  end
-
-  # use let! instead of let otherwise the value will be lazily-instantiated
-  let!(:notification) { Notification.create!(valid_attributes) }
+  let(:notification) { FactoryBot.create :notification }
+  let(:user) { notification.user }
 
   before do
     sign_in user
@@ -28,24 +18,23 @@
     end
   end
 
-  describe 'POST #destroy_and_redirect' do
-    it 'destroys the notification' do
-      expect do
-        delete :destroy_and_redirect, params: { id: notification.to_param }
-      end.to change(Notification, :count).by(-1)
+  describe 'GET #mark_as_read_and_redirect' do
+    it 'marks the notification as read' do
+      expect {
+        get :mark_as_read_and_redirect, params: { id: notification.to_param }
+      }.to change{notification.reload.read}.from(false).to(true)
     end
 
     it 'redirects to the notification link' do
-      link = notification.link
-      delete :destroy_and_redirect, params: { id: notification.to_param }
-      expect(response).to redirect_to(link)
+      get :mark_as_read_and_redirect, params: { id: notification.to_param }
+      expect(response).to redirect_to(notification.link)
     end
 
     it 'redirects back if no link is provided' do
       notification.update!(link: '')
       redirect_url = 'https://www.google.com/'
       from redirect_url
-      delete :destroy_and_redirect, params: { id: notification.to_param }
+      get :mark_as_read_and_redirect, params: { id: notification.to_param }
       expect(response).to redirect_to(redirect_url)
     end
   end

spec/factories/notifications.rb

@@ -6,5 +6,6 @@
     title { 'Test Notification' }
     message { 'This is a test' }
     read { false }
+    link { 'https://example.com' }
   end
 end

spec/features/dashboard/dashboard_spec.rb

@@ -38,7 +38,7 @@
 
     it 'does not redirect after deleting notification' do
       visit dashboard_path
-      all(:css, '.delete.icon-link').first.click
+      find(:css, "a[data-method='delete'][href='#{notification_path(@notifications.second)}']").click
       expect(current_path).to eql(dashboard_path)
     end
 
@@ -51,7 +51,7 @@
   context 'without notifications' do
     it 'informs about no notifications' do
       visit dashboard_path
-      expect(page).to have_text('You don\'t have any notifications at the moment.')
+      expect(page).to have_css('#no-notifications', count: 1)
     end
   end
 end

spec/features/notifications/notifications_spec.rb

@@ -37,8 +37,7 @@
 
     it 'informs about no notifications' do
       visit notifications_path
-
-      expect(page).to have_text('You don\'t have any notifications at the moment.')
+      expect(page).to have_css('#no-notifications', count: 1)
     end
   end
 end