Improve handling of outdated roles

- highlight oudated roles on person page
- flash message when editing outdated roles
- consistent error handling across jobs

refs #2255

app/controllers/roles_controller.rb

@@ -26,6 +26,7 @@ class RolesController < CrudController # rubocop:disable Metrics/ClassLength
 
   before_action :set_person_id, only: [:new] # rubocop:disable Rails/LexicallyScopedActionFilter
   before_action :remember_primary_group, only: [:destroy, :update]
+  before_action :set_outdated_flash_message, only: [:edit]
   after_action :last_primary_group_role_deleted, only: [:destroy, :update]
 
   def create
@@ -294,4 +295,8 @@ def extract_start_at
   def delete_model_param(key)
     model_params&.delete(key).presence
   end
+
+  def set_outdated_flash_message
+    flash.now[:alert] = entry.decorate.outdated_role_title if entry.outdated?
+  end
 end

app/decorators/role_decorator.rb

@@ -9,6 +9,21 @@ class RoleDecorator < ApplicationDecorator
   decorates :role
 
   def for_aside
-    content_tag(:strong, model.to_s)
+    text = content_tag(:strong, model.to_s)
+    return text unless model.outdated?
+
+    safe_join(
+      [helpers.icon(:exclamation_triangle, title: outdated_role_title), text],
+      FormatHelper::EMPTY_STRING
+    )
+  end
+
+  def outdated_role_title
+    case model
+    when FutureRole
+      translate(:outdated_future_role, date: I18n.l(model.convert_on))
+    else
+      translate(:outdated_deleted_role, date: I18n.l(model.delete_on))
+    end
   end
 end

app/jobs/people/create_roles_job.rb

@@ -7,26 +7,28 @@
 
 class People::CreateRolesJob < RecurringJob
 
-  run_every 15.minutes
+  run_every 1.day
 
   Error = Class.new(StandardError)
 
-  def perform
+  def perform_internal
     future_roles.find_each { |role| convert(role) }
   end
 
   private
 
+  def reschedule
+    enqueue!(run_at: Time.zone.tomorrow.midnight, priority: 5) unless others_scheduled?
+  end
+
   def future_roles
     FutureRole.where('convert_on <= :today', today: Time.zone.today).order(:convert_on)
   end
 
   def convert(role)
     role.convert!
   rescue => e
-    message = "#{e.message} - FutureRole(#{role.id})"
-    role.update!(label: message)
-    notify(message)
+    notify("#{e.message} - FutureRole(#{role.id})")
   end
 
   def notify(message)

app/jobs/people/destroy_roles_job.rb

@@ -6,10 +6,32 @@
 #  https://github.com/hitobito/hitobito.
 #
 class People::DestroyRolesJob < RecurringJob
-  run_every 15.minutes
 
-  def perform
-    roles = Role.where('delete_on <= ?', Time.zone.today)
-    roles.find_each(&:destroy!)
+  run_every 1.day
+
+  Error = Class.new(StandardError)
+
+  def perform_internal
+    obsolete_roles.find_each { |role| destroy(role) }
+  end
+
+  private
+
+  def reschedule
+    enqueue!(run_at: Time.zone.tomorrow.midnight, priority: 5) unless others_scheduled?
+  end
+
+  def obsolete_roles
+    Role.where('delete_on <= ?', Time.zone.today).order(:delete_on)
+  end
+
+  def destroy(role)
+    role.destroy!
+  rescue => e
+    notify("#{e.message} - Role(#{role.id})")
+  end
+
+  def notify(message)
+    Raven.capture_exception(Error.new(message))
   end
 end

app/models/role.rb

@@ -151,6 +151,7 @@ def attr_used?(attr)
   def to_s(format = :default)
     model_name = self.class.label
     string = label? ? "#{model_name} (#{label})" : model_name
+    string += " (#{formatted_delete_date})" if delete_on
     if format == :long
       I18n.t('activerecord.attributes.role.string_long', role: string, group: group.to_s)
     else
@@ -199,6 +200,10 @@ def end_on
     delete_on || deleted_at&.to_date
   end
 
+  def outdated?
+    [convert_on, delete_on].compact.any? { |date| date <= Time.zone.today }
+  end
+
   private
 
   def nextcloud_group_details
@@ -264,4 +269,9 @@ def prevent_changes
 
     raise ActiveRecord::ReadOnlyRecord unless new_record? || only_archival
   end
+
+  def formatted_delete_date
+    [I18n.t('global.until'), I18n.l(delete_on)].join(' ')
+  end
+
 end

config/locales/views.de.yml

@@ -1718,6 +1718,10 @@ de:
     destroy:
       foreign_key_error: Es gibt Personen, die diesen Eintrittsgrund verwenden, löschen ist daher nicht möglich.
 
+  role_decorator:
+    outdated_future_role: Zukünftige Rolle konnte seit %{date} nicht erstellt werden.
+    outdated_deleted_role: Terminierte Rolle konnte seit %{date} nicht gelöscht werden.
+
   roles:
     full_entry_label: '%{model_label} <i>%{role}</i> für <i>%{person}</i> in <i>%{group}</i>'
     role_changed: '%{full_entry_label} zu <i>%{new_role}</i> geändert.'

spec/controllers/roles_controller_spec.rb

@@ -270,13 +270,29 @@
         end
       end
     end
+  end
+
+  describe 'GET edit' do
+    before { role } # create it
+    let(:page) { Capybara::Node::Simple.new(response.body) }
+
+    render_views
 
+    it 'renders no flash message if role is not outdated' do
+      get :edit, params: { group_id: group.id, id: role.id }
+      expect(page).not_to have_css '#flash'
+    end
+
+    it 'renders flash message if role is outdated' do
+      role.update_columns(delete_on: Time.zone.today)
+      get :edit, params: { group_id: group.id, id: role.id }
+      expect(page).to have_css('#flash .alert.alert-error', text: 'Terminierte Rolle konnte seit')
+    end
   end
 
   describe 'PUT update' do
     before { role } # create it
 
-
     it 'without type displays error' do
       put :update, params: { group_id: group.id, id: role.id, role: { group_id: group.id, person_id: person.id, type: "" } }
 

spec/decorators/role_decorator_spec.rb

@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+#
+# Copyright (c) 2023, Schweizer Alpen-Club. This file is part of
+# hitobito_sac_cas and licensed under the Affero General Public License version 3
+# or later. See the COPYING file at the top-level directory or at
+# https://github.com/hitobito/hitobito
+
+require 'spec_helper'
+
+describe RoleDecorator, :draper_with_helpers do
+  let(:role) { roles(:top_leader) }
+  let(:today) { Time.zone.local(2023, 11, 13) }
+
+  let(:decorator) { described_class.new(role) }
+
+  describe '#for_aside' do
+    let(:tomorrow) { Time.zone.tomorrow }
+    let(:title) { node.find_css('i.fa.fa-exclamation-triangle')[0].attr('title') }
+
+    subject(:node) { Capybara::Node::Simple.new(decorator.for_aside) }
+
+    around do |example|
+      travel_to(today.midnight) { example.run }
+    end
+
+    it 'wraps role#to_s in strong tag wihtout triangle' do
+      expect(node).to have_css('strong', text: role.to_s)
+      expect(node).not_to have_css('i.fa.fa-exclamation-triangle')
+    end
+
+    context 'role marked for deletion' do
+      it 'does not render triangle if delete_on is in the future' do
+        role.delete_on = tomorrow
+        expect(node).not_to have_css('i.fa.fa-exclamation-triangle')
+      end
+
+      it 'does renders triangle if outdated' do
+        role.delete_on = today
+        expect(node).to have_css('i.fa.fa-exclamation-triangle')
+        expect(node).to have_css('strong', text: role.to_s)
+        expect(title).to eq 'Terminierte Rolle konnte seit 13.11.2023 nicht gelöscht werden.'
+      end
+    end
+
+    context 'role marked for conversion' do
+      let(:group) { groups(:top_group) }
+      let(:role) do
+        Fabricate.build(:future_role, convert_to: group.role_types.first, group: group)
+      end
+
+      it 'does not render triangle if delete_on is in the future' do
+        role.convert_on = tomorrow
+        expect(node).not_to have_css('i.fa.fa-exclamation-triangle')
+      end
+
+      it 'does renders triangle if outdated' do
+        role.convert_on = today
+        expect(node).to have_css('i.fa.fa-exclamation-triangle')
+        expect(node).to have_css('strong', text: role.to_s)
+        expect(title).to eq 'Zukünftige Rolle konnte seit 13.11.2023 nicht erstellt werden.'
+      end
+    end
+  end
+end

spec/jobs/people/create_roles_job_spec.rb

@@ -21,11 +21,13 @@ def create_future_role(attrs = {})
     Fabricate(:future_role, defaults.merge(attrs))
   end
 
-  it 'noops if not scheduled' do
+  it 'noops and reschedules if no future role exists' do
     expect { job.perform }.to not_change { person.roles.count }
+      .and change { Delayed::Job.count }.by(1)
+    expect(Delayed::Job.last.run_at).to eq Time.zone.tomorrow.midnight
   end
 
-  it 'noops if node is scheduled for tomorrow' do
+  it 'noops if role is scheduled for tomorrow' do
     create_future_role(convert_on: Time.zone.tomorrow)
     expect { job.perform }.to not_change { person.roles.where(type: role_type).count }
       .and not_change { person.roles.where(type: FutureRole.sti_name).count }

spec/jobs/people/destroy_roles_job_spec.rb

@@ -16,6 +16,8 @@
 
   it 'noops if not scheduled' do
     expect { job.perform }.to not_change { person.roles.count }
+      .and change { Delayed::Job.count }.by(1)
+    expect(Delayed::Job.last.run_at).to eq Time.zone.tomorrow.midnight
   end
 
   it 'noops if not scheduled in the future' do
@@ -39,11 +41,16 @@
     expect { job.perform }.to change { Role.count }.by(-2)
   end
 
-  it 'raises if any role fails to destroy' do
+  it 'logs errors and continues' do
     role.update!(created_at: 3.days.ago, delete_on: Time.zone.yesterday.to_date)
     roles(:top_leader).update!(delete_on: Time.zone.today.to_date)
-    allow_any_instance_of(Role).to receive(:destroy!).and_raise('ouch')
-    expect { job.perform }.to raise_error 'ouch'
+    allow_any_instance_of(Role).to receive(:destroy!).and_wrap_original do |m|
+      raise 'ouch' if m.receiver == role
+      m.call
+    end
+    expect(Raven).to receive(:capture_exception).with(described_class::Error.new("ouch - Role(#{role.id})"))
+
+    expect { job.perform }.to change { Role.count }.by(-1)
   end
 end
 

spec/models/role_spec.rb

@@ -237,6 +237,55 @@ def build(attrs)
     end
   end
 
+  context '#to_s' do
+    let(:group) { groups(:bottom_layer_one) }
+    let(:date) { Date.new(2023, 11, 15) }
+
+    def build_role(attrs = {})
+      Fabricate.build(Group::BottomLayer::Leader.sti_name, attrs.merge(group: group))
+    end
+
+    it 'includes group specific role type' do
+      expect(build_role.to_s).to eq 'Leader'
+    end
+
+    it 'appends label if set' do
+      expect(build_role(label: 'test').to_s).to eq 'Leader (test)'
+    end
+
+    it 'appends delete_on if set' do
+      expect(build_role(delete_on: date).to_s).to eq 'Leader (Bis 15.11.2023)'
+    end
+
+    it 'combines label and delete_on if both are set' do
+      expect(build_role(label: 'test', delete_on: date).to_s).to eq 'Leader (test) (Bis 15.11.2023)'
+    end
+  end
+
+  context '#outdated?' do
+    let(:group) { groups(:bottom_layer_one) }
+
+    def build_role(attrs = {})
+      Fabricate.build(Group::BottomLayer::Leader.sti_name, attrs.merge(group: group))
+    end
+
+    it 'is not outdated by default' do
+      expect(build_role).not_to be_outdated
+    end
+
+    it 'is outdated if delete_on is today or earlier' do
+      expect(build_role(delete_on: Time.zone.tomorrow)).not_to be_outdated
+      expect(build_role(delete_on: Time.zone.yesterday)).to be_outdated
+      expect(build_role(delete_on: Time.zone.today)).to be_outdated
+    end
+
+    it 'is outdated if convert_on is today or earlier' do
+      expect(build_role(convert_on: Time.zone.tomorrow)).not_to be_outdated
+      expect(build_role(convert_on: Time.zone.yesterday)).to be_outdated
+      expect(build_role(convert_on: Time.zone.today)).to be_outdated
+    end
+  end
+
   context '#available_labels' do
     before { described_class.sweep_available_labels }
     subject { Group::BottomLayer::Leader.available_labels }