Merge pull request #1885 from tactilenews/1884_fix_deactivated_by_use…

…r_logic

Fix contributor deactivated_by_user_id being set on any udpate

ansible/fix_mismatch_deactivated_by_user_id.yml

@@ -0,0 +1,5 @@
+---
+- hosts: webservers
+  become: yes
+  roles:
+    - role: temporary/fix_mismatch_deactivated_by_user_id

ansible/roles/temporary/fix_mismatch_deactivated_by_user_id/main.yml

@@ -0,0 +1,5 @@
+---
+- name: Data manipulation to update Setting channels structure
+  command:
+    cmd: docker compose -f docker-compose.yml -f docker-compose.prod.yml exec app bin/rails contributors:fix_mismatch_deactivated_by_user_id
+    chdir: /home/ansible

app/components/contributor_status_toggle/contributor_status_toggle.html.erb

@@ -19,7 +19,7 @@
         <%= t('.inactive.heading') %>
       <% end %>
 
-      <% if contributor.deactivated_by_user.present? %>
+      <% if contributor.deactivated_by_user.present? && !contributor.deactivated_by_user.admin? %>
         <p>
           <%= t('.inactive.text',
                  name: contributor.name,

app/controllers/admin/contributors_controller.rb

@@ -4,22 +4,39 @@ module Admin
   class ContributorsController < Admin::ApplicationController
     include AdministrateExportable::Exporter
 
+    before_action :set_contributor, only: :update
+
     def update
-      contributor = Contributor.find(update_params[:id])
-      contributor.deactivated_by_admin = !ActiveModel::Type::Boolean.new.cast(update_params[:contributor][:active])
+      toggle_active_state
 
-      if contributor.update(update_params[:contributor])
-        redirect_to admin_contributor_path(contributor), flash: { success: 'Contributor was successfully updated.' }
+      if @contributor.update(update_params[:contributor])
+        redirect_to admin_contributor_path(@contributor), flash: { success: 'Contributor was successfully updated.' }
       else
         render :edit, status: :unprocessable_entity
       end
     end
 
     private
 
+    def set_contributor
+      @contributor = Contributor.find(params[:id])
+    end
+
     def update_params
       params.permit(:id,
-                    contributor: %i[note first_name last_name active])
+                    contributor: %i[note first_name last_name])
+    end
+
+    def toggle_active_state_params
+      params.require(:contributor).permit(:active)
+    end
+
+    def toggle_active_state
+      if ActiveModel::Type::Boolean.new.cast(toggle_active_state_params[:active])
+        @contributor.reactivate!
+      else
+        @contributor.deactivate!(admin: true)
+      end
     end
   end
 end

app/controllers/contributors_controller.rb

@@ -41,7 +41,7 @@ def edit; end
 
   def update
     @contributor.editor_guarantees_data_consent = true
-    @contributor.deactivated_by_user = deactivate_by_user_state
+    toggle_active_state if toggle_active_state_params[:active]
 
     if @contributor.update(contributor_params)
       redirect_to contributor_url, flash: { success: I18n.t('contributor.saved', name: @contributor.name) }
@@ -79,13 +79,25 @@ def contributors_sidebar
                               .with_attached_avatar
   end
 
+  def toggle_active_state
+    if ActiveModel::Type::Boolean.new.cast(toggle_active_state_params[:active])
+      @contributor.reactivate!
+    else
+      @contributor.deactivate!(user_id: current_user.id)
+    end
+  end
+
   def contributors_params
     params.permit(:state, :page, tag_list: [])
   end
 
   def contributor_params
     params.require(:contributor).permit(:note, :first_name, :last_name, :avatar, :email, :threema_id, :phone, :zip_code, :city, :tag_list,
-                                        :active, :additional_email)
+                                        :additional_email)
+  end
+
+  def toggle_active_state_params
+    params.require(:contributor).permit(:active)
   end
 
   def message_params
@@ -133,9 +145,5 @@ def handle_failed_update
     contributor.avatar = old_avatar.blob
   end
 
-  def deactivate_by_user_state
-    ActiveModel::Type::Boolean.new.cast(contributor_params[:active]) ? nil : current_user
-  end
-
   attr_reader :contributor
 end

app/models/contributor.rb

@@ -168,7 +168,7 @@ def recent_replies
   end
 
   def active?
-    deactivated_at.nil?
+    deactivated_at.nil? && unsubscribed_at.nil?
   end
 
   alias active active?
@@ -190,8 +190,19 @@ def avatar_url=(url)
     avatar.attach(io: remote_file_location.open, filename: File.basename(remote_file_location.path))
   end
 
-  def active=(value)
-    self.deactivated_at = ActiveModel::Type::Boolean.new.cast(value) ? nil : Time.current
+  def deactivate!(user_id: nil, admin: false)
+    self.deactivated_by_admin = admin
+    update!(deactivated_at: Time.current, deactivated_by_user_id: user_id)
+  end
+
+  def reactivate!
+    return if active?
+
+    update!(
+      deactivated_at: nil,
+      deactivated_by_user_id: nil,
+      deactivated_by_admin: false
+    )
   end
 
   def data_processing_consent=(value)

lib/tasks/temporary/fix_contributors_deactivated_by_user_id_mismatch.rake

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+namespace :contributors do
+  desc 'Fix mismatch where a contributor can be active, but have been deactivate by a user'
+  task fix_mismatch_deactivated_by_user_id: :environment do
+    ActiveRecord::Base.transaction do
+      Contributor.where(deactivated_at: nil).where.not(deactivated_by_user_id: nil).find_each do |contributor|
+        contributor.deactivated_by_user_id = nil
+        contributor.save!
+        print '.'
+      end
+    end
+  end
+end

spec/components/contributor_status_toggle_spec.rb

@@ -5,8 +5,7 @@
 RSpec.describe ContributorStatusToggle::ContributorStatusToggle, type: :component do
   subject { render_inline(described_class.new(**params)) }
 
-  let(:contributor) { create(:contributor, active: active) }
-  let(:active) { true }
+  let(:contributor) { create(:contributor) }
   let(:deactivated_by) { nil }
   let(:params) { { contributor: contributor, deactivated_by: deactivated_by } }
 
@@ -20,7 +19,7 @@
   end
 
   context 'given an inactive contributor' do
-    let(:active) { false }
+    let!(:contributor) { create(:contributor, :inactive) }
 
     it { should have_css('input[type="hidden"][value="on"]', visible: false) }
     it { should have_css('h2', text: 'Mitglied reaktivieren') }
@@ -34,6 +33,19 @@
       it { should have_css('strong', text: deactivated_by.name) }
     end
 
+    context 'marked inactive by an admin' do
+      let(:deactivated_by) { create(:user, admin: true) }
+      before { contributor.update(deactivated_by_user_id: deactivated_by.id, deactivated_by_admin: true) }
+
+      it 'does not display admin name' do
+        expect(subject).not_to have_css('strong', text: deactivated_by.name)
+      end
+
+      it "displays 'Admin' to make clear it was deactivated by and admin" do
+        expect(subject).to have_css('strong', text: 'Admin')
+      end
+    end
+
     context 'through WhatsApp who requested to unsubscribe' do
       before { contributor.update(whats_app_phone_number: '+49151234567', email: nil, unsubscribed_at: 1.minute.ago) }
 

spec/factories/contributors.rb

@@ -8,6 +8,10 @@
     data_processing_consent { true }
     email { Faker::Internet.email }
 
+    trait :inactive do
+      deactivated_at { Time.current }
+    end
+
     trait :with_an_avatar do
       after(:build) do |contributor|
         contributor.avatar.attach(

spec/models/contributor_spec.rb

@@ -710,6 +710,14 @@
       let(:contributor) { create(:contributor, deactivated_at: 1.day.ago) }
       it { should be(false) }
     end
+
+    describe 'given "unsubscribed_at" timestamp' do
+      let(:contributor) { create(:contributor, unsubscribed_at: 1.day.ago) }
+
+      it 'should return false' do
+        expect(subject).to be(false)
+      end
+    end
   end
 
   describe '.inactive' do
@@ -723,24 +731,42 @@
     end
   end
 
-  describe '.active=' do
-    describe 'given active contributor' do
-      let(:contributor) { create(:contributor, deactivated_at: nil) }
-      describe 'false' do
-        it { expect { contributor.active = false }.to change { contributor.deactivated_at.is_a?(ActiveSupport::TimeWithZone) }.to(true) }
-        it { expect { contributor.active = false }.to change { contributor.active? }.to(false) }
-        it { expect { contributor.active = '0' }.to change { contributor.active? }.to(false) }
-        it { expect { contributor.active = 'off' }.to change { contributor.active? }.to(false) }
+  describe '.deactivate!(user_id:, admin: false)' do
+    describe 'given an active contributor' do
+      subject { contributor.deactivate!(user_id: user.id) }
+      let(:contributor) { create(:contributor) }
+      let(:user) { create(:user) }
+
+      it 'deactivates the contributor' do
+        expect { subject }.to change { contributor.reload.deactivated_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
+      end
+
+      it 'sets the deactivated_by_user_id to user_id' do
+        expect { subject }.to change { contributor.reload.deactivated_by_user_id }.from(nil).to(user.id)
       end
     end
+  end
+
+  describe '.reactivate!' do
+    subject { contributor.reactivate! }
 
-    describe 'given deactivated contributor' do
+    describe 'given a deactivated contributor' do
       let(:contributor) { create(:contributor, deactivated_at: 1.day.ago) }
-      describe 'true' do
-        it { expect { contributor.active = true }.to change { contributor.deactivated_at.is_a?(ActiveSupport::TimeWithZone) }.to(false) }
-        it { expect { contributor.active = true }.to change { contributor.active? }.to(true) }
-        it { expect { contributor.active = '1' }.to change { contributor.active? }.to(true) }
-        it { expect { contributor.active = 'on' }.to change { contributor.active? }.to(true) }
+
+      it 'reactivates the contributor' do
+        expect { subject }.to change { contributor.reload.deactivated_at }.from(kind_of(ActiveSupport::TimeWithZone)).to(nil)
+      end
+    end
+
+    describe 'given a deactivated contributor by a user' do
+      let(:contributor) { create(:contributor, deactivated_at: 1.day.ago, deactivated_by_user: user) }
+      let(:user) { create(:user) }
+
+      it 'reactivates the contributor, keeping attrs in sync' do
+        expect { subject }.to change { contributor.reload.deactivated_at }.from(kind_of(ActiveSupport::TimeWithZone)).to(nil)
+                                                                          .and change {
+                                                                                 contributor.reload.deactivated_by_user_id
+                                                                               }.from(user.id).to(nil)
       end
     end
   end

spec/models/request_spec.rb

@@ -310,9 +310,9 @@
 
     describe 'given contributors who are deactivated' do
       before(:each) do
-        create(:contributor, id: 3, email: 'deactivated@example.org', active: false)
-        create(:contributor, id: 4, email: 'activated@example.org', active: true)
-        create(:contributor, id: 5, telegram_id: 24, active: false)
+        create(:contributor, :inactive, id: 3, email: 'deactivated@example.org')
+        create(:contributor, id: 4, email: 'activated@example.org')
+        create(:contributor, :inactive, id: 5, telegram_id: 24)
       end
 
       it { should change { Message.count }.from(0).to(1) }

spec/requests/contributors_spec.rb

@@ -83,6 +83,10 @@
       expect(contributor.tag_list).to match_array(%w[programmer student])
     end
 
+    it 'does not update the deactivated_by_user_id' do
+      expect { subject.call }.not_to(change { contributor.reload.deactivated_by_user_id })
+    end
+
     context 'removing tags' do
       let(:updated_attrs) do
         { tag_list: 'ops' }

spec/system/contributors/filter_spec.rb

@@ -4,9 +4,9 @@
 
 RSpec.describe 'Filter contributors' do
   let(:user) { create(:user) }
-  let!(:active_contributor) { create(:contributor, active: true, tag_list: ['entwickler']) }
-  let!(:inactive_contributor) { create(:contributor, active: false, tag_list: ['entwickler']) }
-  let!(:another_contributor) { create(:contributor, active: true) }
+  let!(:active_contributor) { create(:contributor, tag_list: ['entwickler']) }
+  let!(:inactive_contributor) { create(:contributor, :inactive, tag_list: ['entwickler']) }
+  let!(:another_contributor) { create(:contributor) }
 
   it 'Editor lists contributors' do
     visit contributors_path(as: user)