Merge pull request #4435 from betagouv/fix_active_admin_for_real

Corrige le bug qui maintenait les administrateurs dans un état inactif

app/controllers/application_controller.rb

@@ -250,7 +250,7 @@ class ApplicationController < ActionController::Base
         payload: {
           DS_SIGN_IN_COUNT: current_user&.sign_in_count,
           DS_CREATED_AT: current_administrateur&.created_at,
-          DS_ACTIVE: current_administrateur&.active,
+          DS_ACTIVE: current_administrateur&.active?,
           DS_ID: current_administrateur&.id,
           DS_GESTIONNAIRE_ID: current_instructeur&.id,
           DS_ROLES: current_user_roles

app/models/administrateur.rb

@@ -56,7 +56,7 @@ class Administrateur < ApplicationRecord
   end
 
   def invitation_expired?
-    !active && !user.reset_password_period_valid?
+    !active? && !user.reset_password_period_valid?
   end
 
   def self.reset_password(reset_password_token, password)
@@ -66,10 +66,6 @@ class Administrateur < ApplicationRecord
       reset_password_token: reset_password_token
     })
 
-    if administrateur && administrateur.errors.empty?
-      administrateur.update_column(:active, true)
-    end
-
     administrateur
   end
 
@@ -84,4 +80,8 @@ class Administrateur < ApplicationRecord
   def can_be_deleted?
     dossiers.state_instruction_commencee.none? && procedures.none?
   end
+
+  def active?
+    user.last_sign_in_at.present?
+  end
 end

app/services/administrateur_usage_statistics_service.rb

@@ -29,7 +29,7 @@ class AdministrateurUsageStatisticsService
     result = {
       ds_sign_in_count: administrateur.user.sign_in_count,
       ds_created_at: administrateur.created_at,
-      ds_active: administrateur.active,
+      ds_active: administrateur.active?,
       ds_id: administrateur.id,
       nb_services: nb_services_by_administrateur_id[administrateur.id],
       nb_instructeurs: nb_instructeurs_by_administrateur_id[administrateur.id],

db/migrate/20191024092136_drop_active_column_from_administrateurs.rb

@@ -0,0 +1,5 @@
+class DropActiveColumnFromAdministrateurs < ActiveRecord::Migration[5.2]
+  def change
+    remove_column :administrateurs, :active
+  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: 2019_10_23_183120) do
+ActiveRecord::Schema.define(version: 2019_10_24_092136) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -51,7 +51,6 @@ ActiveRecord::Schema.define(version: 2019_10_23_183120) do
     t.string "email", default: "", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.boolean "active", default: false
     t.string "encrypted_token"
     t.index ["email"], name: "index_administrateurs_on_email", unique: true
   end

spec/features/admin/admin_creation_spec.rb

@@ -3,6 +3,7 @@ require 'spec_helper'
 feature 'As an administrateur', js: true do
   let(:administration) { create(:administration) }
   let(:admin_email) { 'new_admin@gouv.fr' }
+  let(:new_admin) { Administrateur.find_by(email: admin_email) }
 
   before do
     perform_enqueued_jobs do
@@ -11,6 +12,8 @@ feature 'As an administrateur', js: true do
   end
 
   scenario 'I can register' do
+    expect(new_admin.reload.active?).to be(false)
+
     confirmation_email = open_email(admin_email)
     token_params = confirmation_email.body.match(/token=[^"]+/)
 
@@ -20,5 +23,7 @@ feature 'As an administrateur', js: true do
     click_button 'Continuer'
 
     expect(page).to have_content 'Mot de passe enregistré'
+
+    expect(new_admin.reload.active?).to be(true)
   end
 end

spec/jobs/administrateurs/activate_before_expiration_job_spec.rb

@@ -2,7 +2,8 @@ require 'rails_helper'
 
 RSpec.describe Administrateurs::ActivateBeforeExpirationJob, type: :job do
   describe 'perform' do
-    let(:administrateur) { create(:administrateur, active: active) }
+    let(:administrateur) { create(:administrateur) }
+    let(:user) { administrateur.user }
     let(:mailer_double) { double('mailer', deliver_later: true) }
 
     subject { Administrateurs::ActivateBeforeExpirationJob.perform_now }
@@ -16,7 +17,7 @@ RSpec.describe Administrateurs::ActivateBeforeExpirationJob, type: :job do
     after { Timecop.return }
 
     context "with an inactive administrateur" do
-      let(:active) { false }
+      before { user.update(last_sign_in_at: nil) }
 
       context "created now" do
         before { subject }
@@ -43,7 +44,7 @@ RSpec.describe Administrateurs::ActivateBeforeExpirationJob, type: :job do
     end
 
     context "with an active administrateur" do
-      let(:active) { true }
+      before { user.update(last_sign_in_at: Time.zone.now) }
 
       context "created now" do
         before { subject }

spec/lib/tasks/deployment/20191007124230_update_admin_last_sign_in_at.rake_spec.rb

@@ -1,27 +0,0 @@
-describe '20191007124230_update_admin_last_sign_in_at.rake' do
-  let(:rake_task) { Rake::Task['after_party:update_admin_last_sign_in_at'] }
-
-  subject { rake_task.invoke }
-  after { rake_task.reenable }
-
-  context 'with 2 administrateurs' do
-    let!(:admin) { create(:administrateur, active: true) }
-    let(:user) { admin.user }
-    let!(:admin2) { create(:administrateur, active: false) }
-    let(:user2) { admin2.user }
-
-    before do
-    end
-
-    it do
-      expect(admin.active).to be true
-      expect(user.last_sign_in_at).to be_nil
-      expect(admin.updated_at).not_to be_nil
-
-      subject
-
-      expect(user.reload.last_sign_in_at).to eq(admin.reload.updated_at)
-      expect(user2.reload.last_sign_in_at).to be_nil
-    end
-  end
-end

spec/models/administrateur_spec.rb

@@ -50,4 +50,22 @@ describe Administrateur, type: :model do
   #     it { expect(subject).to eq([]) }
   #   end
   # end
+
+  describe '#active?' do
+    let!(:administrateur) { create(:administrateur) }
+
+    subject { administrateur.active? }
+
+    context 'when the user has never signed in' do
+      before { administrateur.user.update(last_sign_in_at: nil) }
+
+      it { is_expected.to be false }
+    end
+
+    context 'when the user has already signed in' do
+      before { administrateur.user.update(last_sign_in_at: Time.zone.now) }
+
+      it { is_expected.to be true }
+    end
+  end
 end

spec/services/administrateur_usage_statistics_service_spec.rb

@@ -39,7 +39,6 @@ describe AdministrateurUsageStatisticsService do
       let(:administrateur) do
         create(:administrateur,
           user: create(:user, sign_in_count: 17, current_sign_in_at: Time.zone.local(2019, 3, 7), last_sign_in_at: Time.zone.local(2019, 2, 27)),
-          active: true,
           services: [create(:service)],
           instructeurs: [create(:instructeur)])
       end