Merge branch '36782-rename-approval-status' into 'master'

Rename `approval_status` to `classification`

See merge request gitlab-org/gitlab!20414

db/migrate/20191119220425_rename_software_license_policies_approval_status_to_classification.rb

+# frozen_string_literal: true
+
+class RenameSoftwareLicensePoliciesApprovalStatusToClassification < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    rename_column_concurrently :software_license_policies, :approval_status, :classification
+  end
+
+  def down
+    undo_rename_column_concurrently :software_license_policies, :approval_status, :classification
+  end
+end

db/post_migrate/20191119221041_cleanup_software_license_policies_classification_rename.rb

+# frozen_string_literal: true
+
+class CleanupSoftwareLicensePoliciesClassificationRename < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    cleanup_concurrent_column_rename :software_license_policies, :approval_status, :classification
+  end
+
+  def down
+    undo_cleanup_concurrent_column_rename :software_license_policies, :approval_status, :classification
+  end
+end

db/schema.rb

@@ -3673,7 +3673,7 @@ ActiveRecord::Schema.define(version: 2019_12_02_031812) do
   create_table "software_license_policies", id: :serial, force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "software_license_id", null: false
-    t.integer "approval_status", default: 0, null: false
+    t.integer "classification", default: 0, null: false
     t.index ["project_id", "software_license_id"], name: "index_software_license_policies_unique_per_project", unique: true
     t.index ["software_license_id"], name: "index_software_license_policies_on_software_license_id"
   end

ee/app/models/sca/license_policy.rb

@@ -10,7 +10,7 @@ module SCA
       @url = reported_license&.url
       @dependencies = reported_license&.dependencies || []
       @spdx_identifier = software_policy&.spdx_identifier || reported_license&.id
-      @classification = software_policy&.approval_status || 'unclassified'
+      @classification = software_policy&.classification || 'unclassified'
     end
   end
 end

ee/app/models/software_license.rb

@@ -15,9 +15,9 @@ class SoftwareLicense < ApplicationRecord
   scope :unknown, -> { where(spdx_identifier: nil) }
   scope :grouped_by_name, -> { group(:name) }
 
-  def self.create_policy_for!(project:, name:, approval_status:)
+  def self.create_policy_for!(project:, name:, classification:)
     project.software_license_policies.create!(
-      approval_status: approval_status,
+      classification: classification,
       software_license: safe_find_or_create_by!(name: name)
     )
   end

ee/app/models/software_license_policy.rb

@@ -13,8 +13,7 @@ class SoftwareLicensePolicy < ApplicationRecord
   belongs_to :software_license, -> { readonly }
   attr_readonly :software_license
 
-  # Licenses must be approved or blacklisted.
-  enum approval_status: {
+  enum classification: {
       blacklisted: 0,
       approved: 1
   }
@@ -24,7 +23,7 @@ class SoftwareLicensePolicy < ApplicationRecord
   validates_presence_of :software_license
 
   validates_presence_of :project
-  validates :approval_status, presence: true
+  validates :classification, presence: true
 
   # A license is unique for its project since it can't be approved and blacklisted.
   validates :software_license, uniqueness: { scope: :project_id }
@@ -46,6 +45,6 @@ class SoftwareLicensePolicy < ApplicationRecord
   delegate :name, :spdx_identifier, to: :software_license
 
   def self.workaround_cache_key
-    pluck(:id, :approval_status).flatten
+    pluck(:id, :classification).flatten
   end
 end

ee/app/serializers/license_scanning_report_license_entity.rb

@@ -10,7 +10,7 @@ class LicenseScanningReportLicenseEntity < Grape::Entity
   expose :url
 
   def classification
-    default = { id: nil, name: value_for(:name), approval_status: 'unclassified' }
+    default = { id: nil, name: value_for(:name), classification: 'unclassified' }
     found = SoftwareLicensePoliciesFinder.new(request&.current_user, request&.project, name: value_for(:name)).find
     ManagedLicenseEntity.represent(found || default)
   end

ee/app/serializers/managed_license_entity.rb

@@ -2,7 +2,7 @@
 
 class ManagedLicenseEntity < Grape::Entity
   expose :id
-  expose :approval_status
+  expose :classification, as: :approval_status
   expose :software_license, merge: true do
     expose :name
   end

ee/app/services/projects/licenses/create_policy_service.rb

@@ -17,9 +17,9 @@ module Projects
       end
 
       def create_policy(software_license, classification)
-        raise error_for(:approval_status, :invalid) unless known?(classification)
+        raise error_for(:classification, :invalid) unless known?(classification)
 
-        policy = project.software_license_policies.create!(software_license: software_license, approval_status: classification)
+        policy = project.software_license_policies.create!(software_license: software_license, classification: classification)
         RefreshLicenseComplianceChecksWorker.perform_async(project.id)
         policy
       end
@@ -29,7 +29,7 @@ module Projects
       end
 
       def known?(classification)
-        SoftwareLicensePolicy.approval_statuses.key?(classification)
+        SoftwareLicensePolicy.classifications.key?(classification)
       end
 
       def error_for(attribute, error)

ee/app/services/projects/licenses/update_policy_service.rb

@@ -32,12 +32,12 @@ module Projects
 
       def classification_error
         errors = ActiveModel::Errors.new(SoftwareLicensePolicy.new)
-        errors.add(:approval_status, :invalid)
+        errors.add(:classification, :invalid)
         error(errors, :unprocessable_entity)
       end
 
       def valid_classification?
-        SoftwareLicensePolicy.approval_statuses.key?(params[:classification])
+        SoftwareLicensePolicy.classifications.key?(params[:classification])
       end
 
       def blacklisted_classification?

ee/app/services/software_license_policies/create_service.rb

@@ -24,7 +24,7 @@ module SoftwareLicensePolicies
       policy = SoftwareLicense.create_policy_for!(
         project: project,
         name: params[:name],
-        approval_status: params[:approval_status]
+        classification: params[:approval_status]
       )
       RefreshLicenseComplianceChecksWorker.perform_async(project.id)
       policy

ee/app/services/software_license_policies/update_service.rb

@@ -7,14 +7,12 @@ module SoftwareLicensePolicies
       super(project, user, params.with_indifferent_access)
     end
 
-    # returns the updated managed license
     def execute(software_license_policy)
       return error("", 403) unless can?(@current_user, :admin_software_license_policy, @project)
-
-      @params = @params.slice(*SoftwareLicensePolicy::FORM_EDITABLE)
+      return success(software_license_policy: software_license_policy) unless params[:approval_status].present?
 
       begin
-        software_license_policy.update(params)
+        software_license_policy.update(classification: params[:approval_status])
         RefreshLicenseComplianceChecksWorker.perform_async(project.id)
       rescue ArgumentError => ex
         return error(ex.message, 400)

ee/changelogs/unreleased/36782-rename-approval-status.yml

+---
+title: Rename software_license_policies.approval_status to software_license_policies.classification
+merge_request: 20414
+author:
+type: changed

ee/lib/ee/api/entities.rb

@@ -865,7 +865,8 @@ module EE
       end
 
       class ManagedLicense < Grape::Entity
-        expose :id, :name, :approval_status
+        expose :id, :name
+        expose :classification, as: :approval_status
       end
 
       class ProjectAlias < Grape::Entity

ee/spec/controllers/projects/security/licenses_controller_spec.rb

@@ -345,7 +345,7 @@ describe Projects::Security::LicensesController do
           end
 
           it { expect(response).to have_http_status(:unprocessable_entity) }
-          it { expect(json).to eq({ "errors" => { "approval_status" => ["is invalid"] } }) }
+          it { expect(json).to eq({ "errors" => { "classification" => ["is invalid"] } }) }
         end
       end
     end

ee/spec/db/schema_support.rb

@@ -31,7 +31,7 @@ module EE
         }.with_indifferent_access.freeze
 
         IGNORED_LIMIT_ENUMS = {
-          'SoftwareLicensePolicy' => %w[approval_status],
+          'SoftwareLicensePolicy' => %w[classification],
           'User' => %w[group_view]
         }.freeze
       end

ee/spec/factories/software_license_policy.rb

@@ -2,20 +2,20 @@
 
 FactoryBot.define do
   factory :software_license_policy, class: SoftwareLicensePolicy do
-    approval_status { 1 }
+    classification { :approved }
     project
     software_license
 
     trait :blacklist do
-      approval_status { :blacklisted }
+      classification { :blacklisted }
     end
 
     trait :allowed do
-      approval_status { :approved }
+      classification { :approved }
     end
 
     trait :denied do
-      approval_status { :blacklisted }
+      classification { :blacklisted }
     end
   end
 end

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -336,7 +336,7 @@ describe ApprovalMergeRequestRule do
       let!(:project_approval_rule) { create(:approval_project_rule, :requires_approval, :license_management, project: project) }
       let(:project) { create(:project) }
       let!(:open_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [open_merge_request]) }
-      let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, approval_status: :blacklisted) }
+      let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, classification: :blacklisted) }
 
       before do
         subject.refresh_required_approvals!(project_approval_rule)

ee/spec/models/merge_request_spec.rb

@@ -490,7 +490,7 @@ describe MergeRequest do
             expect_any_instance_of(Ci::CompareLicenseScanningReportsService)
                 .to receive(:execute).with(base_pipeline, head_pipeline).and_call_original
 
-            expect(subject[:key]).to include(*[license_1.id, license_1.approval_status, license_2.id, license_2.approval_status])
+            expect(subject[:key]).to include(*[license_1.id, license_1.classification, license_2.id, license_2.classification])
           end
         end
 

ee/spec/models/software_license_policy_spec.rb

@@ -9,7 +9,7 @@ describe SoftwareLicensePolicy do
     it { is_expected.to include_module(Presentable) }
     it { is_expected.to validate_presence_of(:software_license) }
     it { is_expected.to validate_presence_of(:project) }
-    it { is_expected.to validate_presence_of(:approval_status) }
+    it { is_expected.to validate_presence_of(:classification) }
     it { is_expected.to validate_uniqueness_of(:software_license).scoped_to(:project_id).with_message(/has already been taken/) }
   end
 

ee/spec/models/software_license_spec.rb

@@ -18,7 +18,7 @@ describe SoftwareLicense do
 
     context 'when a software license with a given name has already been created' do
       let(:mit_license) { create(:software_license, :mit) }
-      let(:result) { subject.create_policy_for!(project: project, name: mit_license.name, approval_status: :approved) }
+      let(:result) { subject.create_policy_for!(project: project, name: mit_license.name, classification: :approved) }
 
       specify { expect(result).to be_persisted }
       specify { expect(result).to be_approved }
@@ -27,7 +27,7 @@ describe SoftwareLicense do
 
     context 'when a software license with a given name has NOT been created' do
       let(:license_name) { SecureRandom.uuid }
-      let(:result) { subject.create_policy_for!(project: project, name: license_name, approval_status: :blacklisted) }
+      let(:result) { subject.create_policy_for!(project: project, name: license_name, classification: :blacklisted) }
 
       specify { expect(result).to be_persisted }
       specify { expect(result).to be_blacklisted }

ee/spec/requests/api/managed_licenses_spec.rb

@@ -38,7 +38,7 @@ describe API::ManagedLicenses do
         expect(json_response).to be_a(Array)
         expect(json_response.first['id']).to eq(software_license_policy.id)
         expect(json_response.first['name']).to eq(software_license_policy.name)
-        expect(json_response.first['approval_status']).to eq(software_license_policy.approval_status)
+        expect(json_response.first['approval_status']).to eq(software_license_policy.classification)
       end
     end
 
@@ -51,7 +51,7 @@ describe API::ManagedLicenses do
         expect(json_response).to be_a(Array)
         expect(json_response.first['id']).to eq(software_license_policy.id)
         expect(json_response.first['name']).to eq(software_license_policy.name)
-        expect(json_response.first['approval_status']).to eq(software_license_policy.approval_status)
+        expect(json_response.first['approval_status']).to eq(software_license_policy.classification)
       end
     end
 
@@ -92,7 +92,7 @@ describe API::ManagedLicenses do
         expect(response).to match_response_schema('software_license_policy', dir: 'ee')
         expect(json_response['id']).to eq(software_license_policy.id)
         expect(json_response['name']).to eq(software_license_policy.name)
-        expect(json_response['approval_status']).to eq(software_license_policy.approval_status)
+        expect(json_response['approval_status']).to eq(software_license_policy.classification)
       end
 
       it 'returns project managed license details using the license name as key' do
@@ -103,7 +103,7 @@ describe API::ManagedLicenses do
         expect(response).to match_response_schema('software_license_policy', dir: 'ee')
         expect(json_response['id']).to eq(software_license_policy.id)
         expect(json_response['name']).to eq(software_license_policy.name)
-        expect(json_response['approval_status']).to eq(software_license_policy.approval_status)
+        expect(json_response['approval_status']).to eq(software_license_policy.classification)
       end
 
       it 'responds with 404 Not Found if requesting non-existing managed license' do
@@ -121,7 +121,7 @@ describe API::ManagedLicenses do
         expect(response).to match_response_schema('software_license_policy', dir: 'ee')
         expect(json_response['id']).to eq(software_license_policy.id)
         expect(json_response['name']).to eq(software_license_policy.name)
-        expect(json_response['approval_status']).to eq(software_license_policy.approval_status)
+        expect(json_response['approval_status']).to eq(software_license_policy.classification)
       end
     end
 
@@ -208,7 +208,7 @@ describe API::ManagedLicenses do
         initial_license = project.software_license_policies.first
         initial_id = initial_license.id
         initial_name = initial_license.name
-        initial_approval_status = initial_license.approval_status
+        initial_classification = initial_license.classification
         patch api("/projects/#{project.id}/managed_licenses/#{software_license_policy.id}", maintainer_user),
           params: { approval_status: 'blacklisted' }
 
@@ -220,15 +220,15 @@ describe API::ManagedLicenses do
         # Check that response is equal to the updated object
         expect(json_response['id']).to eq(initial_id)
         expect(json_response['name']).to eq(updated_software_license_policy.name)
-        expect(json_response['approval_status']).to eq(updated_software_license_policy.approval_status)
+        expect(json_response['approval_status']).to eq(updated_software_license_policy.classification)
 
         # Check that the approval status was updated
-        expect(updated_software_license_policy.approval_status).to eq('blacklisted')
+        expect(updated_software_license_policy.classification).to eq('blacklisted')
 
         # Check that response is equal to the old object except for the approval status
         expect(initial_id).to eq(updated_software_license_policy.id)
         expect(initial_name).to eq(updated_software_license_policy.name)
-        expect(initial_approval_status).not_to eq(updated_software_license_policy.approval_status)
+        expect(initial_classification).not_to eq(updated_software_license_policy.classification)
       end
 
       it 'responds with 404 Not Found if requesting non-existing managed license' do

ee/spec/services/software_license_policies/create_service_spec.rb

@@ -36,7 +36,7 @@ describe SoftwareLicensePolicies::CreateService do
         software_license_policy = project.software_license_policies.last
         expect(software_license_policy).to be_persisted
         expect(software_license_policy.name).to eq(params[:name])
-        expect(software_license_policy.approval_status).to eq('blacklisted')
+        expect(software_license_policy.classification).to eq('blacklisted')
       end
 
       context "when valid parameters are specified" do
@@ -51,7 +51,7 @@ describe SoftwareLicensePolicies::CreateService do
         specify { expect(result[:software_license_policy]).to be_present }
         specify { expect(result[:software_license_policy]).to be_persisted }
         specify { expect(result[:software_license_policy].name).to eql(params[:name]) }
-        specify { expect(result[:software_license_policy].approval_status).to eql('blacklisted') }
+        specify { expect(result[:software_license_policy].classification).to eql('blacklisted') }
         specify { expect(RefreshLicenseComplianceChecksWorker).to have_received(:perform_async).with(project.id) }
       end
 

ee/spec/services/software_license_policies/update_service_spec.rb

@@ -15,7 +15,7 @@ describe SoftwareLicensePolicies::UpdateService do
     create(
       :software_license_policy,
       software_license: create(:software_license, name: 'ExamplePL/2.1'),
-      approval_status: 'blacklisted'
+      classification: 'blacklisted'
     )
   end
 
@@ -42,7 +42,7 @@ describe SoftwareLicensePolicies::UpdateService do
           update_software_license_policy(opts)
 
           expect(software_license_policy).to be_valid
-          expect(software_license_policy.approval_status).not_to eq(opts[:approval_status])
+          expect(software_license_policy.classification).not_to eq(opts[:approval_status])
         end
       end
 
@@ -52,7 +52,7 @@ describe SoftwareLicensePolicies::UpdateService do
           update_software_license_policy(opts)
 
           expect(software_license_policy).to be_valid
-          expect(software_license_policy.approval_status).to eq(opts[:approval_status])
+          expect(software_license_policy.classification).to eq(opts[:approval_status])
           expect(RefreshLicenseComplianceChecksWorker).to have_received(:perform_async).with(project.id)
         end
       end
@@ -64,7 +64,7 @@ describe SoftwareLicensePolicies::UpdateService do
           update_software_license_policy(opts)
 
           expect(software_license_policy).to be_valid
-          expect(software_license_policy.approval_status).not_to eq(opts[:approval_status])
+          expect(software_license_policy.classification).not_to eq(opts[:classification])
         end
       end
     end

ee/spec/workers/refresh_license_compliance_checks_worker_spec.rb

@@ -24,7 +24,7 @@ describe RefreshLicenseComplianceChecksWorker do
         context "when a license is blacklisted, that appears in some of the license management reports" do
           let!(:open_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [open_merge_request]) }
           let!(:closed_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [closed_merge_request]) }
-          let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, approval_status: :blacklisted) }
+          let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, classification: :blacklisted) }
           let(:license) { create(:software_license, name: license_report.license_names[0]) }
           let(:license_report) { open_pipeline.license_scanning_report }
 
@@ -39,7 +39,7 @@ describe RefreshLicenseComplianceChecksWorker do
         context "when none of the blacklisted licenses appear in the most recent license management reports" do
           let!(:open_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [open_merge_request]) }
           let!(:closed_pipeline) { create(:ee_ci_pipeline, :success, :with_license_management_report, project: project, merge_requests_as_head_pipeline: [closed_merge_request]) }
-          let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, approval_status: :blacklisted) }
+          let!(:blacklist_policy) { create(:software_license_policy, project: project, software_license: license, classification: :blacklisted) }
           let(:license) { create(:software_license, name: SecureRandom.uuid) }
 
           before do