Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to straighten out language and improve dfe:analytics:check #30

Merged
merged 8 commits into from
Jul 25, 2022
Merged

Refactor to straighten out language and improve dfe:analytics:check #30

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1183c3a
Make language around entities and models consistent
duncanjbrown Jul 19, 2022
7b0805e
Remove unused method
duncanjbrown Jul 14, 2022
d7fe973
Dedupe anonymise function
duncanjbrown Jul 14, 2022
ed3ca8e
Refactor field diffing
duncanjbrown Jul 19, 2022
c2ab521
Detect conflicting fields in dfe:analytics:check
duncanjbrown Jul 19, 2022
0487f75
Remove class method-level memoization
duncanjbrown Jul 19, 2022
5f8277c
Disable Metrics/ModuleLength
duncanjbrown Jul 19, 2022
68885e9
Further refactor diffing code
duncanjbrown Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/dfe/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ def self.async?
end

def self.entities_for_analytics
allowlist.keys & entity_model_mapping.keys.map(&:to_sym)
allowlist.keys & all_entities_in_application
end

def self.all_entities_in_application
entity_model_mapping.keys.map(&:to_sym)
end

def self.model_for_entity(entity)
Expand Down
34 changes: 15 additions & 19 deletions lib/dfe/analytics/fields.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,21 @@ def self.surplus_fields
end

def self.diff_model_attributes_against(*lists)
Rails.application.eager_load!
ActiveRecord::Base.descendants
.reject { |model| model.name.include? 'ActiveRecord' } # ignore internal AR classes
.reduce({ missing: {}, surplus: {} }) do |diff, next_model|
table_name = next_model.table_name&.to_sym

if table_name.present?
attributes_considered = # then combine to get all the attrs we deal with
lists.map do |list|
# for each list of model attrs, look up the attrs for this model
list.fetch(table_name, [])
end.reduce(:concat)
missing_attributes = next_model.attribute_names - attributes_considered
surplus_attributes = attributes_considered - next_model.attribute_names

diff[:missing][table_name] = missing_attributes if missing_attributes.any?

diff[:surplus][table_name] = surplus_attributes if surplus_attributes.any?
end
DfE::Analytics.all_entities_in_application
.reduce({ missing: {}, surplus: {} }) do |diff, entity|
attributes_considered = lists.map do |list|
# for each list of model attrs, look up the attrs for this model
list.fetch(entity, [])
end.reduce(:concat)

model = DfE::Analytics.model_for_entity(entity)

missing_attributes = model.attribute_names - attributes_considered
surplus_attributes = attributes_considered - model.attribute_names

diff[:missing][entity] = missing_attributes if missing_attributes.any?

diff[:surplus][entity] = surplus_attributes if surplus_attributes.any?
Copy link
Contributor

@misaka misaka Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this bit a bit hard to digest. I think if you're going to be refactoring this bit, it would make sense to try to make it easier to comprehend, if possible. Maybe something like this (NB: untested code):

      def self.entity_attributes_from_lists(entity, lists)
        lists.map do |list|
          # for each list of model attrs, look up the attrs for this model
          list.fetch(entity, [])
        end.reduce(:concat)
      end

      def self.entities_with_missing_attributes_from_lists(*lists)
        DfE::Analytics.all_entities_in_application
          .reduce({}) do |entities_missing_attributes, entity|
            attributes_considered = entity_attributes_from_lists(entity, lists)
            model = DfE::Analytics.model_for_entity(entity)

            entities_missing_attributes[entity] = model.attribute_names - attributes_considered
            entities_missing_attributes
          end.reject { |_entity, attributes| attributes.empty? }
      end

      def self.entities_with_surplus_attributes_from_lists(*lists)
        DfE::Analytics.all_entities_in_application
          .reduce({}) do |entities_surplus_attributes, entity|
            attributes_considered = entity_attributes_from_lists(entity, lists)
            model = DfE::Analytics.model_for_entity(entity)

            entities_surplus_attributes[entity] =  attributes_considered - model.attribute_names
            entities_surplus_attributes
          end.reject { |_entity, attributes| attributes.empty? }
      end


diff
end
Expand Down