Merge b74dc22 into 6fa25cf

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/controllers/requests_controller.rb

@@ -146,7 +146,7 @@ def notify_request_update
 
     if @request.accepted?
       ([@request.user] + User.admin).uniq.each do |each|
-        each.notify('Request has been accepted', @request.description_text, @request.description_url(host_url))
+        each.notify('VM request has been accepted', @request.description_text, @request.description_url(host_url))
       end
       @request.users.uniq.each do |each|
         rights = @request.sudo_users.include?(each) ? 'sudo access' : 'access'
@@ -155,7 +155,7 @@ def notify_request_update
     elsif @request.rejected?
       message = @request.description_text
       message += @request.rejection_information.empty? ? '' : "\nwith comment: #{@request.rejection_information}"
-      notify_users('Request has been rejected', message, @request.description_url(host_url))
+      notify_users('VM request has been rejected', message, @request.description_url(host_url))
     end
   end
 

app/controllers/users_controller.rb

@@ -30,7 +30,7 @@ def update_role
     @user.update(role: params[:role])
     time = Time.zone.now.strftime('%d/%m/%Y')
     @user.notify('Changed role',
-                 "Your role has changed from #{former_role} to #{@user.role} on #{time}.",
+                 "Your role has changed from #{former_role} to #{@user.role}.",
                  url_for(controller: :users, action: 'show', id: @user.id))
     redirect_to users_path
   end

app/controllers/vms_controller.rb

@@ -57,8 +57,8 @@ def request_vm_archivation
     return if @vm.archived? || @vm.pending_archivation?
 
     @vm.users.each do |each|
-      each.notify("Your VM #{@vm.name} has been requested to be archived",
-                  "The VM will soon be archived and for that it will then be shut down.\nIf you still need this VM you can stop the archiving of this VM within #{ArchivationRequest.timeout_readable}.",
+      each.notify("VM #{@vm.name} archiving requested",
+                  "The VM will soon be archived and it will be shut down.\nIf you still need this VM you can stop the archiving of this VM within #{ArchivationRequest.timeout_readable}.",
                   url_for(controller: :vms, action: 'show', id: @vm.name))
     end
     @vm.set_pending_archivation
@@ -72,7 +72,7 @@ def request_vm_revive
     return if @vm.pending_reviving?
 
     User.admin.each do |each|
-      each.notify("VM #{@vm.name} has been requested to be revived",
+      each.notify("VM #{@vm.name} revival requested",
                   'The VM has to be revived.',
                   url_for(controller: :vms, action: 'show', id: @vm.name))
     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