more spec refactoring based on feedback

spec/features/projects/import_export/export_file_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 # It looks up for any sensitive word inside the JSON, so if a sensitive word is found
 # we''l have to either include it adding the model that includes it to the +safe_list+
 # or make sure the attribute is blacklisted in the +import_export.yml+ configuration
-feature 'project export', feature: true, js: true do
+feature 'Import/Export - project export integration test', feature: true, js: true do
   include Select2Helper
   include ExportFileHelper
 
@@ -58,9 +58,23 @@ feature 'project export', feature: true, js: true do
         sensitive_words.each do |sensitive_word|
           found = find_sensitive_attributes(sensitive_word, project_hash)
 
-          expect(found).to be_nil, "Found a new sensitive word <#{found.try(:key_found)}>, which is part of the hash #{found.try(:parent)}"
+          expect(found).to be_nil, failure_message(found.try(:key_found), found.try(:parent), sensitive_word)
         end
       end
     end
+
+    def failure_message(key_found, parent, sensitive_word)
+      <<-MSG
+        Found a new sensitive word <#{key_found}>, which is part of the hash #{parent.inspect}
+
+        If you think this information shouldn't get exported, please exclude the model or attribute in IMPORT_EXPORT_CONFIG.
+
+        Otherwise, please add the exception to +safe_list+ in CURRENT_SPEC using #{sensitive_word} as the key and the
+        correspondent hash or model as the value.
+
+        IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file}
+        CURRENT_SPEC: #{__FILE__}
+      MSG
+    end
   end
 end

spec/features/projects/import_export/import_file_spec.rb

 require 'spec_helper'
 
-feature 'project import', feature: true, js: true do
+feature 'Import/Export - project import integration test', feature: true, js: true do
   include Select2Helper
 
   let(:admin) { create(:admin) }

spec/lib/gitlab/import_export/attribute_configuration_spec.rb

@@ -7,7 +7,7 @@ require 'spec_helper'
 # to be included as part of the export, or blacklist them using the import_export.yml configuration file.
 # Likewise, new models added to import_export.yml, will need to be added with their correspondent attributes
 # to this spec.
-describe 'Attribute configuration', lib: true do
+describe 'Import/Export attribute configuration', lib: true do
   include ConfigurationHelper
 
   let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
@@ -20,32 +20,8 @@ describe 'Attribute configuration', lib: true do
     names.flatten.uniq - ['milestones', 'labels'] + ['project']
   end
 
-  let(:safe_model_attributes) do
-    {
-      'Issue' => %w[id title assignee_id author_id project_id created_at updated_at position branch_name description state iid updated_by_id confidential deleted_at due_date moved_to_id lock_version milestone_id weight],
-      'Event' => %w[id target_type target_id title data project_id created_at updated_at action author_id],
-      'Note' => %w[id note noteable_type author_id created_at updated_at project_id attachment line_code commit_id noteable_id system st_diff updated_by_id type position original_position resolved_at resolved_by_id discussion_id original_discussion_id],
-      'LabelLink' => %w[id label_id target_id target_type created_at updated_at],
-      'Label' => %w[id title color project_id created_at updated_at template description priority],
-      'Milestone' => %w[id title project_id description due_date created_at updated_at state iid],
-      'ProjectSnippet' => %w[id title content author_id project_id created_at updated_at file_name type visibility_level],
-      'Release' => %w[id tag description project_id created_at updated_at],
-      'ProjectMember' => %w[id access_level source_id source_type user_id notification_level type created_at updated_at created_by_id invite_email invite_token invite_accepted_at requested_at expires_at],
-      'User' => %w[id username email],
-      'MergeRequest' => %w[id target_branch source_branch source_project_id author_id assignee_id title created_at updated_at state merge_status target_project_id iid description position locked_at updated_by_id merge_error merge_params merge_when_build_succeeds merge_user_id merge_commit_sha deleted_at in_progress_merge_commit_sha lock_version milestone_id approvals_before_merge rebase_commit_sha],
-      'MergeRequestDiff' => %w[id state st_commits merge_request_id created_at updated_at base_commit_sha real_size head_commit_sha start_commit_sha],
-      'Ci::Pipeline' => %w[id project_id ref sha before_sha push_data created_at updated_at tag yaml_errors committed_at gl_project_id status started_at finished_at duration user_id],
-      'CommitStatus' => %w[id project_id status finished_at trace created_at updated_at started_at runner_id coverage commit_id commands job_id name deploy options allow_failure stage trigger_request_id stage_idx tag ref user_id type target_url description artifacts_file gl_project_id artifacts_metadata erased_by_id erased_at artifacts_expire_at environment artifacts_size when yaml_variables queued_at],
-      'Ci::Variable' => %w[id project_id key value encrypted_value encrypted_value_salt encrypted_value_iv gl_project_id],
-      'Ci::Trigger' => %w[id token project_id deleted_at created_at updated_at gl_project_id],
-      'DeployKey' => %w[id user_id created_at updated_at key title type fingerprint public],
-      'Service' => %w[id type title project_id created_at updated_at active properties template push_events issues_events merge_requests_events tag_push_events note_events pipeline_events build_events category default wiki_page_events confidential_issues_events],
-      'ProjectHook' => %w[id url project_id created_at updated_at type service_id push_events issues_events merge_requests_events tag_push_events note_events pipeline_events enable_ssl_verification build_events wiki_page_events token group_id confidential_issues_events],
-      'ProtectedBranch' => %w[id project_id name created_at updated_at],
-      'Project' => %w[description issues_enabled merge_requests_enabled wiki_enabled snippets_enabled visibility_level archived],
-      'Author' => %w[name]
-    }
-  end
+  let(:safe_attributes_file) { 'spec/lib/gitlab/import_export/safe_model_attributes.yml' }
+  let(:safe_model_attributes) { YAML.load_file(safe_attributes_file) }
 
   it 'has no new columns' do
     relation_names.each do |relation_name|
@@ -65,11 +41,11 @@ describe 'Attribute configuration', lib: true do
     <<-MSG
       It looks like #{relation_class}, which is exported using the project Import/Export, has new attributes: #{new_attributes.join(',')}
 
-      Please add the attribute(s) to +safe_model_attributes+ in CURRENT_SPEC if you consider this can be exported.
+      Please add the attribute(s) to SAFE_MODEL_ATTRIBUTES if you consider this can be exported.
       Otherwise, please blacklist the attribute(s) in IMPORT_EXPORT_CONFIG by adding it to its correspondent
       model in the +excluded_attributes+ section.
 
-      CURRENT_SPEC: #{__FILE__}
+      SAFE_MODEL_ATTRIBUTES: #{File.expand_path(safe_attributes_file)}
       IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file}
     MSG
   end

spec/lib/gitlab/import_export/model_configuration_spec.rb

@@ -3,17 +3,17 @@ require 'spec_helper'
 # Part of the test security suite for the Import/Export feature
 # Finds if a new model has been added that can potentially be part of the Import/Export
 # If it finds a new model, it will show a +failure_message+ with the options available.
-describe 'Model configuration', lib: true do
+describe 'Import/Export model configuration', lib: true do
   include ConfigurationHelper
 
   let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
-  let(:relation_names) do
+  let(:model_names) do
     names = names_from_tree(config_hash['project_tree'])
 
     # Remove duplicated or add missing models
     # - project is not part of the tree, so it has to be added manually.
     # - milestone, labels have both singular and plural versions in the tree, so remove the duplicates.
-    # - User, Author... Models we do not care about for checking relations
+    # - User, Author... Models we do not care about for checking models
     names.flatten.uniq - ['milestones', 'labels', 'user', 'author'] + ['project']
   end
 
@@ -22,23 +22,21 @@ describe 'Model configuration', lib: true do
   let(:current_models) { setup_models }
 
   it 'has no new models' do
-    relation_names.each do |relation_name|
-      new_models = current_models[relation_name] - all_models[relation_name]
-      expect(new_models).to be_empty, failure_message(relation_name.classify, new_models)
+    model_names.each do |model_name|
+      new_models = current_models[model_name] - all_models[model_name]
+      expect(new_models).to be_empty, failure_message(model_name.classify, new_models)
     end
   end
 
-  # List of current relations between models, in the format of
+  # List of current models between models, in the format of
   # {model: [model_2, model3], ...}
   def setup_models
     all_models_hash = {}
 
-    relation_names.each do |relation_name|
-      relation_class = relation_class_for_name(relation_name)
+    model_names.each do |model_name|
+      model_class = relation_class_for_name(model_name)
 
-      all_models_hash[relation_name] = relation_class.reflect_on_all_associations.map do |association|
-        association.name.to_s
-      end
+      all_models_hash[model_name] = associations_for(model_class)
     end
 
     all_models_hash
@@ -49,8 +47,8 @@ describe 'Model configuration', lib: true do
       New model(s) <#{new_models.join(',')}> have been added, related to #{parent_model_name}, which is exported by
       the Import/Export feature.
 
-      If you don't think this should be exported, please add it to MODELS_JSON, inside the #{parent_model_name} hash.
-      If you think we should export this new model, please add it to IMPORT_EXPORT_CONFIG and to MODELS_JSON.
+      If you think this model should be included in the export, please add it to IMPORT_EXPORT_CONFIG.
+      Definitely add it to MODELS_JSON to signal that you've handled this error and to prevent it from showing up in the future.
 
       MODELS_JSON: #{File.expand_path(all_models_yml)}
       IMPORT_EXPORT_CONFIG: #{Gitlab::ImportExport.config_file}

spec/lib/gitlab/import_export/safe_model_attributes.yml

+---
+Issue:
+- id
+- title
+- assignee_id
+- author_id
+- project_id
+- created_at
+- updated_at
+- position
+- branch_name
+- description
+- state
+- iid
+- updated_by_id
+- confidential
+- deleted_at
+- due_date
+- moved_to_id
+- lock_version
+- milestone_id
+- weight
+Event:
+- id
+- target_type
+- target_id
+- title
+- data
+- project_id
+- created_at
+- updated_at
+- action
+- author_id
+Note:
+- id
+- note
+- noteable_type
+- author_id
+- created_at
+- updated_at
+- project_id
+- attachment
+- line_code
+- commit_id
+- noteable_id
+- system
+- st_diff
+- updated_by_id
+- type
+- position
+- original_position
+- resolved_at
+- resolved_by_id
+- discussion_id
+- original_discussion_id
+LabelLink:
+- id
+- label_id
+- target_id
+- target_type
+- created_at
+- updated_at
+Label:
+- id
+- title
+- color
+- project_id
+- created_at
+- updated_at
+- template
+- description
+- priority
+Milestone:
+- id
+- title
+- project_id
+- description
+- due_date
+- created_at
+- updated_at
+- state
+- iid
+ProjectSnippet:
+- id
+- title
+- content
+- author_id
+- project_id
+- created_at
+- updated_at
+- file_name
+- type
+- visibility_level
+Release:
+- id
+- tag
+- description
+- project_id
+- created_at
+- updated_at
+ProjectMember:
+- id
+- access_level
+- source_id
+- source_type
+- user_id
+- notification_level
+- type
+- created_at
+- updated_at
+- created_by_id
+- invite_email
+- invite_token
+- invite_accepted_at
+- requested_at
+- expires_at
+User:
+- id
+- username
+- email
+MergeRequest:
+- id
+- target_branch
+- source_branch
+- source_project_id
+- author_id
+- assignee_id
+- title
+- created_at
+- updated_at
+- state
+- merge_status
+- target_project_id
+- iid
+- description
+- position
+- locked_at
+- updated_by_id
+- merge_error
+- merge_params
+- merge_when_build_succeeds
+- merge_user_id
+- merge_commit_sha
+- deleted_at
+- in_progress_merge_commit_sha
+- lock_version
+- milestone_id
+- approvals_before_merge
+- rebase_commit_sha
+MergeRequestDiff:
+- id
+- state
+- st_commits
+- merge_request_id
+- created_at
+- updated_at
+- base_commit_sha
+- real_size
+- head_commit_sha
+- start_commit_sha
+Ci::Pipeline:
+- id
+- project_id
+- ref
+- sha
+- before_sha
+- push_data
+- created_at
+- updated_at
+- tag
+- yaml_errors
+- committed_at
+- gl_project_id
+- status
+- started_at
+- finished_at
+- duration
+- user_id
+CommitStatus:
+- id
+- project_id
+- status
+- finished_at
+- trace
+- created_at
+- updated_at
+- started_at
+- runner_id
+- coverage
+- commit_id
+- commands
+- job_id
+- name
+- deploy
+- options
+- allow_failure
+- stage
+- trigger_request_id
+- stage_idx
+- tag
+- ref
+- user_id
+- type
+- target_url
+- description
+- artifacts_file
+- gl_project_id
+- artifacts_metadata
+- erased_by_id
+- erased_at
+- artifacts_expire_at
+- environment
+- artifacts_size
+- when
+- yaml_variables
+- queued_at
+Ci::Variable:
+- id
+- project_id
+- key
+- value
+- encrypted_value
+- encrypted_value_salt
+- encrypted_value_iv
+- gl_project_id
+Ci::Trigger:
+- id
+- token
+- project_id
+- deleted_at
+- created_at
+- updated_at
+- gl_project_id
+DeployKey:
+- id
+- user_id
+- created_at
+- updated_at
+- key
+- title
+- type
+- fingerprint
+- public
+Service:
+- id
+- type
+- title
+- project_id
+- created_at
+- updated_at
+- active
+- properties
+- template
+- push_events
+- issues_events
+- merge_requests_events
+- tag_push_events
+- note_events
+- pipeline_events
+- build_events
+- category
+- default
+- wiki_page_events
+- confidential_issues_events
+ProjectHook:
+- id
+- url
+- project_id
+- created_at
+- updated_at
+- type
+- service_id
+- push_events
+- issues_events
+- merge_requests_events
+- tag_push_events
+- note_events
+- pipeline_events
+- enable_ssl_verification
+- build_events
+- wiki_page_events
+- token
+- group_id
+- confidential_issues_events
+ProtectedBranch:
+- id
+- project_id
+- name
+- created_at
+- updated_at
+Project:
+- description
+- issues_enabled
+- merge_requests_enabled
+- wiki_enabled
+- snippets_enabled
+- visibility_level
+- archived
+Author:
+- name

spec/support/import_export/configuration_helper.rb

@@ -22,4 +22,10 @@ module ConfigurationHelper
 
     attributes
   end
+
+  def associations_for(model_class)
+    model_class.reflect_on_all_associations.map do |association|
+      association.name.to_s
+    end
+  end
 end