Merge pull request #36 from DFE-Digital/with_model

Adopt with_model

Gemfile.lock

@@ -348,6 +348,8 @@ GEM
     websocket-driver (0.7.5)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
+    with_model (2.1.6)
+      activerecord (>= 5.2)
     yard (0.9.28)
       webrick (~> 1.7.0)
     zeitwerk (2.5.4)
@@ -370,6 +372,7 @@ DEPENDENCIES
   solargraph
   sqlite3
   webmock (~> 3.14)
+  with_model
 
 BUNDLED WITH
    2.3.13

dfe-analytics.gemspec

@@ -37,5 +37,6 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'solargraph'
   spec.add_development_dependency 'sqlite3'
   spec.add_development_dependency 'webmock', '~> 3.14'
+  spec.add_development_dependency 'with_model'
   spec.metadata['rubygems_mfa_required'] = 'true'
 end

spec/dfe/analytics/entities_spec.rb

@@ -4,35 +4,26 @@
   let(:interesting_fields) { [] }
   let(:pii_fields) { [] }
 
-  let(:model) do
-    Class.new(Candidate) do
-      include DfE::Analytics::Entities
-
-      # yes, ugh, but another part of the code is going to enumerate
-      # descendants of activerecord and map them tables, and if the test for
-      # that code runs after this then a class without a .name means we map
-      # from nil.
-      #
-      # Assigning the class to a constant is another way to name it, but that
-      # creates _other_ problems, such as the fact that aforementioned test
-      # will expect the class to be called "Candidate" and there will exist a
-      # class called "model" referencing the same table.
-      def self.name
-        'Candidate'
-      end
+  with_model :Candidate do
+    table do |t|
+      t.string :email_address
+      t.string :last_name
+      t.string :first_name
     end
+
+    model { include DfE::Analytics::Entities }
   end
 
   before do
     allow(DfE::Analytics::SendEvents).to receive(:perform_later)
     allow(DfE::Analytics).to receive(:enabled?).and_return(true)
 
     allow(DfE::Analytics).to receive(:allowlist).and_return({
-      candidates: interesting_fields
+      Candidate.table_name.to_sym => interesting_fields
     })
 
     allow(DfE::Analytics).to receive(:allowlist_pii).and_return({
-      candidates: pii_fields
+      Candidate.table_name.to_sym => pii_fields
     })
   end
 
@@ -41,11 +32,11 @@ def self.name
       let(:interesting_fields) { [:id] }
 
       it 'includes attributes specified in the settings file' do
-        model.create(id: 123)
+        Candidate.create(id: 123)
 
         expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
           .with([a_hash_including({
-            'entity_table_name' => 'candidates',
+            'entity_table_name' => Candidate.table_name,
             'event_type' => 'create_entity',
             'data' => [
               { 'key' => 'id', 'value' => [123] }
@@ -54,11 +45,11 @@ def self.name
       end
 
       it 'does not include attributes not specified in the settings file' do
-        model.create(id: 123, email_address: 'a@b.com')
+        Candidate.create(id: 123, email_address: 'a@b.com')
 
         expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
           .with([a_hash_including({
-            'entity_table_name' => 'candidates',
+            'entity_table_name' => Candidate.table_name,
             'event_type' => 'create_entity',
             'data' => [
               { 'key' => 'id', 'value' => [123] }
@@ -68,7 +59,7 @@ def self.name
       end
 
       it 'sends events that are valid according to the schema' do
-        model.create
+        Candidate.create
 
         expect(DfE::Analytics::SendEvents).to have_received(:perform_later) do |payload|
           schema = DfE::Analytics::EventSchema.new.as_json
@@ -81,7 +72,7 @@ def self.name
       it 'sends events with the request UUID, if available' do
         RequestLocals.store[:dfe_analytics_request_id] = 'example-request-id'
 
-        model.create
+        Candidate.create
 
         expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
           .with([a_hash_including({
@@ -94,7 +85,7 @@ def self.name
         let(:pii_fields) { [:email_address] }
 
         it 'hashes those fields' do
-          model.create(email_address: 'adrienne@example.com')
+          Candidate.create(email_address: 'adrienne@example.com')
 
           expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
             .with([a_hash_including({
@@ -111,7 +102,7 @@ def self.name
         let(:pii_fields) { [:email_address] }
 
         it 'does not include the fields only listed as PII' do
-          model.create(id: 123, email_address: 'adrienne@example.com')
+          Candidate.create(id: 123, email_address: 'adrienne@example.com')
 
           expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
             .with([a_hash_including({
@@ -127,7 +118,7 @@ def self.name
       let(:interesting_fields) { [] }
 
       it 'does not send create_entity events at all' do
-        model.create
+        Candidate.create
 
         expect(DfE::Analytics::SendEvents).not_to have_received(:perform_later)
           .with([a_hash_including({ 'event_type' => 'create_entity' })])
@@ -140,12 +131,12 @@ def self.name
       let(:interesting_fields) { %i[email_address first_name] }
 
       it 'sends update events for fields we care about' do
-        entity = model.create(email_address: 'foo@bar.com', first_name: 'Jason')
+        entity = Candidate.create(email_address: 'foo@bar.com', first_name: 'Jason')
         entity.update(email_address: 'bar@baz.com')
 
         expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
           .with([a_hash_including({
-            'entity_table_name' => 'candidates',
+            'entity_table_name' => Candidate.table_name,
             'event_type' => 'update_entity',
             'data' => [
               { 'key' => 'email_address', 'value' => ['bar@baz.com'] },
@@ -155,7 +146,7 @@ def self.name
       end
 
       it 'does not send update events for fields we don’t care about' do
-        entity = model.create
+        entity = Candidate.create
         entity.update(last_name: 'GB')
 
         expect(DfE::Analytics::SendEvents).not_to have_received(:perform_later)
@@ -165,7 +156,7 @@ def self.name
       end
 
       it 'sends events that are valid according to the schema' do
-        entity = model.create
+        entity = Candidate.create
         entity.update(email_address: 'bar@baz.com')
 
         expect(DfE::Analytics::SendEvents).to have_received(:perform_later).twice do |payload|
@@ -181,7 +172,7 @@ def self.name
       let(:interesting_fields) { [] }
 
       it 'does not send update events at all' do
-        entity = model.create
+        entity = Candidate.create
         entity.update(first_name: 'Persephone')
 
         expect(DfE::Analytics::SendEvents).not_to have_received(:perform_later)
@@ -196,12 +187,12 @@ def self.name
     let(:interesting_fields) { [:email_address] }
 
     it 'sends events when objects are deleted' do
-      entity = model.create(email_address: 'boo@example.com')
+      entity = Candidate.create(email_address: 'boo@example.com')
       entity.destroy
 
       expect(DfE::Analytics::SendEvents).to have_received(:perform_later)
         .with([a_hash_including({
-          'entity_table_name' => 'candidates',
+          'entity_table_name' => Candidate.table_name,
           'event_type' => 'delete_entity',
           'data' => [
             { 'key' => 'email_address', 'value' => ['boo@example.com'] }

spec/dfe/analytics/fields_spec.rb

@@ -1,7 +1,15 @@
 RSpec.describe DfE::Analytics::Fields do
   context 'with dummy data' do
-    let(:existing_allowlist) { { candidates: ['email_address'] } }
-    let(:existing_blocklist) { { candidates: ['id'] } }
+    with_model :Candidate do
+      table do |t|
+        t.string :email_address
+        t.string :first_name
+        t.string :last_name
+      end
+    end
+
+    let(:existing_allowlist) { { Candidate.table_name.to_sym => ['email_address'] } }
+    let(:existing_blocklist) { { Candidate.table_name.to_sym => ['id'] } }
 
     before do
       allow(DfE::Analytics).to receive(:allowlist).and_return(existing_allowlist)
@@ -22,7 +30,7 @@
 
     describe '.unlisted_fields' do
       it 'returns all the fields in the model that aren’t in either list' do
-        fields = described_class.unlisted_fields[:candidates]
+        fields = described_class.unlisted_fields[Candidate.table_name.to_sym]
         expect(fields).to include('first_name')
         expect(fields).to include('last_name')
         expect(fields).not_to include('email_address')
@@ -32,19 +40,18 @@
 
     describe '.conflicting_fields' do
       context 'when fields conflict' do
-        let(:existing_allowlist) { { candidates: %w[email_address id first_name], institutions: %w[id] } }
-        let(:existing_blocklist) { { candidates: %w[email_address first_name] } }
+        let(:existing_allowlist) { { Candidate.table_name.to_sym => %w[email_address id first_name] } }
+        let(:existing_blocklist) { { Candidate.table_name.to_sym => %w[email_address first_name] } }
 
         it 'returns the conflicting fields' do
           conflicts = described_class.conflicting_fields
-          expect(conflicts.keys).to eq(%i[candidates])
-          expect(conflicts[:candidates]).to eq(%w[email_address first_name])
+          expect(conflicts[Candidate.table_name.to_sym]).to eq(%w[email_address first_name])
         end
       end
 
       context 'when there are no conflicts' do
-        let(:existing_allowlist) { { candidates: %w[email_address], institutions: %w[id] } }
-        let(:existing_blocklist) { { candidates: %w[id] } }
+        let(:existing_allowlist) { { Candidate.table_name.to_sym => %w[email_address] } }
+        let(:existing_blocklist) { { Candidate.table_name.to_sym => %w[id] } }
 
         it 'returns nothing' do
           conflicts = described_class.conflicting_fields
@@ -55,7 +62,7 @@
 
     describe '.generate_blocklist' do
       it 'returns all the fields in the model that aren’t in the allowlist' do
-        fields = described_class.generate_blocklist[:candidates]
+        fields = described_class.generate_blocklist[Candidate.table_name.to_sym]
         expect(fields).to include('first_name')
         expect(fields).to include('last_name')
         expect(fields).to include('id')
@@ -65,17 +72,17 @@
 
     describe '.surplus_fields' do
       it 'returns nothing' do
-        fields = described_class.surplus_fields[:candidates]
+        fields = described_class.surplus_fields[Candidate.table_name.to_sym]
         expect(fields).to be_nil
       end
     end
 
     context 'when the lists deal with an attribute that is no longer in the database' do
-      let(:existing_allowlist) { { candidates: ['some_removed_field'] } }
+      let(:existing_allowlist) { { Candidate.table_name.to_sym => ['some_removed_field'] } }
 
       describe '.surplus_fields' do
         it 'returns the field that has been removed' do
-          fields = described_class.surplus_fields[:candidates]
+          fields = described_class.surplus_fields[Candidate.table_name.to_sym]
           expect(fields).to eq ['some_removed_field']
         end
       end

spec/dfe/analytics/load_entities_spec.rb

@@ -3,18 +3,20 @@
 RSpec.describe DfE::Analytics::LoadEntities do
   include ActiveJob::TestHelper
 
+  with_model :Candidate do
+    table do |t|
+      t.string :email_address
+    end
+  end
+
   before do
-    allow(DfE::Analytics).to receive(:allowlist).and_return(
-      {
-        candidates: ['email_address']
-      }
-    )
-
-    allow(DfE::Analytics).to receive(:allowlist_pii).and_return(
-      {
-        candidates: []
-      }
-    )
+    allow(DfE::Analytics).to receive(:allowlist).and_return({
+      Candidate.table_name.to_sym => ['email_address']
+    })
+
+    allow(DfE::Analytics).to receive(:allowlist_pii).and_return({
+      Candidate.table_name.to_sym => []
+    })
 
     allow(DfE::Analytics::SendEvents).to receive(:perform_later)
   end
@@ -28,7 +30,7 @@
   it 'sends a entity’s fields to BQ' do
     Candidate.create(email_address: 'known@address.com')
 
-    described_class.new(entity_name: 'candidates').run
+    described_class.new(entity_name: Candidate.table_name).run
 
     expect(DfE::Analytics::SendEvents).to have_received(:perform_later) do |payload|
       schema = DfE::Analytics::EventSchema.new.as_json
@@ -46,7 +48,7 @@
     Candidate.create
     Candidate.create
 
-    described_class.new(entity_name: 'candidates', batch_size: 2).run
+    described_class.new(entity_name: Candidate.table_name, batch_size: 2).run
 
     expect(DfE::Analytics::SendEvents).to have_received(:perform_later).once
   end

spec/dfe/analytics/requests_spec.rb

@@ -4,9 +4,28 @@
   before do
     controller = Class.new(ApplicationController) do
       include DfE::Analytics::Requests
+
+      def index
+        render plain: 'hello'
+      end
+
+      private
+
+      def current_user
+        Struct.new(:id).new(1)
+      end
+
+      def current_namespace
+        'example_namespace'
+      end
     end
-    unauthenticated_controller = Class.new(UnauthenticatedController) do
+
+    unauthenticated_controller = Class.new(ApplicationController) do
       include DfE::Analytics::Requests
+
+      def index
+        render plain: 'hello'
+      end
     end
 
     stub_const('TestController', controller)
@@ -41,7 +60,7 @@
       anonymised_user_agent_and_ip: '16859db7ca4ec906925a0a2cb227bf307740a0c919ab9e2f7efeadf37779e770',
       response_content_type: 'text/plain; charset=utf-8',
       response_status: 200,
-      namespace: 'ddd',
+      namespace: 'example_namespace',
       user_id: 1 }
   end
 
@@ -58,7 +77,7 @@
     expect(request.with do |req|
       body = JSON.parse(req.body)
       payload = body['rows'].first['json']
-      expect(payload.except('occurred_at', 'request_uuid')).to match(a_hash_including(event.deep_stringify_keys))
+      expect(payload.except('occurred_at', 'request_uuid')).to match(event.deep_stringify_keys)
     end).to have_been_made
   end
 
@@ -72,7 +91,7 @@
         request_query: [],
         request_referer: nil,
         anonymised_user_agent_and_ip: '12ca17b49af2289436f303e0166030a21e525d266e209267433801a8fd4071a0',
-        response_content_type: 'application/json; charset=utf-8',
+        response_content_type: 'text/plain; charset=utf-8',
         response_status: 200 }
     end
 
@@ -88,7 +107,7 @@
       expect(request.with do |req|
         body = JSON.parse(req.body)
         payload = body['rows'].first['json']
-        expect(payload.except('occurred_at', 'request_uuid')).to match(a_hash_including(event.deep_stringify_keys))
+        expect(payload.except('occurred_at', 'request_uuid')).to match(event.deep_stringify_keys)
       end).to have_been_made
     end
   end