[PATCH 1/2] initial implementation of basic notifications

Matt Wagner matt.wagner at redhat.com
Mon Oct 8 18:15:01 UTC 2012


Various ramblings inline below:

On Fri, Oct 05, 2012 at 03:44:13PM -0400, tzumainn at redhat.com wrote:
> From: Tzu-Mainn Chen <tzumainn at redhat.com>
> 
> ---
>  src/app/controllers/application_controller.rb      |    1 +
>  src/app/controllers/notifications_controller.rb    |   64 ++++++++++++
>  src/app/models/deployment.rb                       |   17 +++-
>  src/app/models/instance.rb                         |   10 ++
>  src/app/models/instance_observer.rb                |   52 ++++++++---
>  src/app/models/notification.rb                     |   68 +++++++++++++
>  src/app/models/user_notification.rb                |   32 ++++++
>  src/app/views/notifications/index.html.haml        |   20 ++++
>  src/config/application.rb                          |    1 +
>  src/config/locales/en.yml                          |   48 +++++++++
>  src/config/routes.rb                               |    6 +
>  .../migrate/20120920130441_create_notifications.rb |   28 +++++
>  src/lib/notifications/notifier.rb                  |  105 ++++++++++++++++++++
>  src/lib/notify.rb                                  |   13 +++
>  14 files changed, 451 insertions(+), 14 deletions(-)
>  create mode 100644 src/app/controllers/notifications_controller.rb
>  create mode 100644 src/app/models/notification.rb
>  create mode 100644 src/app/models/user_notification.rb
>  create mode 100644 src/app/views/notifications/index.html.haml
>  create mode 100644 src/db/migrate/20120920130441_create_notifications.rb
>  create mode 100644 src/lib/notifications/notifier.rb
>  create mode 100644 src/lib/notify.rb

You are absolutely free to use whichever you prefer, but this would have
been more fun to read if it were a GitHub pull request. O:-)

> diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb
> index 2c78500..53698ce 100644
> --- a/src/app/controllers/application_controller.rb
> +++ b/src/app/controllers/application_controller.rb
> @@ -24,6 +24,7 @@ require 'will_paginate/array'
>  class ApplicationController < ActionController::Base
>    # FIXME: not sure what we're doing aobut service layer w/ deltacloud
>    include ApplicationService
> +  include Notifications
>    helper_method :current_session, :current_user, :filter_view?
>    before_filter :read_breadcrumbs, :set_locale, :check_session_expiration
>  
> diff --git a/src/app/controllers/notifications_controller.rb b/src/app/controllers/notifications_controller.rb
> new file mode 100644
> index 0000000..dc1187e
> --- /dev/null
> +++ b/src/app/controllers/notifications_controller.rb
> @@ -0,0 +1,64 @@
> +#
> +#   Copyright 2012 Red Hat, Inc.
> +#
> +#   Licensed under the Apache License, Version 2.0 (the "License");
> +#   you may not use this file except in compliance with the License.
> +#   You may obtain a copy of the License at
> +#
> +#       http://www.apache.org/licenses/LICENSE-2.0
> +#
> +#   Unless required by applicable law or agreed to in writing, software
> +#   distributed under the License is distributed on an "AS IS" BASIS,
> +#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +#   See the License for the specific language governing permissions and
> +#   limitations under the License.
> +#
> +
> +class NotificationsController < ApplicationController
> +  before_filter :require_user
> +
> +  def index
> +    clear_breadcrumbs
> +    save_breadcrumb(notifications_path)
> +
> +    load_notifications
> +    load_headers

This is a common pattern in our code, but it doesn't really feel right
to me. In four lines, we've called four separate methods that either
set up various variables for us, or that do stuff not related to an
"index" action.

I'm surprised breadcrumbs can't be set up more globally in
ApplicationController, or at least as a before_filter per-controller,
and that we can't combine clearing old breadcrumbs and saving a new one
into one call. I'd feel much better about all of this is we called
something like "Breadcrumb.replace(notifications_path)" which feels much
more like an object-oriented approach. (Not that I'm asking you to
refactor our breadcrumbs implementation for your patch.)

Even though the net effect would be the same, I also think I'd feel
better if our code was something like:

 notifications = load_notifications
 @header = load_headers

But more on load_notifications in a moment.

> +    respond_to do |format|
> +      format.html
> +      format.js
> +    end

Wouldn't you get the same behavior if you just deleted this block, since
that's the default behavior?

> +  end
> +
> +  def filter
> +    redirect_to_original({ "source_type" => params[:source_type] })
> +  end
> +
> +  protected
> +
> +  def load_notifications
> +    conditions = ["exists (select 1 from user_notifications
> +                                   where user_id = ?
> +                                   and notification_id = notifications.id)",
> +                  current_user.id
> +                 ]
> +
> +    @notifications = Notification.find(:all,
> +                                       :include => :source,
> +                                       :conditions => conditions,
> +                                       :order => "created_at desc"
> +                                       )
> +
> +    @paginated_notifications = paginate_collection(@notifications, params[:page], PER_PAGE)
> +  end

I'm not sure this belongs in a controller.

It would be really nice if there was a method like
UserNotification.find_for_user or something of the sort that did exactly
what load_notifications does, sans pagination.

Because then you could drop the load_notifications method entirely, and
just put something like this in the index method:

  notifications = paginate_collection(
     UserNotification.find_for_user(current_user.id),
     params[:page], PER_PAGE)

Or something along those lines.

> +
> +  def load_headers
> +    @header = [
> +      { :name => t('notifications.index.notification_time'), :sortable => false },
> +      { :name => t('notifications.index.level'), :sortable => false },
> +      { :name => t('notifications.index.source'), :sortable => false },
> +      { :name => t('notifications.index.message'), :sortable => false },
> +      { :name => t('notifications.index.details'), :sortable => false },
> +    ]
> +  end
> +end
> diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
> index 9062c04..e7ba318 100644
> --- a/src/app/models/deployment.rb
> +++ b/src/app/models/deployment.rb
> @@ -243,13 +243,18 @@ class Deployment < ActiveRecord::Base
>      all_inst_match, account, errs = pick_provider_selection_match
>  
>      if all_inst_match
> +      summary = I18n.t('deployments.events.launch_attempt',
> +                           :account => account.name)
>        self.events << Event.create(
>          :source => self,
>          :event_time => DateTime.now,
>          :status_code => 'deployment_launch_match',
> -        :summary => I18n.t('deployments.events.launch_attempt',
> -                           :account => account.name)
> +        :summary => summary
>        )
> +      Notify.notice('notifications.deployments.launch_attempt',
> +                    :details => summary,
> +                    :source => self,
> +                    :user => user)
>      else
>        if errs.any?
>          raise I18n.t('deployments.errors.match_not_found_with_errors',
> @@ -777,6 +782,9 @@ class Deployment < ActiveRecord::Base
>          :summary => I18n.t('deployments.events.state_changed',
>                             :state => self.state)
>        )
> +      Notify.notice("notifications.deployments.#{self.state}_state_change",
> +                    :source => self,
> +                    :user => self.owner)
>      end
>    end
>  
> @@ -796,6 +804,11 @@ class Deployment < ActiveRecord::Base
>            :summary => I18n.t('deployments.errors.launch_failed'),
>            :description => $!.message
>          )
> +        Notify.error('notifications.deployments.launch_failed',
> +                     :source => self,
> +                     :details => $!.message,
> +                     :user => self.owner)
> +
>          update_attribute(:state, STATE_FAILED)
>          cleanup_failed_launch
>          logger.error $!.message
> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> index 8e02002..a011016 100644
> --- a/src/app/models/instance.rb
> +++ b/src/app/models/instance.rb
> @@ -225,6 +225,9 @@ class Instance < ActiveRecord::Base
>      event = Event.create!(:source => self, :event_time => Time.now,
>                            :summary => "#{action} action queued",
>                            :status_code => "#{action}_queued")
> +    Notify.notice("notifications.instances.#{action}_action_queued",
> +                  :source => self,
> +                  :user => user)
>  
>      return task
>    end
> @@ -473,6 +476,10 @@ class Instance < ActiveRecord::Base
>        :summary => "Failed to stop instance #{self.name}",
>        :description => $!.message
>      )
> +    Notify.error("notifications.instances.stop_failed",
> +                 :details => $!.message,
> +                 :source => self,
> +                 :user => user)
>      logger.error $!.message
>      logger.error $!.backtrace.join("\n  ")
>      false
> @@ -493,6 +500,9 @@ class Instance < ActiveRecord::Base
>      event = Event.create!(:source => self, :event_time => Time.now,
>                            :summary => "Instance is not accessible, state changed to stopped",
>                            :status_code => "forced_stop")
> +    Notify.warning("notifications.instances.forced_stop",
> +                 :source => self,
> +                 :user => user)
>    end
>  
>    def deployed?
> diff --git a/src/app/models/instance_observer.rb b/src/app/models/instance_observer.rb
> index d42fbe6..f19dfed 100644
> --- a/src/app/models/instance_observer.rb
> +++ b/src/app/models/instance_observer.rb
> @@ -89,6 +89,9 @@ class InstanceObserver < ActiveRecord::Observer
>      event = Event.new(:source => instance, :event_time => instance.created_at,
>                        :summary => "created")
>      event.save!
> +    Notify.success("notifications.instances.created",
> +                 :source => instance,
> +                 :user => instance.owner)
>    end
>  
>    def after_update(instance)
> @@ -99,31 +102,56 @@ class InstanceObserver < ActiveRecord::Observer
>                          :summary => "state changed to #{instance.state}",
>                          :change_hash => instance.changes, :status_code => instance.state)
>        event.save!
> +      Notify.notice("notifications.instances.#{instance.state}_state_change",
> +                     :source => instance,
> +                     :user => instance.owner)
>        if instance.state == Instance::STATE_RUNNING && instance.deployment
> -        event2 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
> -                        :status_code => "first_running", :summary => "1st instance is running",
> -                        :change_hash => instance.changes) if instance.first_running?
> -        event3 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
> -                        :status_code => "all_running", :summary => "All instances are running",
> -                        :change_hash => instance.changes) if instance.deployment.all_instances_running?
> -        event2.save! if event2
> -        event3.save! if event3
> +        if instance.first_running?
> +          event2 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
> +                             :status_code => "first_running", :summary => "1st instance is running",
> +                             :change_hash => instance.changes)
> +          event2.save!
> +          Notify.success("notifications.deployments.first_instance_running",
> +                        :source => instance.deployment,
> +                        :user => instance.deployment.owner)
> +        end
> +        if instance.deployment.all_instances_running?
> +          event3 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
> +                             :status_code => "all_running", :summary => "All instances are running",
> +                             :change_hash => instance.changes)
> +          event3.save!
> +          Notify.success("notifications.deployments.all_instances_running",
> +                        :source => instance.deployment,
> +                        :user => instance.deployment.owner)
> +        end
>        elsif (instance.state == Instance::STATE_STOPPED || instance.state == Instance::STATE_ERROR) && instance.deployment
>          if instance.deployment.any_instance_running? && !(instance.deployment.events.empty?)
> -          event4 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
> -                             :status_code => "some_stopped", :summary => "Some instances are stopped",
> -                             :change_hash => instance.changes) if instance.deployment.events.lifetime.last{|e|e.status_code == :all_running}
> +          if instance.deployment.events.lifetime.last{|e|e.status_code == :all_running}
> +            event4 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
> +                               :status_code => "some_stopped", :summary => "Some instances are stopped",
> +                               :change_hash => instance.changes)
> +            event4.save!
> +            Notify.notice("notifications.deployments.some_instances_stopped",
> +                          :source => instance.deployment,
> +                          :user => instance.deployment.owner)
> +          end
>          else
>            event4 = Event.new(:source => instance.deployment, :event_time => DateTime.now,
>                               :status_code => "all_stopped", :summary => "All instances are stopped",
>                               :change_hash => instance.changes)
> +          event4.save!
> +          Notify.notice("notifications.deployments.all_instances_stopped",
> +                        :source => instance.deployment,
> +                        :user => instance.deployment.owner)
>          end
> -        event4.save! if event4
>        elsif instance.state == Instance::STATE_VANISHED
>          Event.create!(
>            :source => instance, :event_time => Time.now,
>            :status_code => 'vanished', :summary => "Instance disappeared from cloud provider"
>          )
> +        Notify.error('notifications.instances.vanished',
> +                     :source => instance,
> +                     :user => instance.owner)

I like that this new approach allows the notification text to be
translated.

I wonder if, long-term, we can either (a) do the same for Events, or (b)
get rid of Events, and merge them with Notifications. (Either of these
it outside the scope of your patch, of course.)

>        end
>        instance.deployment.update_state(instance) if instance.deployment
>  
> diff --git a/src/app/models/notification.rb b/src/app/models/notification.rb
> new file mode 100644
> index 0000000..85d3039
> --- /dev/null
> +++ b/src/app/models/notification.rb
> @@ -0,0 +1,68 @@
> +#
> +# Copyright 2011 Red Hat, Inc.
> +#
> +# This software is licensed to you under the GNU General Public
> +# License as published by the Free Software Foundation; either version
> +# 2 of the License (GPLv2) or (at your option) any later version.
> +# There is NO WARRANTY for this software, express or implied,
> +# including the implied warranties of MERCHANTABILITY,
> +# NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should
> +# have received a copy of GPLv2 along with this software; if not, see
> +# http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.

I'm assuming this code is shared with Katello, hence the different
copyright and different license.

However, I think this is a problem. GPL code cannot be used in ASL
projects:
http://www.apache.org/licenses/GPL-compatibility.html

What a headache! I wonder if we can work with Katello to pull this
notification code out into a more compatible license format, so that we
can both use it. (Or we and Katello could just converge on a single
license, since we're sister projects.) I suppose we could also talk
about trying to dual-license it, but that sounds like it's just making
the problem worse.

Incidentally, as we work with Katello more and more, I imagine this is
going to increasingly come up and bite us.

> +
> +class Notification < ActiveRecord::Base
> +
> +  has_many :user_notifications
> +  has_many :users, :through => :user_notifications
> +  belongs_to :source, :polymorphic => true, :conditions => '1=1'
> +
> +  LEVELS = [:notice, :warning, :success, :error]
> +
> +  validates_inclusion_of :level, :in => LEVELS + LEVELS.collect{|type| type.to_s}
> +  validates_presence_of :source_id
> +  validates_presence_of :source_type
> +  validates_presence_of :key
> +  validates_length_of :user_notifications, :minimum => 1
> +
> +  before_validation :set_default_notice_level
> +
> +  scope :readable, lambda { |user| joins(:users).where('users.id' => user) }
> +  scope :read, lambda { viewed true }
> +  scope :unread, lambda { viewed false }
> +
> +  def source
> +    case source_type
> +      when "Deployment" then Deployment.with_deleted.find(source_id)
> +      when "Instance" then Instance.with_deleted.find(source_id)
> +      else super
> +    end
> +  end
> +
> +  def to_s
> +    "#{level}: #{key} #{source.name}"
> +  end
> +
> +  def check_permissions operation
> +    logger.debug "CHECKING #{operation}"
> +    # anybody can create notices
> +    return true if operation == :create
> +    if operation == :update or operation == :destroy
> +      # TODO: who is a real owner of a notice?
> +    end
> +    false
> +  end
> +
> +  def message
> +    if source.nil?
> +      I18n.t("notifications.errors.missing_source")
> +    else
> +      I18n.t(key, :name => source.name)
> +    end
> +  end
> +
> +  private
> +
> +  def set_default_notice_level
> +    self.level ||= TYPES.first
> +  end
> +end
> diff --git a/src/app/models/user_notification.rb b/src/app/models/user_notification.rb
> new file mode 100644
> index 0000000..80f9e74
> --- /dev/null
> +++ b/src/app/models/user_notification.rb
> @@ -0,0 +1,32 @@
> +#
> +# Copyright 2011 Red Hat, Inc.
> +#
> +# This software is licensed to you under the GNU General Public
> +# License as published by the Free Software Foundation; either version
> +# 2 of the License (GPLv2) or (at your option) any later version.
> +# There is NO WARRANTY for this software, express or implied,
> +# including the implied warranties of MERCHANTABILITY,
> +# NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should
> +# have received a copy of GPLv2 along with this software; if not, see
> +# http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> +
> +class UserNotification < ActiveRecord::Base
> +
> +  belongs_to :user
> +  belongs_to :notification
> +
> +  def check_permissions operation
> +    # anybody can create user_notice relationships
> +    return true if operation == :create
> +    # only notice owner can update or destroy
> +    if operation == :update or operation == :destroy
> +      return true if user.id and User.current and user.id == User.current.id
> +    end
> +    false
> +  end
> +
> +  def read!
> +    update_attributes! :viewed => true
> +  end
> +
> +end
> diff --git a/src/app/views/notifications/index.html.haml b/src/app/views/notifications/index.html.haml
> new file mode 100644
> index 0000000..5133e9c
> --- /dev/null
> +++ b/src/app/views/notifications/index.html.haml
> @@ -0,0 +1,20 @@
> += render :partial => 'layouts/nav_history'
> +
> +%header.page-header
> +  %h1.section-index= t('notifications.index.title')
> +
> +%section.content-section.toggle-view.notifications
> +  - content_for :filter_controls do
> +    %li
> +      = label_tag "source_type", t('filter_table.viewing')
> +      = hidden_field_tag :current_path, request.fullpath
> +      = restful_submit_tag t("filter_table.apply_filters"), "index", filter_notifications_path, 'POST', :class => 'button', :id => 'apply_notifications_filter'
> +      %span.label.badge.dark= @notifications.count
> +  = filter_table(@header, @paginated_notifications) do |notification|
> +    - source = notification.source
> +    %tr{:class => cycle('nostripe','stripe')}
> +      %td= notification.created_at.strftime("%d-%b-%Y %H:%M:%S")
> +      %td= notification.level
> +      %td= source.nil? ? t('notifications.index.not_available') : link_to(source.class.to_s + " (" + source.name + ")", polymorphic_path(source))
> +      %td= notification.message
> +      %td= notification.details
> diff --git a/src/config/application.rb b/src/config/application.rb
> index 88d6d1d..88606a5 100644
> --- a/src/config/application.rb
> +++ b/src/config/application.rb
> @@ -38,6 +38,7 @@ module Conductor
>  
>      # Custom directories with classes and modules you want to be autoloadable.
>      # config.autoload_paths += %W(#{config.root}/extras)
> +    config.autoload_paths += %W(#{Rails.root}/lib)
>  
>      # Only load the plugins named here, in the order given (default is alphabetical).
>      # :all can be used as a placeholder for all plugins not explicitly named.
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index e052f91..b6ac407 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -12,6 +12,53 @@ en:
>      no_results: "No matching results."
>    activity:
>      activity: Activity
> +  notifications:
> +    index:
> +      title: Notifications
> +      notification_time: Time
> +      source: Source
> +      message: Message
> +      details: Details
> +      not_available: N/A
> +      level: Level
> +    deployments:
> +      launch_attempt: Attempting to launch deployment %{name}
> +      launch_failed: Failed to launch deployment %{name}
> +      first_instance_running: First instance running for deployment %{name}
> +      all_instances_running: All instances running for deployment %{name}
> +      some_instances_stopped: Some instances stopped for deployment %{name}
> +      all_instances_stopped: All instances stopped for deployment %{name}
> +      new_state_change: Deployment %{name} created
> +      pending_state_change: Deployment %{name} pending
> +      running_state_change: Deployment %{name} running
> +      shutting_down_state_change: Deployment %{name} shutting down
> +      stopped_state_change: Deployment %{name} stopped
> +      failed_state_change: Deployment %{name} failed
> +      incomplete_state_change: Deployment %{name} incomplete
> +      rollback_in_progress: Deployment %{name} rollback in progress
> +      rollback_complete: Deployment %{name} rollback complete
> +      rollback_failed: Deployment %{name} rollback failed
> +    instances:
> +      created: Instance %{name} created
> +      new_state_change: Instance %{name} state changed to new
> +      pending_state_change: Instance %{name} state changed to pending
> +      running_state_change: Instance %{name} state changed to running
> +      shutting_down_state_change: Instance %{name} state changed to shutting down
> +      stopped_state_change: Instance %{name} state changed to stopped
> +      stopping_state_change: Instance %{name} state changed to stopping
> +      create_failed_state_change: Instance %{name} state changed to create failed
> +      error_state_change: Instance %{name} state changed to error
> +      vanished_state_change: Instance %{name} state changed to new
> +      create_action_queued: Create action queued for instance %{name}
> +      start_action_queued: Start action queued for instance %{name}
> +      stop_action_queued: Stop action queued for instance %{name}
> +      reboot_action_queued: Reboot action queued for instance %{name}
> +      destroy_action_queued: Destroy action queued for instance %{name}
> +      stop_failed: Failed to stop instance %{name}
> +      forced_stop: Instance %{name} is not accessible, state changed to stopped
> +      vanished: Instance %{name} vanished
> +    errors:
> +      missing_source: Notification source is missing
>    logs:
>      options:
>        default_users: All Users
> @@ -1580,3 +1627,4 @@ en:
>      deployables: Deployables
>      deployments: Deployments
>      logs: Logs
> +    notifications: Notifications
> diff --git a/src/config/routes.rb b/src/config/routes.rb
> index 1f57578..fed1957 100644
> --- a/src/config/routes.rb
> +++ b/src/config/routes.rb
> @@ -189,6 +189,12 @@ Conductor::Application.routes.draw do
>      end
>    end
>  
> +  resources :notifications do
> +    collection do
> +      post 'filter'
> +    end
> +  end
> +
>    resources :config_servers do
>      member do
>        get 'test'
> diff --git a/src/db/migrate/20120920130441_create_notifications.rb b/src/db/migrate/20120920130441_create_notifications.rb
> new file mode 100644
> index 0000000..ea5fd91
> --- /dev/null
> +++ b/src/db/migrate/20120920130441_create_notifications.rb
> @@ -0,0 +1,28 @@
> +class CreateNotifications < ActiveRecord::Migration
> +  def self.up
> +    # These are notifications
> +    create_table :notifications do |t|
> +      t.integer    :source_id, :null => false
> +      t.string     :source_type, :null => false
> +      t.string     :key, :null => false
> +      t.text       :details
> +      t.string     :level, :null => false
> +      t.timestamps
> +    end
> +    create_table :user_notifications do |t|
> +      t.references :user
> +      t.references :notification
> +      t.boolean    :viewed, :null => false, :default => false
> +    end
> +    add_index  :user_notifications, :user_id
> +    add_index  :user_notifications, :notification_id
> +  end
> +
> +  def self.down
> +    remove_index :user_notifications, :column=>:user_id
> +    remove_index :user_notifications, :column=>:notification_id
> +    drop_table :user_notifications
> +    drop_table :notifications
> +  end
> +
> +end
> diff --git a/src/lib/notifications/notifier.rb b/src/lib/notifications/notifier.rb
> new file mode 100644
> index 0000000..baab263
> --- /dev/null
> +++ b/src/lib/notifications/notifier.rb
> @@ -0,0 +1,105 @@
> +#
> +# Copyright 2011 Red Hat, Inc.
> +#
> +# This software is licensed to you under the GNU General Public
> +# License as published by the Free Software Foundation; either version
> +# 2 of the License (GPLv2) or (at your option) any later version.
> +# There is NO WARRANTY for this software, express or implied,
> +# including the implied warranties of MERCHANTABILITY,
> +# NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should
> +# have received a copy of GPLv2 along with this software; if not, see
> +# http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> +
> +module Notifications
> +
> +  class Notifier
> +    include Rails.application.routes.url_helpers
> +
> +    attr_reader :controller, :default_options
> +
> +    def initialize(controller = nil, options = { })
> +      @controller      = controller
> +      @default_options = { :level        => :success,
> +                           :details      => nil,
> +                           :user         => nil,
> +                           :source       => nil}.merge(options)
> +    end
> +
> +    # generates success-notice
> +    def success(key, options = { })
> +      notification key, options

This is fairly nitpicky, but it's customary to use parenthesis if there
is more than one argument. So this should really be:

  notification(key, options)

(This is repeated in most methods below). In terms of syntax there is no
difference, but this one method is weirdly inconsistent with the rest of
our project, and consistency is nice. :)

> +    end
> +
> +    # generates message-notice
> +    def notice(key, options = { })
> +      options = { :level => :notice }.merge options
> +      notification key, options
> +    end
> +
> +    # generates warning-notice, not persisted by default
> +    def warning(key, options = { })
> +      options = { :level => :warning }.merge options
> +      notification key, options
> +    end
> +
> +    # generates error-notice
> +    def error(key, options = { })
> +      options = { :level => :error }.merge options
> +      notification key, options
> +    end
> +
> +    # generates error-notice form exception, optionally adding a note
> +    # @example
> +    #   exception(an_exception)
> +    #   exception("some note", an_exception)
> +    def exception(key, *args)
> +      options   = args.extract_options!
> +      exception = args.pop
> +
> +      options = { :level   => :error,
> +        :details => exception.backtrace.join("\n") }.merge options
> +
> +      notification key, options
> +    end
> +
> +    protected
> +
> +    # Generate a notification:
> +    #
> +    # key:                  The notification dictionary key
> +    # options:              Optional hash containing various optional parameters. This includes:
> +    #
> +    #   level:               The type of notice to be generated. Supported values include:
> +    #                        :notice, :success (Default), :warning, :error
> +    #
> +    #   details:             String containing additional details. This would typically be to store
> +    #                        information such as a stack trace that is in addition to the notice text.
> +    #
> +    #   user:                the user to send the notifcation to.  If not set, User.current is used
> +    #
> +    def notification(key, options = { })
> +      options = process_options options
> +
> +      Notification.create! :key          => key,
> +                           :details      => options[:details],
> +                           :level        => options[:level],
> +                           :source       => options[:source],
> +                           :user_notifications =>
> +        [UserNotification.new(:user => options[:user], :viewed => false)]

Not using parenthesis for _this_ makes me sad. ;)

> +    end
> +
> +    LEVELS          = [:notice, :success, :warning, :error]
> +
> +    def process_options(options)
> +      options        = default_options.merge options
> +      options[:user] ||= User.current
> +
> +      options.assert_valid_keys(*default_options.keys)
> +
> +      raise ArgumentError, "unknown notification level #{options[:level].inspect}" unless LEVELS.include? options[:level]
> +
> +      return options
> +    end
> +  end
> +end
> +
> diff --git a/src/lib/notify.rb b/src/lib/notify.rb
> new file mode 100644
> index 0000000..504de9e
> --- /dev/null
> +++ b/src/lib/notify.rb
> @@ -0,0 +1,13 @@
> +#
> +# Copyright 2011 Red Hat, Inc.
> +#
> +# This software is licensed to you under the GNU General Public
> +# License as published by the Free Software Foundation; either version
> +# 2 of the License (GPLv2) or (at your option) any later version.
> +# There is NO WARRANTY for this software, express or implied,
> +# including the implied warranties of MERCHANTABILITY,
> +# NON-INFRINGEMENT, or FITNESS FOR A PARTICULAR PURPOSE. You should
> +# have received a copy of GPLv2 along with this software; if not, see
> +# http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> +
> +Notify = Notifications::Notifier.new
> -- 
> 1.7.6.5
>

Unfortunately, I have to NACK this because of the license issues -- it
adds GPLv2 code from Katello, which is seemingly incompatible with ASL
(see http://www.apache.org/licenses/GPL-compatibility.html ). That's a
bit of a headache since we're working on complementary projects, but
hopefully we can work something out with them.

I also have some inline nits unrelated to the license problem, though
the general implementation is good.

-- Matt



More information about the aeolus-devel mailing list