Merge pull request #535 from x4d3/fix/alert

[MNO-695] Api v2 Dashboard missing dependency for KPI Alert Recipients

api/app/views/mno_enterprise/jpi/v1/impac/alerts/_alert.json.jbuilder

@@ -1,7 +1,6 @@
 json.ignore_nil!
-json.extract! alert, :id, :title, :webhook, :service, :sent
+json.extract! alert, :id, :title, :webhook, :service, :sent, :kpi_id
 json.metadata alert.settings
-json.kpi_id alert.kpi_id
 json.recipients alert.recipients.map do |recipient|
   json.extract! recipient, :id, :email
 end

api/lib/mno_enterprise/concerns/controllers/jpi/v1/impac/alerts_controller.rb

@@ -12,8 +12,7 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::Impac::AlertsController
 
   # GET /jpi/v1/impac/alerts
   def index
-    u = current_user.load_required(:alerts)
-    @alerts = u.alerts
+    @alerts = MnoEnterprise::Alert.includes(:recipients).where('recipient.id' => current_user.id)
   end
 
   # POST /jpi/v1/impac/kpis/:kpi_id/alerts
@@ -23,8 +22,9 @@ def create
 
     # TODO: Manage authorization
     #authorize! :manage_alert, kpi_alert
-
-    @alert = MnoEnterprise::Alert.create!(alert_params.merge(recipient_ids: [current_user.id]))
+    @alert = MnoEnterprise::Alert.create!(alert_params)
+    @alert.update_recipients!([current_user.id])
+    @alert = @alert.load_required(:recipients)
     render 'show'
   end
 

api/lib/mno_enterprise/concerns/controllers/jpi/v1/impac/dashboards_controller.rb

@@ -10,7 +10,7 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::Impac::DashboardsControlle
     respond_to :json
   end
 
-  DASHBOARD_DEPENDENCIES = [:widgets, :'widgets.kpis', :kpis, :'kpis.alerts']
+  DASHBOARD_DEPENDENCIES = [:widgets, :'widgets.kpis', :kpis, :'kpis.alerts', :'kpis.alerts.recipients']
 
   #==================================================================
   # Instance methods
@@ -29,9 +29,6 @@ def show
   # POST /mnoe/jpi/v1/impac/dashboards
   #   -> POST /api/mnoe/v1/users/1/dashboards
   def create
-    # TODO: dashboards.build breaks as dashboard.organization_ids returns nil, instead of an
-    #       empty array. (see MnoEnterprise::Impac::Dashboard #organizations)
-    # @dashboard = dashboards.build(dashboard_create_params)
     # TODO: enable authorization
     # authorize! :manage_dashboard, @dashboard
     # if @dashboard.save

api/lib/mno_enterprise/concerns/controllers/jpi/v1/impac/kpis_controller.rb

@@ -25,9 +25,9 @@ def index
     auth = { username: MnoEnterprise.tenant_id, password: MnoEnterprise.tenant_key }
 
     begin
-      response = MnoEnterprise::ImpacClient.send_get('/api/v2/kpis', attrs, basic_auth: auth)
       # TODO check there was no error, something like
       # return render json: { message: "Unable to retrieve kpis from Impac API | Error #{response.code}" } unless response.success?
+      response = MnoEnterprise::ImpacClient.send_get('/api/v2/kpis', attrs, basic_auth: auth)
     rescue => e
       return render json: { message: "Unable to retrieve kpis from Impac API | Error #{e}" }
     end
@@ -39,11 +39,10 @@ def index
         kpi.merge(
           name: "#{kpi[:name]} #{watchable.capitalize unless kpi[:name].downcase.index(watchable)}".strip,
           watchables: [watchable],
-          target_placeholders: {watchable => kpi[:target_placeholders][watchable]},
+          target_placeholders: { watchable => kpi[:target_placeholders][watchable] },
         )
       end
-    end
-    .flatten
+    end.flatten
 
     render json: { kpis: kpis }
   end
@@ -59,16 +58,15 @@ def create
       return render_not_found('dashboard') if dashboard.blank?
       authorize! :manage_dashboard, dashboard
     end
-    # TODO: nest alert in as a param, with the current user as a recipient.
     @kpi = MnoEnterprise::Kpi.create!(kpi_create_params)
     # Creates a default alert for kpis created with targets defined.
     if kpi.targets.present?
-      MnoEnterprise::Alert.create({service: 'inapp', kpi_id: kpi.id, recipient_ids: [current_user.id]})
+      MnoEnterprise::Alert.create_with_recipients!({ service: 'inapp', kpi_id: kpi.id }, [current_user.id])
       # TODO: should widget KPIs create an email alert automatically?
-      MnoEnterprise::Alert.create({service: 'email', kpi_id: kpi.id, recipient_ids: [current_user.id]}) if widget.present?
+      MnoEnterprise::Alert.create_with_recipients!({ service: 'email', kpi_id: kpi.id }, [current_user.id]) if widget.present?
       # TODO: reload is adding the recipients to the kpi alerts (making another request).
     end
-    @kpi = kpi.load_required(:alerts)
+    @kpi = kpi.load_required(:alerts, :'alerts.recipients')
     render 'show'
   end
 
@@ -85,17 +83,17 @@ def update
     # --
     # Creates an in-app alert if target is set for the first time (in-app alerts should be activated by default)
     if kpi.targets.blank? && params[:targets].present?
-      MnoEnterprise::Alert.create!(service: 'inapp', kpi_id: kpi.id, recipient_ids: [current_user.id])
-    # If targets have changed, reset all the alerts 'sent' status to false.
+      MnoEnterprise::Alert.create_with_recipients!({ service: 'inapp', kpi_id: kpi.id }, [current_user.id])
+      # If targets have changed, reset all the alerts 'sent' status to false.
     elsif kpi.targets && params[:targets].present? && params[:targets] != kpi.targets
-      kpi.alerts.each { |alert| alert.update_attributes!(sent: false) }
-    # Removes all the alerts if the targets are removed (kpi has no targets set,
-    # and params contains no targets to be set)
+      kpi.alerts.each { |alert| alert.update_attributes(sent: false) }
+      # Removes all the alerts if the targets are removed (kpi has no targets set,
+      # and params contains no targets to be set)
     elsif params[:targets].blank? && kpi.targets.blank?
       kpi.alerts.each(&:destroy!)
     end
     kpi.update_attributes!(kpi_update_params)
-    @kpi = kpi.load_required(:dashboard, :alerts)
+    @kpi = kpi.load_required(:dashboard, :alerts, :'alerts.recipients')
     render 'show'
   end
 
@@ -114,45 +112,44 @@ def destroy
   #=================================================
   private
 
-    def dashboard
-      @dashboard ||= MnoEnterprise::Dashboard.find_one(params.require(:dashboard_id))
-    end
+  def dashboard
+    @dashboard ||= MnoEnterprise::Dashboard.find_one(params.require(:dashboard_id))
+  end
 
-    def widget
-      widget_id = params.require(:kpi)[:widget_id]
-      @widget ||= (widget_id.present? && MnoEnterprise::Widget.find_one(widget_id.to_i))
-    end
+  def widget
+    widget_id = params.require(:kpi)[:widget_id]
+    @widget ||= (widget_id.present? && MnoEnterprise::Widget.find_one(widget_id.to_i))
+  end
 
-    def kpi
-      @kpi ||= MnoEnterprise::Kpi.find_one(params[:id], :dashboard, :widget, :alerts)
-    end
+  def kpi
+    @kpi ||= MnoEnterprise::Kpi.find_one(params[:id], :dashboard, :widget, :alerts, :'alerts.recipients')
+  end
 
-   def kpi_create_params
-      whitelist = [:widget_id, :endpoint, :source, :element_watched, {extra_watchables: []}]
-      create_params = extract_params(whitelist)
-      #either it is a widget kpi or a dashboard kpi
-      if create_params[:widget_id]
-        create_params
-      else
-        create_params.merge(dashboard_id: params[:dashboard_id])
-      end
+  def kpi_create_params
+    whitelist = [:widget_id, :endpoint, :source, :element_watched, { extra_watchables: [] }]
+    create_params = extract_params(whitelist)
+    #either it is a widget kpi or a dashboard kpi
+    if create_params[:widget_id]
+      create_params
+    else
+      create_params.merge(dashboard_id: params[:dashboard_id])
     end
+  end
 
-    def kpi_update_params
-      whitelist = [:name, :element_watched, {extra_watchables: []}]
-      extract_params(whitelist)
-    end
+  def kpi_update_params
+    whitelist = [:name, :element_watched, { extra_watchables: [] }]
+    extract_params(whitelist)
+  end
 
-    def extract_params(whitelist)
-      (p = params).require(:kpi).permit(*whitelist).tap do |whitelisted|
-        whitelisted[:settings] = p[:kpi][:metadata] || {}
-        # TODO: strong params for targets & extra_params attributes (keys will depend on the kpi).
-        whitelisted[:targets] = p[:kpi][:targets] unless p[:kpi][:targets].blank?
-        whitelisted[:extra_params] = p[:kpi][:extra_params] unless p[:kpi][:extra_params].blank?
-      end
-      .except(:metadata)
-    end
+  def extract_params(whitelist)
+    (p = params).require(:kpi).permit(*whitelist).tap do |whitelisted|
+      whitelisted[:settings] = p[:kpi][:metadata] || {}
+      # TODO: strong params for targets & extra_params attributes (keys will depend on the kpi).
+      whitelisted[:targets] = p[:kpi][:targets] unless p[:kpi][:targets].blank?
+      whitelisted[:extra_params] = p[:kpi][:extra_params] unless p[:kpi][:extra_params].blank?
+    end.except(:metadata)
+  end
 
-    alias :find_valid_kpi  :kpi
+  alias :find_valid_kpi :kpi
 
 end

api/spec/controllers/mno_enterprise/jpi/v1/impac/alerts_controller_spec.rb

@@ -12,49 +12,55 @@ module MnoEnterprise
     before { allow(ability).to receive(:can?).with(any_args).and_return(true) }
 
     # Stub user and user call
-    let!(:user) { build(:user, alerts: [alert]) }
+    let!(:user) { build(:user) }
     let!(:current_user_stub) { stub_user(user) }
 
-
     before { sign_in user }
 
     let(:kpi) { build(:impac_kpi) }
-    let(:alert) { build(:impac_alert, kpi: kpi) }
+    let(:alert) { build(:impac_alert, kpi: kpi, recipients: [build(:user)]) }
     let(:alert_hash) { serialize_type(alert).except(:kpi) }
 
     describe 'GET #index' do
-      before { stub_api_v2(:get, "/users/#{user.id}", user, %i(alerts)) }
+      before { stub_api_v2(:get, "/alerts", alert, [:recipients], { filter: { 'recipient.id': user.id } }) }
       subject { get :index }
-      # TODO: Add and test authorization
+      it { is_expected.to be_success }
     end
 
     describe 'POST #create' do
 
-      before { stub_api_v2(:post, '/alerts', alert) }
-      before { stub_api_v2(:get, "/kpis/#{kpi.id}", kpi) }
+      before do
+        stub_api_v2(:post, '/alerts', alert)
+        stub_api_v2(:get, "/kpis/#{kpi.id}", kpi)
+        stub_api_v2(:patch, "/alerts/#{alert.id}/update_recipients", alert)
+        stub_api_v2(:get, "/alerts/#{alert.id}", alert, [:recipients])
+      end
+
       subject { post :create, kpi_id: kpi.id, alert: alert_hash }
 
       # TODO: Add authorization
       # it_behaves_like 'jpi v1 authorizable action'
 
-      it { subject; expect(response.code).to eq('200') }
+      it { is_expected.to be_success }
     end
 
     describe 'PUT #update' do
-      let(:update_alert_hash) { {title: 'test', webhook: 'test', sent: true, forbidden: 'test', } }
-      let(:updated_alert) { build(:impac_alert, kpi_id: kpi.id, title: 'test', webhook: 'test', sent: true) }
+      before do
+        stub_api_v2(:get, "/alerts/#{alert.id}", alert)
+        stub_api_v2(:patch, "/alerts/#{alert.id}", updated_alert)
+        stub_api_v2(:get, "/alerts/#{alert.id}", alert, [:recipients])
+      end
 
-      before { stub_api_v2(:get, "/alerts/#{alert.id}", alert) }
-      before { stub_api_v2(:patch, "/alerts/#{alert.id}", updated_alert) }
+      let(:update_alert_hash) { { title: 'test', webhook: 'test', sent: true, forbidden: 'test', } }
+      let(:updated_alert) { build(:impac_alert, id: alert.id, kpi_id: kpi.id, title: 'test', webhook: 'test', sent: true) }
 
       subject { put :update, id: alert.id, alert: update_alert_hash }
 
       # TODO: Add and test authorization
       # it_behaves_like "jpi v1 authorizable action"
 
       # TODO: Test that rendering is equal to update_alert_hash
-
-      it { subject; expect(response.code).to eq('200') }
+      it { is_expected.to be_success }
     end
 
     describe 'DELETE #destroy' do
@@ -65,8 +71,8 @@ module MnoEnterprise
       # TODO: Add and test authorization
       # it_behaves_like "jpi v1 authorizable action"
 
-      it { subject; expect(response.code).to eq('200') }
-      it { subject; expect(JSON.parse(response.body)).to eq({'deleted' => {'service' => alert.service}}) }
+      it { is_expected.to be_success }
+      it { subject; expect(JSON.parse(response.body)).to eq({ 'deleted' => { 'service' => alert.service } }) }
     end
   end
 end

api/spec/controllers/mno_enterprise/jpi/v1/impac/dashboards_controller_spec.rb

@@ -8,7 +8,7 @@ module MnoEnterprise
     before { request.env["HTTP_ACCEPT"] = 'application/json' }
     before { Rails.cache.clear }
 
-    let(:dashboard_dependencies) { [:widgets, :'widgets.kpis', :kpis, :'kpis.alerts'] }
+    let(:dashboard_dependencies) { [:widgets, :'widgets.kpis', :kpis, :'kpis.alerts', :'kpis.alerts.recipients'] }
 
     # Stub user and user call
     let(:org) { build(:organization, users: [], orga_relations: []) }
@@ -104,7 +104,7 @@ def hash_for_kpi(kpi)
 
     describe 'GET #show' do
       before do
-        stub_api_v2(:get, "/dashboards/#{dashboard.id}", dashboard, [:widgets, :'widgets.kpis', :kpis, :'kpis.alerts'], { filter: { owner_id: user.id } })
+        stub_api_v2(:get, "/dashboards/#{dashboard.id}", dashboard, dashboard_dependencies, {filter: {owner_id: user.id}})
       end
 
       subject { get :show, id: dashboard.id }

api/spec/controllers/mno_enterprise/jpi/v1/impac/kpis_controller_spec.rb

@@ -93,12 +93,16 @@ module MnoEnterprise
       context "a dashboard KPI" do
         subject { post :create, dashboard_id: dashboard.id, kpi: kpi_hash }
         let(:created_kpi) { build(:impac_kpi) }
+        let(:alert) { build(:impac_alert, kpi: created_kpi, recipients: [build(:user)]) }
+
         before do
           stub_api_v2(:get, "/dashboards/#{dashboard.id}", dashboard)
           stub_api_v2(:post, "/kpis", created_kpi)
-          stub_api_v2(:post, "/alerts")
+          stub_api_v2(:post, "/alerts", alert)
+          stub_api_v2(:patch, "/alerts/#{alert.id}/update_recipients")
+
           # kpi reload
-          stub_api_v2(:get, "/kpis/#{created_kpi.id}", [created_kpi], [:alerts])
+          stub_api_v2(:get, "/kpis/#{created_kpi.id}", [created_kpi], [:alerts, :'alerts.recipients'])
         end
 
         it_behaves_like 'jpi v1 authorizable action'
@@ -113,7 +117,7 @@ module MnoEnterprise
           let(:kpi_targets) { {evolution: [{max: "20"}]} }
 
           it "creates a kpi inapp alert" do
-            expect(MnoEnterprise::Alert).to receive(:create).once.with(hash_including(service: 'inapp'))
+            expect(MnoEnterprise::Alert).to receive(:create_with_recipients!).once.with(hash_including(service: 'inapp'), array_including(user.id))
             subject
             expect(response).to have_http_status(:ok)
           end
@@ -123,14 +127,17 @@ module MnoEnterprise
       context "a widget KPI" do
         let(:widget) { build(:impac_widget) }
         let(:created_kpi) { build(:impac_kpi) }
+        let(:alert) { build(:impac_alert, kpi: created_kpi, recipients: [build(:user)]) }
+
         subject { post :create, dashboard_id: dashboard.id, kpi: kpi_hash.merge(widget_id: widget.id) }
 
         before do
           stub_api_v2(:get, "/widgets/#{widget.id}", widget)
           stub_api_v2(:post, "/kpis", created_kpi)
-          stub_api_v2(:post, "/alerts")
+          stub_api_v2(:post, "/alerts", alert)
+          stub_api_v2(:patch, "/alerts/#{alert.id}/update_recipients")
           # kpi reload
-          stub_api_v2(:get, "/kpis/#{created_kpi.id}", [created_kpi], [:alerts])
+          stub_api_v2(:get, "/kpis/#{created_kpi.id}", [created_kpi], [:alerts, :'alerts.recipients'])
         end
 
         it_behaves_like 'jpi v1 authorizable action'
@@ -147,8 +154,8 @@ module MnoEnterprise
           let(:kpi_targets) { { evolution: [{max: "20"}] } }
 
           it "creates kpi alerts" do
-            expect(MnoEnterprise::Alert).to receive(:create).once.with(hash_including(service: 'inapp'))
-            expect(MnoEnterprise::Alert).to receive(:create).once.with(hash_including(service: 'email'))
+            expect(MnoEnterprise::Alert).to receive(:create_with_recipients!).once.with(hash_including(service: 'inapp'), array_including(user.id))
+            expect(MnoEnterprise::Alert).to receive(:create_with_recipients!).once.with(hash_including(service: 'email'), array_including(user.id))
 
             subject
             expect(response).to have_http_status(:ok)
@@ -168,9 +175,11 @@ module MnoEnterprise
         context 'target set for the first time' do
           let(:kpi_targets) { nil }
           let(:params) { {targets: {evolution: [{max: '20'}]}} }
+          let(:alert) { build(:impac_alert) }
 
           before do
-            stub_api_v2(:post, '/alerts')
+            stub_api_v2(:post, '/alerts', alert)
+            stub_api_v2(:patch, "/alerts/#{alert.id}/update_recipients", alert)
           end
 
           it 'creates an alert' do
@@ -236,9 +245,9 @@ module MnoEnterprise
       before {
         kpi.alerts << alert
         updated_kpi.alerts << alert
-        stub_api_v2(:get, "/kpis/#{kpi.id}", kpi, %i(dashboard widget alerts))
+        stub_api_v2(:get, "/kpis/#{kpi.id}", kpi, %i(dashboard widget alerts alerts.recipients))
         # reload of the kpi
-        stub_api_v2(:get, "/kpis/#{kpi.id}", updated_kpi, %i(dashboard alerts))
+        stub_api_v2(:get, "/kpis/#{kpi.id}", updated_kpi, %i(dashboard alerts alerts.recipients))
         stub_api_v2(:patch, "/kpis/#{kpi.id}", updated_kpi)
         stub_api_v2(:patch, "/alerts/#{alert.id}", alert)
       }
@@ -261,7 +270,7 @@ module MnoEnterprise
       subject { delete :destroy, id: kpi.id }
 
       before {
-        stub_api_v2(:get, "/kpis/#{kpi.id}", kpi, %i(dashboard widget alerts))
+        stub_api_v2(:get, "/kpis/#{kpi.id}", kpi, %i(dashboard widget alerts alerts.recipients))
         stub_api_v2(:delete, "/kpis/#{kpi.id}")
       }