From b952f600f13b2da8bcacd1a8c7943153a594ffdf Mon Sep 17 00:00:00 2001 From: Colin Darie Date: Mon, 4 Jun 2018 20:39:53 +0200 Subject: [PATCH] Add & removal of notification from a check --- Gemfile | 3 + Gemfile.lock | 3 + app/controllers/checks_controller.rb | 13 ++- app/controllers/notifications_controller.rb | 44 ++++++++++ app/frontend/scss/icons.scss | 5 ++ app/frontend/scss/index.scss | 1 + app/helpers/notifications_helper.rb | 7 ++ app/models/check.rb | 5 +- app/models/notification.rb | 4 +- app/policies/notification_policy.rb | 21 +++++ app/views/checks/_form.html.erb | 15 +++- app/views/notifications/_nested_form.html.erb | 29 +++++++ .../_nested_form_headers.html.erb | 15 ++++ app/views/notifications/destroy.js.erb | 1 + .../initializers/content_security_policy.rb | 2 +- config/locales/en.yml | 9 ++ config/routes.rb | 5 +- test/application_system_test_case.rb | 5 ++ test/chexpire_assertions.rb | 18 ++++ test/factories/checks.rb | 6 ++ test/factories/notifications.rb | 2 +- test/models/notification_test.rb | 2 +- test/policies/check_policy_test.rb | 37 ++++++-- test/policies/notification_policy_test.rb | 32 +++++++ test/system/checks_test.rb | 85 +++++++++++++++++++ 25 files changed, 354 insertions(+), 15 deletions(-) create mode 100644 app/controllers/notifications_controller.rb create mode 100644 app/frontend/scss/icons.scss create mode 100644 app/policies/notification_policy.rb create mode 100644 app/views/notifications/_nested_form.html.erb create mode 100644 app/views/notifications/_nested_form_headers.html.erb create mode 100644 app/views/notifications/destroy.js.erb create mode 100644 test/policies/notification_policy_test.rb create mode 100644 test/system/checks_test.rb diff --git a/Gemfile b/Gemfile index 9f96957..ea40166 100644 --- a/Gemfile +++ b/Gemfile @@ -39,6 +39,9 @@ gem 'bcrypt', '~> 3.1.7' gem 'open4' gem 'naught' + +gem 'octicons' + # Reduces boot times through caching; required in config/boot.rb gem 'bootsnap', '>= 1.1.0', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 3326b9c..646924f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -175,6 +175,8 @@ GEM notiffany (0.1.1) nenv (~> 0.1) shellany (~> 0.0) + octicons (7.3.0) + nokogiri (>= 1.6.3.1) open4 (1.3.4) orm_adapter (0.5.0) parallel (1.12.1) @@ -335,6 +337,7 @@ DEPENDENCIES listen (>= 3.0.5, < 3.2) mysql2 (>= 0.4.4, < 0.6.0) naught + octicons open4 pry-byebug pry-rails diff --git a/app/controllers/checks_controller.rb b/app/controllers/checks_controller.rb index c1fd0df..185d488 100644 --- a/app/controllers/checks_controller.rb +++ b/app/controllers/checks_controller.rb @@ -10,6 +10,7 @@ class ChecksController < ApplicationController def new @check = Check.new + build_empty_notification authorize @check end @@ -27,7 +28,9 @@ class ChecksController < ApplicationController end end - def edit; end + def edit + build_empty_notification + end def update if @check.update(update_check_params) @@ -35,6 +38,7 @@ class ChecksController < ApplicationController redirect_to checks_path else flash.now[:alert] = "An error occured." + build_empty_notification render :edit end end @@ -62,6 +66,11 @@ class ChecksController < ApplicationController end def check_params(*others) - params.require(:check).permit(:domain, :domain_created_at, :comment, :vendor, *others) + params.require(:check).permit(:domain, :domain_created_at, :comment, :vendor, *others, + notifications_attributes: [:id, :channel, :recipient, :delay]) + end + + def build_empty_notification + @check.notifications.build end end diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb new file mode 100644 index 0000000..b572bf3 --- /dev/null +++ b/app/controllers/notifications_controller.rb @@ -0,0 +1,44 @@ +class NotificationsController < ApplicationController + before_action :authenticate_user! + before_action :set_notification, except: [:create] + + def create + check = Check.find(params[:check_id]) + @notification = check.notifications.build(notification_params) + authorize @notification + + if @notification.save + flash[:notice] = "Your notification has been saved." + redirect_to check_path + else + flash.now[:alert] = "An error occured." + render "checks/edit" + end + end + + def destroy + @notification.destroy! + + respond_to do |format| + format.js + end + end + + private + + def set_notification + # joins the check because policy use the check relation + @notification = Notification + .joins(:check) + .find_by!(id: params[:id], check_id: params[:check_id]) + authorize @notification + end + + def notification_params + params.require(:notification).permit(:channel, :recipient, :delay) + end + + def check_path + edit_check_path(check_id: params[:check_id]) + end +end diff --git a/app/frontend/scss/icons.scss b/app/frontend/scss/icons.scss new file mode 100644 index 0000000..2bb0254 --- /dev/null +++ b/app/frontend/scss/icons.scss @@ -0,0 +1,5 @@ +.octicon { + fill: currentColor; + vertical-align: text-top; + display: inline-block; +} diff --git a/app/frontend/scss/index.scss b/app/frontend/scss/index.scss index 649b167..24da8f7 100644 --- a/app/frontend/scss/index.scss +++ b/app/frontend/scss/index.scss @@ -1,3 +1,4 @@ @import '~bootstrap/scss/bootstrap'; @import 'layout'; +@import 'icons'; @import 'components/users'; diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 7342393..7afd73d 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -1,2 +1,9 @@ module NotificationsHelper + def many_channels_available? + Notification.channels.many? + end + + def recipient_col_class + many_channels_available? ? "col-md-7" : "col-md-9" + end end diff --git a/app/models/check.rb b/app/models/check.rb index 82e9749..7a02a34 100644 --- a/app/models/check.rb +++ b/app/models/check.rb @@ -29,7 +29,10 @@ class Check < ApplicationRecord belongs_to :user has_many :logs, class_name: "CheckLog" - has_many :notifications + has_many :notifications, validate: true, dependent: :destroy + accepts_nested_attributes_for :notifications, + allow_destroy: true, + reject_if: lambda { |at| at["recipient"].blank? && at["delay"].blank? } enum kind: [:domain, :ssl] diff --git a/app/models/notification.rb b/app/models/notification.rb index 21983f3..f3c3bd1 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -3,7 +3,7 @@ # Table name: notifications # # id :bigint(8) not null, primary key -# channel :integer not null +# channel :integer default("email"), not null # delay :integer not null # recipient :string(255) not null # sent_at :datetime @@ -28,7 +28,7 @@ class Notification < ApplicationRecord enum status: [:pending, :ongoing, :succeed, :failed] validates :channel, presence: true - validates :delay, presence: true + validates :delay, numericality: { only_integer: true, greater_than_or_equal_to: 1 } validates :recipient, presence: true def pending! diff --git a/app/policies/notification_policy.rb b/app/policies/notification_policy.rb new file mode 100644 index 0000000..68c5167 --- /dev/null +++ b/app/policies/notification_policy.rb @@ -0,0 +1,21 @@ +class NotificationPolicy < ApplicationPolicy + class Scope < Scope + def resolve + scope.joins(:check).where(checks: { user: user }) + end + end + + def destroy? + check_owner? + end + + def show? + false + end + + private + + def check_owner? + record.check.user == user + end +end diff --git a/app/views/checks/_form.html.erb b/app/views/checks/_form.html.erb index ffd7c39..e4dd82f 100644 --- a/app/views/checks/_form.html.erb +++ b/app/views/checks/_form.html.erb @@ -12,5 +12,18 @@ <%= f.input :active %> <% end %> - <%= f.button :submit, "Validate", class: "btn-primary" %> +

<%= t(".notifications") %>

+

<%= t(".notifications_hint") %>

+ + <%- check.notifications.each_with_index do |notification, index| %> +
+ <%= f.fields_for :notifications, notification do |nf| %> + <%= render "notifications/nested_form_headers", f: nf if index.zero? %> + <%= render "notifications/nested_form", f: nf, check: check %> + <% end %> +
+ <% end %> + + + <%= f.button :submit, class: "btn-primary mt-5" %> <% end %> diff --git a/app/views/notifications/_nested_form.html.erb b/app/views/notifications/_nested_form.html.erb new file mode 100644 index 0000000..1ffa964 --- /dev/null +++ b/app/views/notifications/_nested_form.html.erb @@ -0,0 +1,29 @@ +
+
+ <%- if many_channels_available? %> +
+ <%- if f.object.new_record? -%> + <%= f.input :channel, collection: Notification.channels.keys, label: false %> + <% else -%> + <%= f.input_field :channel, as: :string, readonly: true, class: "form-control-plaintext" %> + <%- end %> +
+ <% end %> + +
+ <%= f.input :recipient, as: :email, label: false %> +
+ +
+ <%= f.input :delay, as: :integer, label: false %> +
+ +
+ <% if f.object.persisted? %> + <%= link_to check_notification_path(check, f.object), method: :delete, remote: true, class: "btn btn-danger" do %> + <%== Octicons::Octicon.new("x", width: 15, height: 20).to_svg %> + <% end %> + <% end %> +
+
+
diff --git a/app/views/notifications/_nested_form_headers.html.erb b/app/views/notifications/_nested_form_headers.html.erb new file mode 100644 index 0000000..37ca899 --- /dev/null +++ b/app/views/notifications/_nested_form_headers.html.erb @@ -0,0 +1,15 @@ +
+ <%- if many_channels_available? %> +
+ <%= f.label :channel %> +
+ <% end %> + +
+ <%= f.label :recipient %> +
+ +
+ <%= f.label :delay %> +
+
diff --git a/app/views/notifications/destroy.js.erb b/app/views/notifications/destroy.js.erb new file mode 100644 index 0000000..7e031d0 --- /dev/null +++ b/app/views/notifications/destroy.js.erb @@ -0,0 +1 @@ +document.querySelector("[data-notification-id='<%= @notification.id %>']").remove(); diff --git a/config/initializers/content_security_policy.rb b/config/initializers/content_security_policy.rb index 57519e9..d83ffdf 100644 --- a/config/initializers/content_security_policy.rb +++ b/config/initializers/content_security_policy.rb @@ -18,7 +18,7 @@ Rails.application.config.content_security_policy do |policy| end # If you are using UJS then enable automatic nonce generation -# Rails.application.config.content_security_policy_nonce_generator = -> request { SecureRandom.base64(16) } +Rails.application.config.content_security_policy_nonce_generator = -> request { SecureRandom.base64(16) } # Report CSP violations to a specified URI # For further information see the following documentation: diff --git a/config/locales/en.yml b/config/locales/en.yml index 86d0712..74de909 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -17,6 +17,11 @@ en: new: tos_acceptance_html: "You must accept our Terms of service" + simple_form: + placeholders: + notifications: + recipient: john@example.org + flashes: user_not_authorized: "You are not authorized to access to this resource." @@ -44,3 +49,7 @@ en: You have not set up a check yet. Please add a domain or a ssl ! + form: + notifications_hint: | + Receive notifications to warn you when our system detects that the + expiration date is coming. The delay is set in number of days. diff --git a/config/routes.rb b/config/routes.rb index b0d436a..3e481ec 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,6 +1,7 @@ # == Route Map # # Prefix Verb URI Pattern Controller#Action +# check_notification DELETE /checks/:check_id/notifications/:id(.:format) notifications#destroy # checks GET /checks(.:format) checks#index # POST /checks(.:format) checks#create # new_check GET /checks/new(.:format) checks#new @@ -46,7 +47,9 @@ Rails.application.routes.draw do # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html - resources :checks, except: [:show] + resources :checks, except: [:show] do + resources :notifications, only: [:destroy] + end devise_for :users root to: "pages#home" diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb index 27aacc6..89fe8b5 100644 --- a/test/application_system_test_case.rb +++ b/test/application_system_test_case.rb @@ -2,4 +2,9 @@ require "test_helper" class ApplicationSystemTestCase < ActionDispatch::SystemTestCase driven_by :headless_chrome + + def teardown + Capybara.reset_sessions! + Warden.test_reset! + end end diff --git a/test/chexpire_assertions.rb b/test/chexpire_assertions.rb index 453fe79..32265eb 100644 --- a/test/chexpire_assertions.rb +++ b/test/chexpire_assertions.rb @@ -2,4 +2,22 @@ module ChexpireAssertions def assert_just_now(expected) assert_in_delta expected.to_i, Time.now.to_i, 1.0 end + + def assert_permit(user, record, action) + msg = "User #{user.inspect} should be permitted to #{action} #{record}, but isn't permitted" + assert policy_permit(user, record, action), msg + end + + def refute_permit(user, record, action) + msg = "User #{user.inspect} should NOT be permitted to #{action} #{record}, but is permitted" + refute policy_permit(user, record, action), msg + end + + private + + def policy_permit(user, record, action) + test_name = self.class.ancestors.select { |a| a.to_s.match(/PolicyTest/) }.first + klass = test_name.to_s.gsub(/Test/, "") + klass.constantize.new(user, record).public_send("#{action}?") + end end diff --git a/test/factories/checks.rb b/test/factories/checks.rb index 181327f..7621f96 100644 --- a/test/factories/checks.rb +++ b/test/factories/checks.rb @@ -67,5 +67,11 @@ FactoryBot.define do trait :inactive do active false end + + trait :with_notifications do + after :create do |check| + create_list :notification, 2, check: check + end + end end end diff --git a/test/factories/notifications.rb b/test/factories/notifications.rb index 8bb069f..865f6fe 100644 --- a/test/factories/notifications.rb +++ b/test/factories/notifications.rb @@ -3,7 +3,7 @@ # Table name: notifications # # id :bigint(8) not null, primary key -# channel :integer not null +# channel :integer default("email"), not null # delay :integer not null # recipient :string(255) not null # sent_at :datetime diff --git a/test/models/notification_test.rb b/test/models/notification_test.rb index 543a662..19592e7 100644 --- a/test/models/notification_test.rb +++ b/test/models/notification_test.rb @@ -3,7 +3,7 @@ # Table name: notifications # # id :bigint(8) not null, primary key -# channel :integer not null +# channel :integer default("email"), not null # delay :integer not null # recipient :string(255) not null # sent_at :datetime diff --git a/test/policies/check_policy_test.rb b/test/policies/check_policy_test.rb index 0245c66..ff90b3c 100644 --- a/test/policies/check_policy_test.rb +++ b/test/policies/check_policy_test.rb @@ -1,13 +1,40 @@ require "test_helper" class CheckPolicyTest < ActiveSupport::TestCase - def test_scope; end + setup do + @owner, @other = create_list(:user, 2) + @check = create(:check, user: @owner) + end - def test_show; end + test "create" do + assert_permit @other, Check, :create + assert_permit @other, Check, :new + end - def test_create; end + test "check owner" do + assert_permit @owner, @check, :update + assert_permit @owner, @check, :edit + assert_permit @owner, @check, :destroy + assert_permit @owner, @check, :show + end - def test_update; end + test "anonymous and other user" do + refute_permit @other, @check, :update + refute_permit @other, @check, :edit + refute_permit @other, @check, :destroy + refute_permit @other, @check, :show - def test_destroy; end + refute_permit nil, @check, :update + refute_permit nil, @check, :edit + refute_permit nil, @check, :destroy + refute_permit nil, @check, :show + end + + test "scope only to owner" do + others = create_list(:check, 2, user: @other) + + assert_empty Pundit.policy_scope!(nil, Check) + assert_equal [@check], Pundit.policy_scope!(@owner, Check) + assert_equal others, Pundit.policy_scope!(@other, Check) + end end diff --git a/test/policies/notification_policy_test.rb b/test/policies/notification_policy_test.rb new file mode 100644 index 0000000..7c0012e --- /dev/null +++ b/test/policies/notification_policy_test.rb @@ -0,0 +1,32 @@ +require "test_helper" + +class NotificationPolicyTest < ActiveSupport::TestCase + setup do + @owner, @other = create_list(:user, 2) + @notification = create(:notification, check: build(:check, user: @owner)) + end + + test "permit to check user" do + assert_permit @owner, @notification, :destroy + end + + test "disallow to anonymous and other user" do + refute_permit @other, @notification, :destroy + refute_permit nil, @notification, :destroy + end + + test "scope only to user checks" do + other_notifications = create_list(:notification, 2, check: build(:check, user: @other)) + + assert_empty Pundit.policy_scope!(nil, Notification) + assert_equal [@notification], Pundit.policy_scope!(@owner, Notification) + assert_equal other_notifications, Pundit.policy_scope!(@other, Notification) + end + + test "disabled actions" do + refute_permit @owner, @notification, :update + refute_permit @owner, @notification, :edit + refute_permit @owner, @notification, :create + refute_permit @owner, @notification, :index + end +end diff --git a/test/system/checks_test.rb b/test/system/checks_test.rb new file mode 100644 index 0000000..83bdcc2 --- /dev/null +++ b/test/system/checks_test.rb @@ -0,0 +1,85 @@ +require "application_system_test_case" + +class ChecksTest < ApplicationSystemTestCase + setup do + @user = create(:user) + login_as(@user) + + @check = create(:check, :with_notifications, user: @user) + end + + test "create a check and a notification" do + visit new_check_path + + domain = "domain-test.fr" + fill_in("check[domain]", with: domain) + choose "domain" + + recipient = "recipient@example.org" + fill_in("check[notifications_attributes][0][recipient]", with: recipient) + fill_in("check[notifications_attributes][0][delay]", with: 30) + + click_button + + assert_equal checks_path, page.current_path + + assert page.has_css?(".alert-success") + assert page.has_content?(domain) + + notification = Notification.last + assert_equal recipient, notification.recipient + assert_equal 30, notification.delay + assert notification.email? + assert notification.pending? + end + + test "remove a notification" do + visit edit_check_path(@check) + notification = @check.notifications.first + + selector = "[data-notification-id=\"#{notification.id}\"]" + + assert_difference "Notification.where(check_id: #{@check.id}).count", -1 do + within selector do + find(".btn-danger").click + end + + page.has_no_content?(selector) + end + end + + test "update a check" do + visit edit_check_path(@check) + + fill_in "check[comment]", with: "My comment" + + click_button "Update Check" + + assert_equal checks_path, page.current_path + + assert page.has_css?(".alert-success") + assert page.has_content?("My comment") + end + + test "add a notification" do + visit edit_check_path(@check) + + recipient = "recipient2@example.org" + fill_in("check[notifications_attributes][2][recipient]", with: recipient) + fill_in("check[notifications_attributes][2][delay]", with: 55) + + assert_difference "Notification.where(check_id: #{@check.id}).count", +1 do + click_button "Update Check" + + assert_equal checks_path, page.current_path + end + + assert page.has_css?(".alert-success") + + notification = Notification.last + assert_equal recipient, notification.recipient + assert_equal 55, notification.delay + assert notification.email? + assert notification.pending? + end +end