Merged in r2-3008-summary-fields-caching (pull request #7025)

R2-3008 - Fix: Readable fields are not permitted when an update is performed

app/services/permitted_form_fields_service.rb

@@ -4,9 +4,9 @@
 
 # This service handles computing the permitted fields for a given role,
 # based on the forms associated with that role. The query is optionally cached.
-# The similarly named PermittedFieldService uses this service to compuyte the full list of permitted fields
+# The similarly named PermittedFieldService uses this service to compute the full list of permitted fields
 class PermittedFormFieldsService
-  attr_accessor :fields, :field_names, :with_cache
+  attr_accessor :with_cache
 
   PERMITTED_WRITEABLE_FIELD_TYPES = [
     Field::TEXT_FIELD, Field::TEXT_AREA, Field::RADIO_BUTTON, Field::TICK_BOX,
@@ -15,7 +15,7 @@ class PermittedFormFieldsService
     Field::SUBFORM, Field::TALLY_FIELD, Field::CALCULATED
   ].freeze
 
-  # TODO: Primero is assuming that these forms exist in the configuration. If they
+  # TODO: Primero is assuming that these forms exist in the configuration.
   PERMITTED_SUBFORMS_FOR_ACTION = {
     Permission::ADD_NOTE => 'notes_section',
     Permission::INCIDENT_DETAILS_FROM_CASE => 'incident_details',
@@ -30,17 +30,27 @@ def initialize(with_cache = false)
     self.with_cache = with_cache
   end
 
-  def rebuild_cache(roles, record_type, module_unique_id, writeable, force = false)
-    return unless force || fields.nil?
+  def generate_cache_key(roles, record_type, module_unique_id, writeable)
+    role_keys = roles.map(&:cache_key_with_version)
 
     # The assumption here is that the cache will be updated if any changes took place to Forms, or Roles
-    role_keys = roles.map(&:cache_key_with_version)
-    cache_key = "permitted_form_fields_service/#{role_keys.join('/')}/#{module_unique_id}/#{record_type}/#{writeable}"
-    self.fields = Rails.cache.fetch(cache_key, expires_in: 48.hours) do
+    "permitted_form_fields_service/#{role_keys.join('/')}/#{module_unique_id}/#{record_type}/#{writeable}"
+  end
+
+  def permitted_fields_from_cache(roles, record_type, module_unique_id, writeable, force = false)
+    cache_key = generate_cache_key(roles, record_type, module_unique_id, writeable)
+
+    Rails.cache.fetch(cache_key, expires_in: 48.hours, force:) do
       permitted_fields_from_forms(roles, record_type, module_unique_id, writeable).to_a
     end
-    # TODO: This can be cached too
-    self.field_names = fields.map(&:name).uniq
+  end
+
+  def permitted_field_names_from_cache(roles, record_type, module_unique_id, writeable, force = false)
+    cache_key = generate_cache_key(roles, record_type, module_unique_id, writeable)
+
+    Rails.cache.fetch("#{cache_key}/names", expires_in: 48.hours, force:) do
+      permitted_fields_from_cache(roles, record_type, module_unique_id, writeable, force).map(&:name).uniq
+    end
   end
 
   def permitted_fields_from_forms(roles, record_type, module_unique_id, writeable, visible_only = false)
@@ -56,17 +66,15 @@ def permitted_fields_from_forms(roles, record_type, module_unique_id, writeable,
 
   def permitted_fields(roles, record_type, module_unique_id, writeable)
     if with_cache?
-      rebuild_cache(roles, record_type, module_unique_id, writeable)
-      fields
+      permitted_fields_from_cache(roles, record_type, module_unique_id, writeable)
     else
       permitted_fields_from_forms(roles, record_type, module_unique_id, writeable).to_a
     end
   end
 
   def permitted_field_names(roles, record_type, module_unique_id, writeable)
     if with_cache?
-      rebuild_cache(roles, record_type, module_unique_id, writeable)
-      field_names
+      permitted_field_names_from_cache(roles, record_type, module_unique_id, writeable)
     else
       permitted_fields_from_forms(roles, record_type, module_unique_id, writeable).map(&:name).uniq
     end

app/services/subform_summary_fields_service.rb

@@ -1,11 +1,13 @@
 # frozen_string_literal: true
 
 # Copyright (c) 2014 - 2023 UNICEF. All rights reserved.
+
+# This service retrieves the summary fields. The query is optionally cached.
 class SubformSummaryFieldsService
-  attr_accessor :fields, :with_cache
+  attr_accessor :with_cache
 
   def self.instance
-    @instance ||= new(Rails.configuration.use_app_cache)
+    new(Rails.configuration.use_app_cache)
   end
 
   alias with_cache? with_cache
@@ -14,27 +16,24 @@ def initialize(with_cache = false)
     self.with_cache = with_cache
   end
 
-  def rebuild_cache(record_type, force = false)
-    return unless force || fields.nil?
-
+  def subform_summary_fields_from_cache(record_type, force = false)
     # The assumption here is that the cache will be updated if any changes took place to Fields
     max_updated_at = Field.maximum(:updated_at).as_json
     cache_key = "subform_summary_fields_service/fields/#{max_updated_at}/#{record_type}"
-    self.fields = Rails.cache.fetch(cache_key, expires_in: 48.hours) do
-      loaded_fields(record_type).to_a
+    Rails.cache.fetch(cache_key, expires_in: 48.hours, force:) do
+      subform_summary_fields_from_forms(record_type).to_a
     end
   end
 
   def subform_summary_fields(record_type)
     if with_cache?
-      rebuild_cache(record_type)
-      fields
+      subform_summary_fields_from_cache(record_type)
     else
-      loaded_fields(record_type)
+      subform_summary_fields_from_forms(record_type)
     end
   end
 
-  def loaded_fields(record_type)
+  def subform_summary_fields_from_forms(record_type)
     Field.joins(:form_section)
          .where(form_section: { parent_form: record_type }, visible: true)
          .where.not(subform_summary: nil)

spec/services/permitted_form_fields_service_spec.rb

@@ -355,7 +355,7 @@
 
   describe '#permitted_fields' do
     it 'lists all writeable fields' do
-      permitted_fields = service.permitted_fields(role, 'case', 'primeromodule-cpa', true)
+      permitted_fields = service.permitted_fields([role], 'case', 'primeromodule-cpa', true)
       expect(permitted_fields.size).to eq(11)
       expect(permitted_fields.map(&:name)).to match_array(
         %w[name age sex national_id_no consent_for_services current_address protection_concerns
@@ -364,7 +364,7 @@
     end
 
     it 'lists all readable fields' do
-      permitted_fields = service.permitted_fields(role, 'case', 'primeromodule-cpa', false)
+      permitted_fields = service.permitted_fields([role], 'case', 'primeromodule-cpa', false)
       expect(permitted_fields.size).to eq(14)
       expect(permitted_fields.map(&:name)).to match_array(
         %w[name age sex national_id_no consent_for_services current_address protection_concerns
@@ -373,7 +373,7 @@
     end
 
     it 'includes action subforms when writeable for the module' do
-      permitted_fields = service.permitted_fields(role_with_actions, 'case', 'primeromodule-cpa', true)
+      permitted_fields = service.permitted_fields([role_with_actions], 'case', 'primeromodule-cpa', true)
       expect(permitted_fields.size).to eq(13)
       expect(permitted_fields.map(&:name)).to match_array(
         %w[name age sex national_id_no consent_for_services current_address protection_concerns other_documents
@@ -382,37 +382,57 @@
     end
 
     it 'excludes action subforms when readable' do
-      permitted_fields = service.permitted_fields(role_with_actions, 'case', 'primeromodule-cpa', false)
+      permitted_fields = service.permitted_fields([role_with_actions], 'case', 'primeromodule-cpa', false)
       expect(permitted_fields.size).to eq(14)
       expect(permitted_fields.map(&:name)).to_not include(:notes_section, :services_section)
     end
 
     it 'includes mrm subforms when writeable' do
-      permitted_fields = service.permitted_fields(role_mrm, 'incident', PrimeroModule::MRM, true)
+      permitted_fields = service.permitted_fields([role_mrm], 'incident', PrimeroModule::MRM, true)
       expect(permitted_fields.size).to eq(2)
       expect(permitted_fields.map(&:name)).to match_array(%w[another_field killing])
     end
 
     context 'when a user has access to the notes_section field and can add notes' do
       it 'returns fields for the specified parent_form and module' do
-        permitted_fields = service.permitted_fields(role_with_notes, 'case', 'primeromodule-cpa', true)
+        permitted_fields = service.permitted_fields([role_with_notes], 'case', 'primeromodule-cpa', true)
         expect(permitted_fields.size).to eq(1)
         expect(permitted_fields.map { |field| field.form_section.parent_form }).to match_array(%w[case])
         expect(permitted_fields.map(&:name)).to match_array(%w[notes_section])
       end
 
       it 'returns the notes_section fields for the cp module' do
-        permitted_fields = service.permitted_fields(role_cp, 'case', PrimeroModule::CP, true)
+        permitted_fields = service.permitted_fields([role_cp], 'case', PrimeroModule::CP, true)
         expect(permitted_fields.size).to eq(1)
         expect(permitted_fields.first.subform.fields.map(&:name)).to match_array(%w[name comment])
       end
 
       it 'returns the notes_section fields for the gbv module' do
-        permitted_fields = service.permitted_fields(role_gbv, 'case', PrimeroModule::GBV, true)
+        permitted_fields = service.permitted_fields([role_gbv], 'case', PrimeroModule::GBV, true)
         expect(permitted_fields.size).to eq(1)
         expect(permitted_fields.first.subform.fields.map(&:name)).to match_array(%w[name age])
       end
     end
+
+    describe 'when with_cache?' do
+      let(:service_with_cache) { PermittedFormFieldsService.new(true) }
+
+      it 'lists all writeable and readable fields' do
+        permitted_writeable_fields = service_with_cache.permitted_fields([role], 'case', 'primeromodule-cpa', true)
+        permitted_readable_fields = service_with_cache.permitted_fields([role], 'case', 'primeromodule-cpa', false)
+
+        expect(permitted_writeable_fields.size).to eq(11)
+        expect(permitted_writeable_fields.map(&:name)).to match_array(
+          %w[name age sex national_id_no consent_for_services current_address protection_concerns
+             registration_date created_on family_details other_documents]
+        )
+        expect(permitted_readable_fields.size).to eq(14)
+        expect(permitted_readable_fields.map(&:name)).to match_array(
+          %w[name age sex national_id_no consent_for_services current_address protection_concerns
+             registration_date created_on separator1 other_documents family_details interview_date consent_for_referral]
+        )
+      end
+    end
   end
 
   describe '#permitted_field_names' do

spec/services/subform_summary_fields_service_spec.rb

@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+# Copyright (c) 2014 - 2023 UNICEF. All rights reserved.
+
+require 'rails_helper'
+
+describe SubformSummaryFieldsService do
+  before :each do
+    clean_data(Field, FormSection)
+  end
+
+  let(:service) { SubformSummaryFieldsService.new }
+
+  let!(:form_section1) do
+    score_section = FormSection.create!(
+      unique_id: 'scores_section', parent_form: 'case', name_en: 'Scores', is_nested: true,
+      fields: [
+        Field.new(name: 'date', type: Field::DATE_FIELD, display_name_en: 'Date'),
+        Field.new(name: 'score', type: Field::NUMERIC_FIELD, display_name_en: 'Score')
+      ]
+    )
+
+    FormSection.create!(
+      unique_id: 'form_section1', parent_form: 'case', name_en: 'form_section1',
+      fields: [
+        Field.new(name: 'name', type: Field::TEXT_FIELD, display_name_en: 'Name'),
+        Field.new(name: 'age', type: Field::NUMERIC_FIELD, display_name_en: 'Age'),
+        Field.new(name: 'sex', type: Field::SELECT_BOX, display_name_en: 'Sex'),
+        Field.new(
+          name: 'most_recent_score',
+          type: Field::NUMERIC_FIELD,
+          display_name_en: 'Most Recent Score',
+          subform_summary: {
+            subform_field_name: 'scores_section',
+            first: {
+              field_name: 'score',
+              order_by: 'date',
+              order: 'asc'
+            }
+          }
+        ),
+        Field.new(
+          name: 'last_score',
+          type: Field::NUMERIC_FIELD,
+          display_name_en: 'Last Score',
+          visible: false,
+          subform_summary: {
+            subform_field_name: 'scores_section',
+            first: {
+              field_name: 'score',
+              order_by: 'date',
+              order: 'asc'
+            }
+          }
+        ),
+        Field.new(name: 'scores_section', display_name_en: 'Scores', type: Field::SUBFORM, subform: score_section)
+      ]
+    )
+  end
+
+  it 'returns all the subform summary fields' do
+    summary_fields = SubformSummaryFieldsService.instance.subform_summary_fields('case')
+
+    expect(summary_fields.size).to eq(1)
+    expect(summary_fields.map(&:name)).to match_array(%w[most_recent_score])
+  end
+
+  describe 'when with_cache?' do
+    let(:service) { SubformSummaryFieldsService.new(true) }
+
+    it 'returns all the subform summary fields' do
+      summary_fields = SubformSummaryFieldsService.instance.subform_summary_fields('case')
+
+      expect(summary_fields.size).to eq(1)
+      expect(summary_fields.map(&:name)).to match_array(%w[most_recent_score])
+    end
+  end
+end