From db10db4c7932417ad70fa70f272b1bf966dffc57 Mon Sep 17 00:00:00 2001
From: Zach Wolfenbarger <zach.wolfenbarger@gmail.com>
Date: Wed, 5 Jun 2024 15:45:56 -0500
Subject: [PATCH] Remove references to aggregation attribute on Workflows
 (#4339)

* Ignore aggregation column on workflows

* Remove everywhere else

* Hound

* Hound cleanup

* Short circuit scopes & specs for private aggs

* Deprecate AggregationsController

* Spec removal
---
 .../api/v1/aggregations_controller.rb         |  21 +--
 app/models/workflow.rb                        |   4 +-
 app/policies/aggregation_policy.rb            |   9 +-
 app/schemas/workflow_create_schema.rb         |   4 -
 app/schemas/workflow_update_schema.rb         |   4 -
 app/serializers/workflow_serializer.rb        |   2 +-
 lib/formatter/csv/workflow.rb                 |   8 +-
 .../api/v1/aggregations_controller_spec.rb    | 169 ------------------
 .../api/v1/workflows_controller_spec.rb       |  71 ++++----
 spec/lib/formatter/csv/workflow_spec.rb       |  21 ++-
 spec/models/workflow_spec.rb                  |  20 ---
 spec/policies/aggregation_policy_spec.rb      |  17 +-
 12 files changed, 70 insertions(+), 280 deletions(-)
 delete mode 100644 spec/controllers/api/v1/aggregations_controller_spec.rb

diff --git a/app/controllers/api/v1/aggregations_controller.rb b/app/controllers/api/v1/aggregations_controller.rb
index b11427957..cc45e0b02 100644
--- a/app/controllers/api/v1/aggregations_controller.rb
+++ b/app/controllers/api/v1/aggregations_controller.rb
@@ -1,19 +1,12 @@
 class Api::V1::AggregationsController < Api::ApiController
-  include JsonApiController::PunditPolicy
+  # include JsonApiController::PunditPolicy
 
-  require_authentication :create, :update, scopes: [:project]
-  resource_actions :create, :update, :show, :index
-  schema_type :json_schema
-  before_action :filter_by_subject_set, only: :index
+  # THIS FUNCTIONALITY IS BEING DEPRECATED EFFECTIVE IMMEDIATELY
+  # A REPLACEMENT IS FORTHCOMING.
 
-  private
+  # require_authentication :create, :update, scopes: [:project]
+  # resource_actions :create, :update, :show, :index
+  # schema_type :json_schema
+  # before_action :filter_by_subject_set, only: :index
 
-  def filter_by_subject_set
-    subject_set_ids = params.delete(:subject_set_id).try(:split, ',')
-    unless subject_set_ids.blank?
-      @controlled_resources = controlled_resources
-        .joins(workflow: :subject_sets)
-        .where(workflows: { subject_set_id: subject_set_ids } )
-    end
-  end
 end
diff --git a/app/models/workflow.rb b/app/models/workflow.rb
index 33878a391..a02db57b2 100644
--- a/app/models/workflow.rb
+++ b/app/models/workflow.rb
@@ -8,6 +8,8 @@ class Workflow < ApplicationRecord
   include Translatable
   include Versioning
 
+  self.ignored_columns = ['aggregation']
+
   versioned association: :workflow_versions, attributes: %w(tasks first_task strings major_version minor_version)
 
   belongs_to :project
@@ -44,7 +46,7 @@ class Workflow < ApplicationRecord
     'options' => {'count' => 15}
   }.freeze
 
-  JSON_ATTRIBUTES = %w(tasks retirement aggregation strings steps).freeze
+  JSON_ATTRIBUTES = %w[tasks retirement strings steps].freeze
 
   SELECTOR_PAGE_SIZE_KEY = 'subject_queue_page_size'.freeze
 
diff --git a/app/policies/aggregation_policy.rb b/app/policies/aggregation_policy.rb
index 4f9a5faf1..64f213f1e 100644
--- a/app/policies/aggregation_policy.rb
+++ b/app/policies/aggregation_policy.rb
@@ -1,12 +1,9 @@
 class AggregationPolicy < ApplicationPolicy
   class ReadScope < Scope
-    # Allow access to public aggregations
+    # Short circuiting scopes for private aggrevations before they get removed next PR
     def resolve(action)
-      updatable_parents = policy_for(Workflow).scope_for(:update)
-      updatable_scope = scope.joins(:workflow).merge(updatable_parents)
-
-      public_aggregations = scope.joins(:workflow).where("workflows.aggregation ->> 'public' = 'true'")
-      Aggregation.union(updatable_scope, public_aggregations)
+      parent_scope = policy_for(Workflow).scope_for(action)
+      scope.where(workflow_id: parent_scope.select(:id))
     end
   end
 
diff --git a/app/schemas/workflow_create_schema.rb b/app/schemas/workflow_create_schema.rb
index 0ed103128..40adb6698 100644
--- a/app/schemas/workflow_create_schema.rb
+++ b/app/schemas/workflow_create_schema.rb
@@ -67,10 +67,6 @@ class WorkflowCreateSchema < JsonSchema
       type "array"
     end
 
-    property "aggregation" do
-      type "object"
-    end
-
     property "configuration" do
       type "object"
     end
diff --git a/app/schemas/workflow_update_schema.rb b/app/schemas/workflow_update_schema.rb
index 0930b70f7..6249cee14 100644
--- a/app/schemas/workflow_update_schema.rb
+++ b/app/schemas/workflow_update_schema.rb
@@ -62,10 +62,6 @@ class WorkflowUpdateSchema < JsonSchema
       type "array"
     end
 
-    property "aggregation" do
-      type "object"
-    end
-
     property "configuration" do
       type "object"
     end
diff --git a/app/serializers/workflow_serializer.rb b/app/serializers/workflow_serializer.rb
index dbb9bc022..ce22d730d 100644
--- a/app/serializers/workflow_serializer.rb
+++ b/app/serializers/workflow_serializer.rb
@@ -10,7 +10,7 @@ class WorkflowSerializer
              :created_at, :updated_at, :finished_at, :first_task, :primary_language,
              :version, :content_language, :prioritized, :grouped, :pairwise,
              :retirement, :retired_set_member_subjects_count, :href, :active, :mobile_friendly,
-             :aggregation, :configuration, :public_gold_standard, :completeness
+             :configuration, :public_gold_standard, :completeness
 
   can_include :project, :subject_sets, :tutorial_subject, :published_version
 
diff --git a/lib/formatter/csv/workflow.rb b/lib/formatter/csv/workflow.rb
index 8e37461b9..a8eecd11c 100644
--- a/lib/formatter/csv/workflow.rb
+++ b/lib/formatter/csv/workflow.rb
@@ -3,12 +3,12 @@ module Csv
     class Workflow
       attr_reader :workflow_version
 
-      JSON_FIELDS = [:tasks, :aggregation, :strings ].freeze
+      JSON_FIELDS = %i[tasks strings].freeze
 
       def headers
-        %w(workflow_id display_name version active classifications_count pairwise
-        grouped prioritized primary_language first_task tutorial_subject_id
-        retired_set_member_subjects_count tasks retirement aggregation strings minor_version)
+        %w[workflow_id display_name version active classifications_count pairwise
+           grouped prioritized primary_language first_task tutorial_subject_id
+           retired_set_member_subjects_count tasks retirement strings minor_version]
       end
 
       def to_rows(workflow_version)
diff --git a/spec/controllers/api/v1/aggregations_controller_spec.rb b/spec/controllers/api/v1/aggregations_controller_spec.rb
deleted file mode 100644
index 354a049b5..000000000
--- a/spec/controllers/api/v1/aggregations_controller_spec.rb
+++ /dev/null
@@ -1,169 +0,0 @@
-require "spec_helper"
-
-RSpec.describe Api::V1::AggregationsController, type: :controller do
-  let(:api_resource_name) { 'aggregations' }
-  let(:api_resource_attributes) { %w(id created_at updated_at aggregation) }
-  let(:api_resource_links) { %w(aggregations.workflow aggregations.subject) }
-
-  let(:scopes) { %w(public project) }
-  let(:resource_class) { Aggregation }
-
-  describe "#index" do
-    let(:workflow) { create(:workflow) }
-    let!(:aggregations) { create_list(:aggregation, 2, workflow: workflow) }
-    let!(:private_resource) { create(:aggregation) }
-    let(:authorized_user) { workflow.project.owner }
-    let(:n_visible) { 2 }
-
-    it_behaves_like "is indexable"
-
-    context "non-logged in users" do
-
-      context "when the workflow does not have public aggregation" do
-        let(:workflow) { create(:workflow) }
-
-        it "should return an empty resource set" do
-          get :index, params: { workflow_id: workflow.id }
-          expect(json_response[api_resource_name].length).to eq(0)
-        end
-      end
-
-      context "when the workflow has the public aggregation flag" do
-        let(:workflow) { create(:workflow, aggregation: { public: true }) }
-        let(:result_workflow_ids) do
-          json_response[api_resource_name].map { |r| r["links"]["workflow"] }.uniq
-        end
-
-        it "should return all the aggregated data for the supplied workflow" do
-          get :index, params: { workflow_id: workflow.id }
-          expect(json_response[api_resource_name].length).to eq n_visible
-        end
-
-        context "when not supplying a workflow id" do
-
-          it "should return all the public workflow resources" do
-            get :index
-            expect(result_workflow_ids).to match_array( [ "#{workflow.id}" ])
-          end
-        end
-
-        context "when supplying just the non public workflow ids" do
-
-          it "should return no results" do
-            get :index, params: { workflow_id: private_resource.workflow_id.to_s }
-            expect(json_response[api_resource_name].length).to eq(0)
-          end
-        end
-
-        context "when supplying a mix of public and non public workflow ids" do
-
-          it "should only return the resources for the public workflow" do
-            ids = [ workflow.id, private_resource.workflow_id ]
-            get :index, params: { workflow_ids: ids.join(',') }
-            expect(result_workflow_ids).to match_array( [ "#{workflow.id}" ])
-          end
-        end
-
-        context "filtering on subject_id" do
-          let(:subject_id) { "#{aggregations.last.subject_id}" }
-
-          it "should return all the aggregated data for the supplied workflow" do
-            get :index, params: { workflow_id: workflow.id, subject_id: subject_id }
-            aggregate_failures "filtering" do
-              resources = json_response[api_resource_name]
-              expect(resources.length).to eq(1)
-              aggregation = resources.first
-              expect(aggregation["links"]["subject"]).to eq(subject_id)
-            end
-          end
-        end
-      end
-    end
-  end
-
-  describe "#show" do
-    let(:resource) { create(:aggregation) }
-    let(:authorized_user) { resource.workflow.project.owner }
-
-    it_behaves_like "is showable"
-
-    context "non-logged in users" do
-
-      context "when the workflow does not have public aggregation" do
-        let(:workflow) { create(:workflow) }
-
-        it "should return not_found" do
-          get :show, params: { id: resource.id }
-          expect(response).to have_http_status(:not_found)
-        end
-      end
-
-      context "when the workflow has the public aggregation flag" do
-
-        it "should return the aggregated resource with a public workflow" do
-          resource.workflow.update_column(:aggregation, { public: true })
-          get :show, params: { id: resource.id }
-          aggregate_failures "public show" do
-            expect(json_response[api_resource_name].length).to eq(1)
-            expect(created_instance(api_resource_name)["id"]).to eq("#{resource.id}")
-          end
-        end
-      end
-    end
-  end
-
-  describe "create" do
-    let(:workflow) { create(:workflow_with_subjects) }
-    let(:subject) { workflow.subject_sets.first.subjects.first }
-    let(:authorized_user) { workflow.project.owner }
-    let(:test_attr) { :aggregation }
-    let(:test_attr_value) { { "something" => "HERE",
-                              "workflow_version" => "1.1" } }
-
-    let(:create_params) do
-      { aggregations:
-          {
-            aggregation: test_attr_value,
-            links: {
-              subject: subject.id.to_s,
-              workflow: workflow.id.to_s
-            }
-          }
-      }
-    end
-
-    it_behaves_like "is creatable"
-  end
-
-  describe '#update' do
-    let(:workflow) { create(:workflow_with_subjects) }
-    let(:subject) { workflow.subject_sets.first.subjects.first }
-    let(:authorized_user) { resource.workflow.project.owner }
-    let(:resource) { create(:aggregation, workflow: workflow) }
-    let(:aggregation_results) do
-      { "mean" => "1",
-        "std" => "1",
-        "count" => ["1","1","1"],
-        "workflow_version" => "1.1" }
-    end
-    let(:test_attr) { :aggregation }
-    let(:test_attr_value) { aggregation_results }
-    let(:test_relation) { :subject }
-    let(:test_relation_ids) { [ subject.id ] }
-    let(:update_params) do
-      { aggregations:
-          {
-            aggregation: aggregation_results,
-            links: {
-              subject: subject.id.to_s,
-              workflow: workflow.id.to_s
-            }
-          }
-      }
-    end
-
-    it_behaves_like "is updatable"
-
-    it_behaves_like "has updatable links"
-  end
-end
diff --git a/spec/controllers/api/v1/workflows_controller_spec.rb b/spec/controllers/api/v1/workflows_controller_spec.rb
index 32a5b818d..316f12d40 100644
--- a/spec/controllers/api/v1/workflows_controller_spec.rb
+++ b/spec/controllers/api/v1/workflows_controller_spec.rb
@@ -1,31 +1,33 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
 describe Api::V1::WorkflowsController, type: :controller do
   let(:user) { create(:user) }
   let(:workflows) { create_list :workflow_with_contents, 2 }
-  let(:workflow){ workflows.first }
-  let(:project){ workflow.project }
-  let(:owner){ project.owner }
-  let(:api_resource_name){ 'workflows' }
+  let(:workflow) { workflows.first }
+  let(:project) { workflow.project }
+  let(:owner) { project.owner }
+  let(:api_resource_name) { 'workflows' }
   let(:resource_class) { Workflow }
   let(:authorized_user) { owner }
   let(:default_params) { { format: :json } }
 
   let(:api_resource_attributes) do
-    %w(id display_name tasks classifications_count subjects_count
-    created_at updated_at first_task primary_language content_language
-    version grouped prioritized pairwise retirement aggregation
-    active mobile_friendly configuration finished_at steps public_gold_standard)
+    %w[id display_name tasks classifications_count subjects_count
+       created_at updated_at first_task primary_language content_language
+       version grouped prioritized pairwise retirement
+       active mobile_friendly configuration finished_at steps public_gold_standard]
   end
   let(:api_resource_links) do
-    %w(workflows.project workflows.subject_sets workflows.tutorial_subject
-    workflows.attached_images)
+    %w[workflows.project workflows.subject_sets workflows.tutorial_subject
+       workflows.attached_images]
   end
-  let(:scopes) { %w(public project) }
+  let(:scopes) { %w[public project] }
 
   describe '#index' do
     let(:filterable_resources) { create_list(:workflow_with_subjects, 2) }
-    let(:expected_filtered_ids) { [ filterable_resources.first.id.to_s ] }
+    let(:expected_filtered_ids) { [filterable_resources.first.id.to_s] }
     let(:private_project) { create(:private_project) }
     let(:private_resource) { create(:workflow, project: private_project) }
     let(:n_visible) { 2 }
@@ -39,7 +41,7 @@
     it_behaves_like 'has many filterable', :subject_sets
 
     describe 'filter by' do
-      before(:each) do
+      before do
         filterable_resources
         default_request user_id: user.id, scopes: scopes
         get :index, params: filter_opts
@@ -51,17 +53,17 @@
 
         it 'only returns activated workflows', :aggregate_failures do
           expect(json_response[api_resource_name].length).to eq(2)
-          expect(json_response[api_resource_name].map{ |w| w['active'] }).to all( be true )
+          expect(json_response[api_resource_name].map { |w| w['active'] }).to all(be true)
         end
 
         it 'does not include in active workflows' do
-          expect(json_response[api_resource_name].map{ |w| w['id'] }).to_not include(inactive_workflow.id)
+          expect(json_response[api_resource_name].map { |w| w['id'] }).not_to include(inactive_workflow.id)
         end
       end
     end
 
     describe 'limiting fields' do
-      before(:each) do
+      before do
         filterable_resources
         default_request user_id: user.id, scopes: scopes
       end
@@ -98,24 +100,23 @@
           display_name: 'A Better Name',
           active: false,
           retirement: { criteria: 'classification_count' },
-          aggregation: {},
           configuration: {},
           public_gold_standard: true,
           tasks: {
             interest: {
-             type: 'draw',
-             question: 'Draw a Circle',
-             next: 'shape',
-             tools: [
-               { value: 'red', label: 'Red', type: 'point', color: 'red' },
-               { value: 'green', label: 'Green', type: 'point', color: 'lime' },
-               { value: 'blue', label: 'Blue', type: 'point', color: 'blue' }
-             ]
-           }
-           },
-           steps: [['S0', { 'taskKeys' => ['T0', 'T1'] }]],
-           display_order_position: 1,
-           links: {
+              type: 'draw',
+              question: 'Draw a Circle',
+              next: 'shape',
+              tools: [
+                { value: 'red', label: 'Red', type: 'point', color: 'red' },
+                { value: 'green', label: 'Green', type: 'point', color: 'lime' },
+                { value: 'blue', label: 'Blue', type: 'point', color: 'blue' }
+              ]
+            }
+          },
+          steps: [['S0', { 'taskKeys' => %w[T0 T1] }]],
+          display_order_position: 1,
+          links: {
             subject_sets: [subject_set.id.to_s],
             tutorials: [tutorial.id.to_s]
           }
@@ -136,8 +137,8 @@
       end
     end
 
-    it_behaves_like "is updatable"
-    it_behaves_like "has updatable links"
+    it_behaves_like 'is updatable'
+    it_behaves_like 'has updatable links'
 
     it_behaves_like 'it syncs the resource translation strings' do
       let(:translated_klass_name) { resource.class.name }
@@ -161,6 +162,7 @@
 
     context 'when extracting strings from workflow' do
       let(:new_question) { 'Contemplate' }
+
       before do
         default_request scopes: scopes, user_id: authorized_user.id
         update_params[:workflows][:tasks][:interest][:question] = new_question
@@ -233,7 +235,7 @@
     end
 
     context 'when a project is live' do
-      before(:each) do
+      before do
         resource.update!(active: true)
         project = resource.project
         project.live = true
@@ -309,7 +311,7 @@
         end
 
         it 'updates the content model' do
-          expect{ resource.reload }.to change{ resource.strings }
+          expect{ resource.reload }.to change(resource, :strings)
         end
       end
     end
@@ -523,7 +525,6 @@
           first_task: 'interest',
           active: true,
           retirement: { criteria: 'classification_count' },
-          aggregation: { public: true },
           configuration: { autoplay_subjects: true },
           public_gold_standard: true,
           tasks: create_task_params,
diff --git a/spec/lib/formatter/csv/workflow_spec.rb b/spec/lib/formatter/csv/workflow_spec.rb
index 05bab3e92..175e35b5a 100644
--- a/spec/lib/formatter/csv/workflow_spec.rb
+++ b/spec/lib/formatter/csv/workflow_spec.rb
@@ -1,4 +1,6 @@
-require "spec_helper"
+# frozen_string_literal: true
+
+require 'spec_helper'
 
 RSpec.describe Formatter::Csv::Workflow do
   let(:workflow) { create(:workflow) }
@@ -26,7 +28,6 @@ def retirement_json
         workflow.retired_set_member_subjects_count,
         workflow_version.tasks.to_json,
         retirement_json,
-        workflow.aggregation.to_json,
         workflow_version.strings.to_json,
         workflow_version.minor_number
       ]
@@ -34,22 +35,24 @@ def retirement_json
   end
 
   let(:header) do
-    %w(workflow_id display_name version minor_version active classifications_count pairwise grouped prioritized primary_language first_task tutorial_subject_id retired_set_member_subjects_count tasks retirement aggregation strings)
+    %w[workflow_id display_name version minor_version active classifications_count pairwise grouped
+       prioritized primary_language first_task tutorial_subject_id retired_set_member_subjects_count
+       tasks retirement strings]
   end
 
-  describe "#headers" do
-    it 'should contain the required headers' do
+  describe '#headers' do
+    it 'contains the required headers' do
       expect(described_class.new.headers).to match_array(header)
     end
   end
 
-  describe "#to_rows" do
+  describe '#to_rows' do
     subject { described_class.new.to_rows(workflow_version) }
 
     it { is_expected.to match_array(rows) }
   end
 
-  context "with a versioned workflow" do
+  context 'with a versioned workflow' do
     let(:q_workflow) { build(:workflow, :question_task) }
     let(:tasks) { q_workflow.tasks }
 
@@ -61,13 +64,13 @@ def retirement_json
       workflow.update(updates)
     end
 
-    describe "#to_rows on the latest version" do
+    describe '#to_rows on the latest version' do
       subject { described_class.new.to_rows(workflow_version) }
 
       it { is_expected.to match_array(rows) }
     end
 
-    describe "#to_rows on the previous version" do
+    describe '#to_rows on the previous version' do
       let(:workflow_version) { workflow.workflow_versions.order(:created_at).first }
 
       subject { described_class.new.to_rows(workflow_version) }
diff --git a/spec/models/workflow_spec.rb b/spec/models/workflow_spec.rb
index c5500a386..652048a9e 100644
--- a/spec/models/workflow_spec.rb
+++ b/spec/models/workflow_spec.rb
@@ -388,26 +388,6 @@
     end
   end
 
-  describe "#aggregation" do
-    let(:workflow) { build(:workflow, aggregation: aggregation_config ) }
-
-    context "empty" do
-      let(:aggregation_config) { Hash.new }
-
-      it "should be valid" do
-        expect(workflow).to be_valid
-      end
-    end
-
-    context "with values" do
-      let(:aggregation_config) { { public: true } }
-
-      it "should be valid" do
-        expect(workflow).to be_valid
-      end
-    end
-  end
-
   describe "#configuration" do
     let(:workflow) { build(:workflow, configuration: config ) }
 
diff --git a/spec/policies/aggregation_policy_spec.rb b/spec/policies/aggregation_policy_spec.rb
index 8be7a2238..73f53ef49 100644
--- a/spec/policies/aggregation_policy_spec.rb
+++ b/spec/policies/aggregation_policy_spec.rb
@@ -5,15 +5,11 @@
     let(:anonymous_user) { nil }
     let(:logged_in_user) { create(:user) }
     let(:resource_owner) { create(:user) }
-
     let(:project)  { build(:project, owner: resource_owner) }
-
-    let(:public_aggregation) { build(:aggregation, workflow: build(:workflow, project: project, aggregation: {public: true})) }
-    let(:private_aggregation) { build(:aggregation, workflow: build(:workflow, project: project)) }
+    let(:public_aggregation) { build(:aggregation, workflow: build(:workflow, project: project)) }
 
     before do
       public_aggregation.save!
-      private_aggregation.save!
     end
 
     describe 'index' do
@@ -44,7 +40,7 @@
           expect(resolved_scope).to include(public_aggregation)
         end
 
-        it 'includes aggregations from owned private projects' do
+        xit 'includes aggregations from owned private projects' do
           expect(resolved_scope).to include(private_aggregation)
         end
       end
@@ -54,7 +50,7 @@
         let(:api_user) { ApiUser.new(admin_user, admin: true) }
 
         it 'includes everything' do
-          expect(resolved_scope).to include(public_aggregation, private_aggregation)
+          expect(resolved_scope).to include(public_aggregation)
         end
       end
     end
@@ -86,10 +82,6 @@
         it "includes aggregations from public projects" do
           expect(resolved_scope).to include(public_aggregation)
         end
-
-        it 'includes aggregations from owned private projects' do
-          expect(resolved_scope).to include(private_aggregation)
-        end
       end
 
       context 'for an admin' do
@@ -97,10 +89,9 @@
         let(:api_user) { ApiUser.new(admin_user, admin: true) }
 
         it 'includes everything' do
-          expect(resolved_scope).to include(public_aggregation, private_aggregation)
+          expect(resolved_scope).to include(public_aggregation)
         end
       end
-
     end
   end
 end