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

@@ -5,31 +5,23 @@
 #  or later. See the COPYING file at the top-level directory or at
 #  https://github.com/hitobito/hitobito.
 
-class People::CreateRolesJob < RecurringJob
+class People::CreateRolesJob < People::RolesBaseJob
 
-  run_every 15.minutes
+  run_every 1.hour
 
   Error = Class.new(StandardError)
 
-  def perform
-    future_roles.find_each { |role| convert(role) }
+  def perform_internal
+    future_roles.find_each do |role|
+      with_handled_exception(role) do
+        role.convert!
+      end
+    end
   end
 
   private
 
   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)
-  end
-
-  def notify(message)
-    Raven.capture_exception(Error.new(message))
-  end
 end

app/jobs/people/destroy_roles_job.rb

@@ -5,11 +5,22 @@
 #  or later. See the COPYING file at the top-level directory or at
 #  https://github.com/hitobito/hitobito.
 #
-class People::DestroyRolesJob < RecurringJob
-  run_every 15.minutes
+class People::DestroyRolesJob < People::RolesBaseJob
+  run_every 1.hour
+
+  Error = Class.new(StandardError)
 
   def perform_internal
-    roles = Role.where('delete_on <= ?', Time.zone.today)
-    roles.find_each(&:destroy!)
+    obsolete_roles.find_each do |role|
+      with_handled_exception(role) do
+        role.destroy!
+      end
+    end
+  end
+
+  private
+
+  def obsolete_roles
+    Role.where('delete_on <= ?', Time.zone.today).order(:delete_on)
   end
 end

app/jobs/people/roles_base_job.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+#  Copyright (c) 2023, Schweizer Alpen-Club. This file is part of
+#  hitobito 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.
+#
+class People::RolesBaseJob < RecurringJob
+
+  run_every 1.hour
+
+  private
+
+  def with_handled_exception(role)
+    yield role
+  rescue => e
+    notify("#{e.message} - #{role.class}(#{role.id})")
+  end
+
+  def reschedule
+    run_at = interval.from_now.beginning_of_hour
+    enqueue!(run_at: run_at, priority: 5) unless others_scheduled?
+  end
+
+  def notify(message)
+    Raven.capture_exception(self.class.const_get('Error').new(message))
+  end
+end

app/models/role.rb

@@ -152,6 +152,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
@@ -200,6 +201,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
@@ -267,6 +272,10 @@ def prevent_changes
   end
 
   def reset_person_minimized_at
-    person&.update!(minimized_at: nil)
+    person&.update_attribute(:minimized_at, nil)
+  end
+
+  def formatted_delete_date
+    [I18n.t('global.until'), I18n.l(delete_on)].join(' ')
   end
 end

config/locales/views.de.yml

@@ -1718,6 +1718,12 @@ de:
     destroy:
       foreign_key_error: Es gibt Personen, die diesen Eintrittsgrund verwenden, löschen ist daher nicht möglich.
 
+  role_decorator:
+    outdated_future_role: Die Rolle konnte nicht wie geplant per %{date} aktiviert werden. Falls
+      das Speichern der Rolle diese nicht aktiviert, wende dich bitte an den Support.
+    outdated_deleted_role: Die Rolle konnte nicht wie geplant am %{date} terminiert werden. Falls
+      das Speichern der Rolle diese nicht terminiert, wende dich bitte an den Support.
+
   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,39 @@
         end
       end
     end
+  end
+
+  describe 'GET edit' do
+    before { role } # create it
+    let(:page) { Capybara::Node::Simple.new(response.body) }
+    let(:today) { Time.zone.today }
+    let(:today_localized) { I18n.l(today) }
 
+    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).to have_css '#flash', text: ''
+    end
+
+    it 'renders flash message for outedated deleted role' 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: 'Die Rolle konnte nicht ' \
+                               "wie geplant am #{today_localized} terminiert werden")
+    end
+
+    it 'renders flash message for outedated future role' do
+      Role.where(id: role.id).update_all(type: FutureRole.sti_name, convert_to: role.type, convert_on: today)
+      get :edit, params: { group_id: group.id, id: role.id }
+      expect(page).to have_css('#flash .alert.alert-error', text: 'Die Rolle konnte nicht wie ' \
+                               "geplant per #{today_localized} aktiviert werden")
+    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: "" } }
 
@@ -328,7 +354,7 @@
           put :update, params: { group_id: group.id, id: role.id, role: { delete_on: yesterday } }
         end.to change { Role.count }.by(-1)
         expect(response).to redirect_to(person_path(person))
-        expect(flash[:notice]).to eq "Rolle <i>Member</i> für <i>#{person}</i> in <i>TopGroup</i> wurde erfolgreich gelöscht."
+        expect(flash[:notice]).to eq "Rolle <i>Member (Bis #{yesterday.strftime('%d.%m.%Y')})</i> für <i>#{person}</i> in <i>TopGroup</i> wurde erfolgreich gelöscht."
       end
 
       it 'renders edit and error messages if destroy does not succeed' do
@@ -337,7 +363,7 @@
           put :update, params: { group_id: group.id, id: role.id, role: { delete_on: yesterday } }
         end.not_to change { Role.count }
         expect(response).to render_template('edit')
-        expect(flash.now[:alert]).to eq "Rolle <i>Member</i> für <i>#{person}</i> in <i>TopGroup</i> konnte nicht gelöscht werden."
+        expect(flash.now[:alert]).to eq "Rolle <i>Member (Bis #{yesterday.strftime('%d.%m.%Y')})</i> für <i>#{person}</i> in <i>TopGroup</i> konnte nicht gelöscht werden."
       end
     end
 

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 'Die Rolle konnte nicht wie geplant am 13.11.2023 terminiert werden. Falls das Speichern der Rolle diese nicht terminiert, wende dich bitte an den Support.'
+      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 'Die Rolle konnte nicht wie geplant per 13.11.2023 aktiviert werden. Falls das Speichern der Rolle diese nicht aktiviert, wende dich bitte an den Support.'
+      end
+    end
+  end
+end

spec/features/roles/future_roles_spec.rb

@@ -15,6 +15,7 @@
   let(:bottom_layer) { groups(:bottom_layer_one) }
   let(:role_type) { Group::TopGroup::Member.sti_name }
   let(:tomorrow) { Time.zone.tomorrow }
+  let(:yesterday) { Time.zone.yesterday }
 
   let(:current_roles_aside) { 'section:nth-of-type(2)' }
   let(:future_roles_aside) { 'section:nth-of-type(3)' }
@@ -189,5 +190,14 @@ def deselect_role
       first(:button, 'Speichern').click
       expect(page).to have_css '.alert-error', text: 'Von kann nicht später als heute sein'
     end
+
+    it 'saving outdated future role converts role' do
+      role = create_future_role.tap { |r| r.update_columns(convert_on: yesterday) }
+      visit edit_group_role_path(group_id: top_group.id, id: role.id, locale: :de)
+      expect(page).to have_field 'Von', with: yesterday.strftime('%d.%m.%Y')
+      first(:button, 'Speichern').click
+      expect(page).not_to have_css '.roles', text: 'TopGroup / Member'
+      expect(page).not_to have_css 'h2', text: 'Zukünftige Rollen'
+    end
   end
 end

spec/features/roles_controller_spec.rb

@@ -27,14 +27,22 @@
       visit edit_group_role_path(group_id: role.group_id, id: role.id)
       fill_in 'Bis', with: tomorrow
       all('form .btn-toolbar').first.click_button 'Speichern'
-      expect(page).to have_content 'Rolle Member für Bottom Member in Bottom One wurde erfolgreich aktualisiert'
+      expect(page).to have_content "Rolle Member (Bis #{tomorrow.strftime('%d.%m.%Y')}) für Bottom Member in Bottom One wurde erfolgreich aktualisiert"
       expect(role.reload.delete_on).to eq tomorrow
     end
 
     it 'shows delete_on date' do
       role.update(delete_on: tomorrow)
       visit edit_group_role_path(group_id: role.group_id, id: role.id)
-      expect(page).to have_field 'Bis', with: tomorrow.strftime("%d.%m.%Y")
+      expect(page).to have_field 'Bis', with: tomorrow.strftime('%d.%m.%Y')
+    end
+
+    it 'saving outdated role deletes role' do
+      role.update_columns(delete_on: Time.zone.yesterday)
+      visit edit_group_role_path(group_id: role.group_id, id: role.id)
+      expect do
+        all('form .btn-toolbar').first.click_button 'Speichern'
+      end.to change { people(:bottom_member).roles.count }.by(-1)
     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 1.hour.from_now.beginning_of_hour
   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 }
@@ -37,6 +39,14 @@ def create_future_role(attrs = {})
       .and change { person.roles.where(type: FutureRole.sti_name).count }.by(-1)
   end
 
+  it 'destroys and creates role even if person is invalid' do
+    person.update_columns(first_name: nil, last_name: nil, email: nil)
+    expect(person).not_to be_valid
+    create_future_role(convert_on: Time.zone.today)
+    expect { job.perform }.to change { person.roles.where(type: role_type).count }.by(1)
+      .and change { person.roles.where(type: FutureRole.sti_name).count }.by(-1)
+  end
+
   it 'destroys and creates multiples' do
     create_future_role(convert_on: Time.zone.today)
     create_future_role(convert_on: Time.zone.today, person: people(:top_leader))
@@ -59,4 +69,3 @@ def create_future_role(attrs = {})
       .and change { Role.where(type: FutureRole.sti_name).count }.by(-1)
   end
 end
-