Merge pull request #1299 from betagouv/simpler-subjects-names-for-imp…

…orts

Simpler subjects names for imports

app/models/institution.rb

@@ -75,22 +75,6 @@ def available_subjects
       .group_by { |is| is.theme } # Enumerable#group_by maintains ordering
   end
 
-  # Find the one subject that matches the passed label
-  # return nil if there’s an ambiguity
-  def find_institution_subject(label)
-    return nil if label.nil?
-
-    clean_label = label.downcase.strip
-    matches = institutions_subjects.filter do |is|
-      label == is.csv_identifier || clean_label.in?([
-        is.description.downcase.strip,
-        is.subject.label.downcase.strip,
-        is.theme.label.downcase.strip
-      ])
-    end
-    matches.first if matches.count == 1
-  end
-
   ##
   #
   def to_param

app/models/institution_subject.rb

@@ -13,6 +13,7 @@
 #
 #  index_institutions_subjects_on_institution_id  (institution_id)
 #  index_institutions_subjects_on_subject_id      (subject_id)
+#  unique_institution_subject_in_institution      (subject_id,institution_id,description) UNIQUE
 #
 # Foreign Keys
 #
@@ -36,6 +37,18 @@ class InstitutionSubject < ApplicationRecord
   has_many :experts, through: :experts_subjects, inverse_of: :institutions_subjects
   has_many :not_deleted_experts, through: :experts_subjects, inverse_of: :institutions_subjects
 
+  # :institution
+  # Other InstitutionSubjects of the same Institution, and the same Subject.
+  has_many :similar_institutions_subjects, -> (is) { where(subject: is.subject).where.not(id: is.id) },
+           through: :institution, source: :institutions_subjects, inverse_of: :similar_institutions_subjects
+
+  # Validations
+  #
+  # In an Institution, the same subject can only be selected several times`
+  # if the description is different (and present.)
+  validates :subject, uniqueness: { scope: [:institution_id, :description] }
+  validate :validate_description_presence
+
   ## Scopes
   #
   scope :support_subjects, -> do
@@ -53,9 +66,45 @@ def to_s
     description
   end
 
+  ## Name / Description uniqueness
+  #
+  def validate_description_presence
+    # description mustn't be blank if there’s a similar subject
+    if description.blank? && similar_institutions_subjects.present?
+      errors.add(:description, :blank)
+    end
+  end
+
   ## used for serialization in advisors csv
   #
-  def csv_identifier
-    [theme, subject, description.presence].compact.to_csv(col_sep: ':').strip
+  def unique_name
+    if similar_institutions_subjects.present?
+      "#{subject.label}:#{description}" # We know description isn‘t blank, see :validate_description_presence
+    else
+      subject.label
+    end
+  end
+
+  def self.find_with_name(institution, name)
+    return nil if name.nil?
+
+    clean_name = name.downcase.strip
+
+    matches = institution.institutions_subjects.preload(:subject, :theme).filter do |institution_subject|
+      institution_subject.possible_names.include? clean_name
+    end
+
+    # return nil if there’s an ambiguity
+    matches.first if matches.count == 1
+  end
+
+  def possible_names
+    [
+      "#{theme.label}:#{subject.label}:#{description}".downcase.strip,
+      "#{subject.label}:#{description}".downcase.strip,
+      description&.downcase.strip,
+      subject.label.downcase.strip,
+      theme.label.downcase.strip
+    ]
   end
 end

app/models/subject.rb

@@ -15,6 +15,7 @@
 # Indexes
 #
 #  index_subjects_on_archived_at  (archived_at)
+#  index_subjects_on_label        (label) UNIQUE
 #  index_subjects_on_slug         (slug) UNIQUE
 #  index_subjects_on_theme_id     (theme_id)
 #
@@ -38,6 +39,7 @@ class Subject < ApplicationRecord
   ## Validations
   #
   validates :theme, :slug, presence: true
+  validates :label, presence: true, uniqueness: true
   before_validation :compute_slug
   before_save :set_support
 

app/services/csv_export/models/user.rb

@@ -29,11 +29,11 @@ def csv_fields_for_relevant_expert_team
           }
         end
 
-        def csv_fields_for_relevant_expert_subjects(subjects)
-          subjects.map do |institution_subject|
+        def csv_fields_for_relevant_expert_subjects(institutions_subjects)
+          institutions_subjects.map do |institution_subject|
             # We build a hash of <institution subject>: <expert subject>
             # * There can be only one expert_subject for an (expert, institution_subject) pair.
-            title = institution_subject.csv_identifier
+            title = institution_subject.unique_name
             lambda = -> {
               # This block is executed in the context of a User
               # (`self` is a User; See `object.instance_exec(&lambda)` in CsvExportService.)

app/services/csv_import/user_importer.rb

@@ -74,7 +74,7 @@ def build_several_subjects_mapping(headers, other_known_headers)
       @several_subjects_mapping =
         headers
           .without(other_known_headers)
-          .index_with { |header| @institution.find_institution_subject(header) }
+          .index_with { |header| InstitutionSubject.find_with_name(@institution, header) }
           .compact
     end
 
@@ -105,10 +105,10 @@ def one_subject_mapping
     end
 
     def import_one_subject(expert, all_attributes)
-      identifier = all_attributes[Expert.human_attribute_name('subject')]
-      return if identifier.blank?
+      name = all_attributes[Expert.human_attribute_name('subject')]
+      return if name.blank?
 
-      institution_subject = expert.institution.institutions_subjects.find{ |is| is.csv_identifier == identifier }
+      institution_subject = InstitutionSubject.find_with_name(expert.institution, name)
 
       expert.experts_subjects.clear
       expert.experts_subjects.new(institution_subject: institution_subject)

db/migrate/20201002163301_make_subject_label_unique.rb

@@ -0,0 +1,6 @@
+class MakeSubjectLabelUnique < ActiveRecord::Migration[6.0]
+  def change
+    add_index :subjects, :label, unique: true
+    add_index :institutions_subjects, [:subject_id, :institution_id, :description], unique: true, name: 'unique_institution_subject_in_institution'
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_10_02_071333) do
+ActiveRecord::Schema.define(version: 2020_10_02_163301) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -183,11 +183,9 @@
     t.string "intervention_criteria"
     t.bigint "expert_id"
     t.bigint "institution_subject_id"
-    t.integer "role", default: 0, null: false
     t.index ["expert_id", "institution_subject_id"], name: "index_experts_subjects_on_expert_id_and_institution_subject_id", unique: true
     t.index ["expert_id"], name: "index_experts_subjects_on_expert_id"
     t.index ["institution_subject_id"], name: "index_experts_subjects_on_institution_subject_id"
-    t.index ["role"], name: "index_experts_subjects_on_role"
   end
 
   create_table "experts_users", id: false, force: :cascade do |t|
@@ -245,6 +243,7 @@
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.index ["institution_id"], name: "index_institutions_subjects_on_institution_id"
+    t.index ["subject_id", "institution_id", "description"], name: "unique_institution_subject_in_institution", unique: true
     t.index ["subject_id"], name: "index_institutions_subjects_on_subject_id"
   end
 
@@ -305,7 +304,6 @@
     t.bigint "need_id", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.integer "old_status", default: 0, null: false
     t.datetime "taken_care_of_at"
     t.datetime "closed_at"
     t.bigint "expert_id"
@@ -314,7 +312,6 @@
     t.index ["expert_id", "need_id"], name: "index_matches_on_expert_id_and_need_id", unique: true, where: "(expert_id <> NULL::bigint)"
     t.index ["expert_id"], name: "index_matches_on_expert_id"
     t.index ["need_id"], name: "index_matches_on_need_id"
-    t.index ["old_status"], name: "index_matches_on_old_status"
     t.index ["status"], name: "index_matches_on_status"
     t.index ["subject_id"], name: "index_matches_on_subject_id"
   end
@@ -374,6 +371,7 @@
     t.boolean "is_support", default: false
     t.string "slug", null: false
     t.index ["archived_at"], name: "index_subjects_on_archived_at"
+    t.index ["label"], name: "index_subjects_on_label", unique: true
     t.index ["slug"], name: "index_subjects_on_slug", unique: true
     t.index ["theme_id"], name: "index_subjects_on_theme_id"
   end

spec/models/institution_spec.rb

@@ -33,32 +33,4 @@
       end
     end
   end
-
-  describe 'find_institution_subject' do
-    subject { institution.find_institution_subject(label) }
-
-    let(:institution) { create :institution, name: 'The Institution' }
-    let(:theme) { create :theme, label: 'The Theme' }
-    let(:the_subject) { create :subject, label: 'The Subject', theme: theme }
-    let!(:is1) { create :institution_subject, institution: institution, subject: the_subject, description: 'First IS' }
-    let!(:is2) { create :institution_subject, institution: institution, subject: the_subject, description: 'Second IS' }
-
-    context 'label is not found' do
-      let(:label) { 'other' }
-
-      it{ is_expected.to be_nil }
-    end
-
-    context 'label is found and unique' do
-      let(:label) { 'First IS' }
-
-      it{ is_expected.to eq is1 }
-    end
-
-    context 'label is found but not unique' do
-      let(:label) { 'The Subject' }
-
-      it{ is_expected.to be_nil }
-    end
-  end
 end

spec/models/institution_subject_spec.rb

@@ -8,25 +8,94 @@
       is_expected.to belong_to :subject
       is_expected.to belong_to :institution
     end
+
+    describe 'description uniqueness in an institution' do
+      let(:institution) { create :institution }
+      let(:the_subject) { create :subject }
+
+      before do
+        create :institution_subject, institution: institution, subject: the_subject, description: 'FOO'
+      end
+
+      context 'same institution and subject and description' do
+        subject { build :institution_subject, institution: institution, subject: the_subject, description: 'FOO' }
+
+        it { is_expected.not_to be_valid }
+      end
+
+      context 'same institution and subject, no description' do
+        subject { build :institution_subject, institution: institution, subject: the_subject, description: '' }
+
+        it { is_expected.not_to be_valid }
+      end
+
+      context 'same institution and subject, differetn description' do
+        subject { build :institution_subject, institution: institution, subject: the_subject, description: 'BAR' }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'same institution, other subject' do
+        subject { build :institution_subject, institution: institution }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'other institution, same subject' do
+        subject { build :institution_subject, subject: the_subject }
+
+        it { is_expected.to be_valid }
+      end
+    end
   end
 
-  describe 'csv_identifier' do
-    subject { institution_subject.csv_identifier }
+  describe 'unique_name' do
+    subject { institution_subject.unique_name }
 
-    let(:institution_subject) { create :institution_subject, subject: the_subject, description: description }
-    let(:the_subject) { create :subject, theme: theme, label: 'Label of the subject' }
+    let(:institution_subject) { create :institution_subject, institution: institution, subject: the_subject, description: 'First IS' }
+    let!(:other_institution_subject) { create :institution_subject, institution: institution, subject: other_subject, description: 'Second IS' }
+    let(:the_subject) { create :subject, theme: theme, label: 'The Subject' }
+    let(:institution) { create :institution }
     let(:theme) { create :theme, label: 'Label of the theme' }
 
-    context 'with a description' do
-      let(:description) { 'Description détaillée' }
+    context 'with no other similar institution_subject' do
+      let(:other_subject) { create :subject, theme: theme, label: 'Other subject' }
+
+      it{ is_expected.to eq 'The Subject' }
+    end
+
+    context 'with a similar institution_subject' do
+      let(:other_subject) { the_subject }
+
+      it{ is_expected.to eq 'The Subject:First IS' }
+    end
+  end
+
+  describe 'find_with_name' do
+    subject { described_class.find_with_name(institution, label) }
+
+    let(:institution) { create :institution, name: 'The Institution' }
+    let(:theme) { create :theme, label: 'The Theme' }
+    let(:the_subject) { create :subject, label: 'The Subject', theme: theme }
+    let!(:is1) { create :institution_subject, institution: institution, subject: the_subject, description: 'First IS' }
+    let!(:is2) { create :institution_subject, institution: institution, subject: the_subject, description: 'Second IS' }
+
+    context 'label is not found' do
+      let(:label) { 'other' }
+
+      it{ is_expected.to be_nil }
+    end
+
+    context 'label is found and unique' do
+      let(:label) { 'First IS' }
 
-      it{ is_expected.to eq 'Label of the theme:Label of the subject:Description détaillée' }
+      it{ is_expected.to eq is1 }
     end
 
-    context 'with no description' do
-      let(:description) { '' }
+    context 'label is found but not unique' do
+      let(:label) { 'The Subject' }
 
-      it{ is_expected.to eq 'Label of the theme:Label of the subject' }
+      it{ is_expected.to be_nil }
     end
   end
 end

spec/services/csv_export_service_spec.rb

@@ -68,7 +68,7 @@
 
       it do
         csv = <<~CSV
-          Institution,Antenne,Prénom et nom,E-mail,Téléphone,Fonction,Test Theme:Test Subject:Description for institution
+          Institution,Antenne,Prénom et nom,E-mail,Téléphone,Fonction,Test Subject
           Test Institution,Test Antenne,User 1,user@user.com,0123456789,User Role,Intervention criteria
         CSV
         is_expected.to eq csv