[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,23 +12,28 @@ 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
   def create
     return render_bad_request('attach alert to kpi', 'no alert specified') unless params.require(:alert)
-    return render_not_found('kpi') unless MnoEnterprise::Kpi.find_one(params.require(:kpi_id))
+    return render_not_found('kpi') unless kpi = MnoEnterprise::Kpi.find_one(params.require(:kpi_id))
 
     # TODO: Manage authorization
     #authorize! :manage_alert, kpi_alert
 
-    @alert = MnoEnterprise::Alert.create(alert_params.merge(recipient_ids: [current_user.id]))
-    if @alert.errors.empty?
+    @alert = MnoEnterprise::Alert.create(alert_params)
+    errors = @alert.errors
+    if errors.empty?
+      result = @alert.update_recipients!([current_user.id])
+      errors = result.errors
+    end
+    if errors.empty?
+      @alert = @alert.load_required(:recipients)
       render 'show'
     else
-      render_bad_request('attach alert to kpi', @alert.errors)
+      render_bad_request('attach alert to kpi', errors.full_messages)
     end
   end
 
@@ -43,6 +48,7 @@ def update
     # authorize! :manage_alert, alert
 
     if alert.update_attributes(attributes)
+      @alert = @alert.load_required(:recipients)
       render 'show'
     else
       render_bad_request('update alert', alert.errors)
@@ -64,7 +70,6 @@ def destroy
     end
   end
 
-
   private
 
     def alert

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

@@ -26,8 +26,8 @@ def index
 
     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?
+        # 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?
     rescue => e
       return render json: { message: "Unable to retrieve kpis from Impac API | Error #{e}" }
     end
@@ -39,11 +39,11 @@ 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
+             .flatten
 
     render json: { kpis: kpis }
   end
@@ -65,12 +65,12 @@ def create
     if @kpi.errors.empty?
       # 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'
     else
       msg = kpi.errors.full_messages.join(', ') || 'unable to create KPI.'
@@ -91,19 +91,19 @@ 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])
+      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.
+      # 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)
+      # 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')
     if kpi.errors.empty?
       render 'show'
     else
@@ -116,11 +116,8 @@ def update
   #   -> DELETE /api/mnoe/v1/kpis/:id
   def destroy
     return render_not_found('kpi') unless kpi.present?
-
     authorize! :manage_kpi, kpi
-
     MnoEnterprise::EventLogger.info('kpi_delete', current_user.id, 'KPI Deletion', kpi)
-
     kpi.destroy
     head status: :ok
   end
@@ -130,45 +127,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: []) }
@@ -101,7 +101,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 }