diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 88eac47b..152f5ff1 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -99,7 +99,7 @@ body { width: auto; } -.unread { +.bg-primary-light { background-color: #e6f3ff; } diff --git a/app/assets/stylesheets/notifications.scss b/app/assets/stylesheets/notifications.scss index 27c3a957..d8daaa78 100644 --- a/app/assets/stylesheets/notifications.scss +++ b/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; diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index b80552e1..d03d8ae9 100644 --- a/app/controllers/dashboard_controller.rb +++ b/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 diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index c30c2e4e..61648c82 100644 --- a/app/controllers/notifications_controller.rb +++ b/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 diff --git a/app/controllers/requests_controller.rb b/app/controllers/requests_controller.rb index d679c759..2c1955cb 100644 --- a/app/controllers/requests_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index aa3ef400..c4867633 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/controllers/vms_controller.rb b/app/controllers/vms_controller.rb index f0b57fb5..0bb3101f 100644 --- a/app/controllers/vms_controller.rb +++ b/app/controllers/vms_controller.rb @@ -68,8 +68,8 @@ def request_vm_archivation return if !@vm || @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 @@ -84,7 +84,7 @@ def request_vm_revive return if !@vm || @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 diff --git a/app/views/dashboard/index.html.erb b/app/views/dashboard/index.html.erb index 85968f7e..3c3b2314 100644 --- a/app/views/dashboard/index.html.erb +++ b/app/views/dashboard/index.html.erb @@ -17,13 +17,11 @@
- <%= link_to notifications_path, method: :get, class: "dashboard-header-link" do %> -
-
-

Notifications

-
-
- <% end %> +
+ <%= link_to notifications_path, class: "dashboard-header-link text-white" do %> +

<%= fa_icon('bell') %> Notifications

+ <% end %> +
<%= render 'notifications/list' %>
diff --git a/app/views/notifications/_form.html.erb b/app/views/notifications/_form.html.erb deleted file mode 100644 index 8a51e8f1..00000000 --- a/app/views/notifications/_form.html.erb +++ /dev/null @@ -1,27 +0,0 @@ -<%= form_with(model: notification, local: true) do |form| %> - <% if notification.errors.any? %> -
-

<%= pluralize(notification.errors.count, "error") %> prohibited this notification from being saved:

- -
    - <% notification.errors.full_messages.each do |message| %> -
  • <%= message %>
  • - <% end %> -
-
- <% end %> - -
- <%= form.label :title %> - <%= form.text_field :title %> -
- -
- <%= form.label :message %> - <%= form.text_field :message %> -
- -
- <%= form.submit %> -
-<% end %> diff --git a/app/views/notifications/_list.html.erb b/app/views/notifications/_list.html.erb index a77e18e9..69d9a5b6 100644 --- a/app/views/notifications/_list.html.erb +++ b/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" %> -
+
">
- <%= link_to destroy_and_redirect_notification_path(notification), {controller: notification, action: 'destroy_and_redirect', method: :delete} do %> -
- <%= notification.title %> -
- <%= simple_format(notification.message).html_safe%> - <% end %> - -
- <%= 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 %> -
+ <%# 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 %> +
+ <%= link_to notification.title, mark_as_read_and_redirect_notification_path(notification) %> +
+ + <%= I18n.l notification.created_at,format: :short%> (<%= time_ago_in_words notification.created_at %> ago) + +
+ <%= simple_format(notification.message).html_safe %>
<% end %> <% else %> -

You don't have any notifications at the moment.

+
No unread notifications
<% end %> diff --git a/app/views/notifications/index.html.erb b/app/views/notifications/index.html.erb index d0c56608..2e75a900 100644 --- a/app/views/notifications/index.html.erb +++ b/app/views/notifications/index.html.erb @@ -1,5 +1,3 @@ -

<%= notice %>

-

Notifications

<%= render 'list' %> \ No newline at end of file diff --git a/app/views/notifications/new.html.erb b/app/views/notifications/new.html.erb deleted file mode 100644 index b0f4cf38..00000000 --- a/app/views/notifications/new.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -

New Notification

- -<%= render 'form', notification: @notification %> - -<%= link_to 'Back', notifications_path %> diff --git a/config/routes.rb b/config/routes.rb index d4ecc7ca..da44e792 100644 --- a/config/routes.rb +++ b/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 diff --git a/spec/controllers/notifications_controller_spec.rb b/spec/controllers/notifications_controller_spec.rb index b1567a35..a2d02aa4 100644 --- a/spec/controllers/notifications_controller_spec.rb +++ b/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 diff --git a/spec/factories/notifications.rb b/spec/factories/notifications.rb index 4e3c84b8..7f8d11f6 100644 --- a/spec/factories/notifications.rb +++ b/spec/factories/notifications.rb @@ -6,5 +6,6 @@ title { 'Test Notification' } message { 'This is a test' } read { false } + link { 'https://example.com' } end end diff --git a/spec/features/dashboard/dashboard_spec.rb b/spec/features/dashboard/dashboard_spec.rb index 5be370f2..ac84982e 100644 --- a/spec/features/dashboard/dashboard_spec.rb +++ b/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 diff --git a/spec/features/notifications/notifications_spec.rb b/spec/features/notifications/notifications_spec.rb index c9a225ff..28ce2d40 100644 --- a/spec/features/notifications/notifications_spec.rb +++ b/spec/features/notifications/notifications_spec.rb @@ -2,78 +2,71 @@ require 'rails_helper' -describe 'Index page', type: :feature do +describe 'Notifications index page', type: :feature do + before do + @user = create(:user) + login_as(@user) + end + context 'with notifications' do before do - user = create(:user) - login_as(user, scope: :user) - @notification = FactoryBot.create(:notification, user: user) - end - - it 'has notifications with title' do + @notification = FactoryBot.create(:notification, user: @user) visit notifications_path - + end + + it 'displays title' do expect(page).to have_text(@notification.title) end - - it 'has notifications with messages' do - visit notifications_path - + + it 'displays messages' do expect(page).to have_text(@notification.message) end + + it 'allows marking notification as read' do + expect{ + find("a[href='#{mark_as_read_and_redirect_notification_path(@notification)}']").click + }.to change{@notification.reload.read}.from(false).to(true) + end + + it 'redirects to notification links within the application' do + @notification.update(link: user_path(@user)) + visit notifications_path + find("a[href='#{mark_as_read_and_redirect_notification_path(@notification)}']").click + expect(page).to have_current_path(@notification.link) + end - it 'marks notification as read' do - visit mark_as_read_notification_path(@notification) - - expect(@notification.reload.read).to be(true) + it 'deals with empty notification links' do + @notification.update(link: nil) + visit notifications_path + find("a[href='#{mark_as_read_and_redirect_notification_path(@notification)}']").click + expect(page).to have_current_path(notifications_path) end end context 'without notifications' do - before do - user = create(:user) - login_as(user, scope: :user) - end - 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 -describe 'Nav header', type: :feature do - before do - user = create(:user) - login_as(user, scope: :user) - @notification = FactoryBot.create(:notification, user: user) - end - - it 'has a link to notifications' do - visit requests_path - - click_link 'header-notification-bell' - expect(page).to have_text(@notification.message) - end -end - describe 'Notification sending', type: :feature do - let(:user) do - user = create(:user) - login_as(user, scope: :user) - user + before do + @user = create(:user) + login_as(@user) + @attrs = FactoryBot.attributes_for(:notification) end it 'notifies slack' do - allow(user).to receive(:notify_slack) - user.notify('Notification title', 'The notification message', '') - expect(user).to have_received(:notify_slack) + allow(@user).to receive(:notify_slack) + @user.notify(@attrs['title'], @attrs['message'], @attrs['link']) + expect(@user).to have_received(:notify_slack) end - it 'creates a notification object' do - notification_count = Notification.all.length - user.notify('Notification title', 'The notification message', '') - expect(Notification.all.length).to equal(notification_count + 1) + it 'creates a notification object when calling notify' do + expect{ + @user.notify(@attrs['title'], @attrs['message'], @attrs['link']) + }.to change{Notification.count}.by(1) end end