From dca696882db00bc282b5ff6add4eb91f348e56ba Mon Sep 17 00:00:00 2001 From: Linus Degen Date: Mon, 7 Sep 2026 11:28:00 +0200 Subject: [PATCH] Make employees and absences evaluator tables sortable (65435) --- app/controllers/evaluator_controller.rb | 4 +- app/domain/evaluations/absences_eval.rb | 33 +++++-- app/domain/evaluations/employees_eval.rb | 51 ++++++++-- app/domain/evaluations/evaluation.rb | 16 ++-- app/domain/evaluations/sortable.rb | 27 ++++++ app/models/employee.rb | 2 +- app/views/evaluator/_category.html.haml | 12 ++- test/domain/evaluations/absences_eval_test.rb | 29 ++++++ .../domain/evaluations/employees_eval_test.rb | 93 +++++++++++++++++++ test/integration/evaluator_overview_test.rb | 51 ++++++++++ test/integration/member_test.rb | 2 +- 11 files changed, 292 insertions(+), 28 deletions(-) create mode 100644 app/domain/evaluations/sortable.rb create mode 100644 test/integration/evaluator_overview_test.rb diff --git a/app/controllers/evaluator_controller.rb b/app/controllers/evaluator_controller.rb index 6cfcaf5c7..476386453 100644 --- a/app/controllers/evaluator_controller.rb +++ b/app/controllers/evaluator_controller.rb @@ -132,7 +132,9 @@ def set_management_evaluation @evaluation = case params[:evaluation].downcase when 'clients' then Evaluations::ClientsEval.new - when 'employees' then Evaluations::EmployeesEval.new(params.slice(:department_id, :member_coach_id)) + when 'employees' then Evaluations::EmployeesEval.new( + params.slice(:department_id, :member_coach_id), sort_conditions + ) when 'departments' then Evaluations::DepartmentsEval.new when 'clientworkitems' then Evaluations::ClientWorkItemsEval.new(params[:category_id]) when 'employeeworkitems' then Evaluations::EmployeeWorkItemsEval.new(params[:category_id]) diff --git a/app/domain/evaluations/absences_eval.rb b/app/domain/evaluations/absences_eval.rb index 910ae03e4..1c6821d7b 100644 --- a/app/domain/evaluations/absences_eval.rb +++ b/app/domain/evaluations/absences_eval.rb @@ -9,6 +9,7 @@ module Evaluations class AbsencesEval < Evaluations::Evaluation include EvaluatorHelper include FormatHelper + include Sortable self.sub_evaluation = 'employeeabsences' self.division_column = :employee_id @@ -16,6 +17,8 @@ class AbsencesEval < Evaluations::Evaluation self.absences = true self.detail_columns = detail_columns.reject { |i| i == :billable } self.detail_labels = detail_labels.merge(account: 'Absenz') + self.sortable_division_header = true + self.sortable_period_header = true attr_reader :department_id, :sort_conditions @@ -26,12 +29,11 @@ def initialize(department_id = nil, sort_conditions = nil, **worktime_search_con end def divisions(period = nil, times = nil) - employees_with_absences(period, times).map do |e| - unformatted_vacations = remaining_vacations(e, format: false) - e.remaining_vacations = format_days(unformatted_vacations) - e.sort_col = unformatted_vacations * descending + employees = employees_with_absences(period, times).map do |e| + e.remaining_vacations = format_days(vacations_value(e)) e - end.sort_by(&:sort_col) + end + sort_divisions(employees, times) end def employees_with_absences(period, times) @@ -67,8 +69,25 @@ def division_supplement(_user) private - def descending - sort_conditions && sort_conditions['sort_dir'] == 'desc' ? -1 : 1 + def sort_divisions(employees, times) + case sort_conditions && sort_conditions['sort'] + when 'name' then sort_by_name(employees) + when 'period_hours' then sort_by_value(employees) { |e| period_hours_value(e, times) } + else sort_by_value(employees) { |e| vacations_value(e) } + end + end + + def sort_by_name(employees) + sorted = employees.sort_by { |e| [e.lastname, e.firstname] } + sort_direction == :desc ? sorted.reverse : sorted + end + + def sort_by_value(employees) + employees.sort_by { |e| yield(e) * sort_multiplier } + end + + def vacations_value(employee) + remaining_vacations(employee, format: false) end end end diff --git a/app/domain/evaluations/employees_eval.rb b/app/domain/evaluations/employees_eval.rb index 47595060b..aaec217bf 100644 --- a/app/domain/evaluations/employees_eval.rb +++ b/app/domain/evaluations/employees_eval.rb @@ -7,18 +7,25 @@ module Evaluations class EmployeesEval < Evaluations::Evaluation + include Sortable + self.division_column = 'employee_id' self.sub_evaluation = 'employeeworkitems' self.label = 'Member Zeit' self.total_details = false + self.sortable_division_header = true + self.sortable_period_header = true + + attr_reader :sort_conditions - def initialize(emp_search_conditions = {}) + def initialize(emp_search_conditions = {}, sort_conditions = nil) @department_id = emp_search_conditions[:department_id].to_i @member_coach_id = emp_search_conditions[:member_coach_id].to_i + @sort_conditions = sort_conditions super(Employee) end - def divisions(period = nil, _times = nil) + def divisions(period = nil, times = nil) employees = if period Employee.list else @@ -27,7 +34,7 @@ def divisions(period = nil, _times = nil) employees = employees.where(member_coach_id: @member_coach_id) unless @member_coach_id.zero? employees = employees.where(department_id: @department_id) unless @department_id.zero? - employees + sort_divisions(employees, period, times) end def employee_id @@ -52,12 +59,42 @@ def sum_total_times(period = nil) def division_supplement(_user) { - overtime: { title: 'Überstunden', align: 'right' }, - vacations: { title: 'Ferien', align: 'right' }, + overtime: { title: 'Überstunden', align: 'right', sortable: true }, + vacations: { title: 'Ferien', align: 'right', sortable: true }, overtime_vacations_tooltip: {}, - worktime_commits: { title: 'Freigabe' }, - worktime_reviews: { title: 'Kontrolle' } + worktime_commits: { title: 'Freigabe', sortable: true }, + worktime_reviews: { title: 'Kontrolle', sortable: true } } end + + private + + def sort_divisions(employees, period, times) + case sort_conditions && sort_conditions['sort'] + when 'name' then employees.reorder(lastname: sort_direction, firstname: sort_direction) + when 'worktime_commits' then employees.reorder(committed_worktimes_at: sort_direction) + when 'worktime_reviews' then employees.reorder(reviewed_worktimes_at: sort_direction) + when 'overtime' then sort_by_value(employees) { |e| overtime_value(e, period) } + when 'vacations' then sort_by_value(employees) { |e| vacations_value(e, period) } + when 'period_hours' then sort_by_value(employees) { |e| period_hours_value(e, times) } + else employees + end + end + + # Built as a fresh relation rather than chained onto `employees`, since the latter may + # carry a `distinct` and Postgres rejects an in_order_of ORDER BY on that. + def sort_by_value(employees) + ranked_ids = employees.sort_by { |e| yield(e) * sort_multiplier }.map(&:id) + Employee.where(id: ranked_ids).in_order_of(:id, ranked_ids) + end + + def overtime_value(employee, period) + period ? employee.statistics.overtime(period) : employee.statistics.current_overtime + end + + def vacations_value(employee, period) + date = period&.end_date || Time.zone.today + employee.statistics.remaining_vacations(date.end_of_year) + end end end diff --git a/app/domain/evaluations/evaluation.rb b/app/domain/evaluations/evaluation.rb index 200766212..e27e89962 100644 --- a/app/domain/evaluations/evaluation.rb +++ b/app/domain/evaluations/evaluation.rb @@ -16,7 +16,14 @@ class Evaluation class_attribute :division_method, :division_column, :division_join, :division_planning_join, :sub_evaluation, :sub_work_items_eval, :label, :absences, :total_details, :billable_hours, :planned_hours, :category_ref, :detail_columns, - :detail_labels + :detail_labels, :sortable_division_header, :sortable_period_header + + # Whether the division header column can be sorted. + self.sortable_division_header = false + + # Whether the period column can be sorted. Only offered for a single selected period, + # since with several period columns there'd be no single value to sort by. + self.sortable_period_header = false # The method to send to the category object to retrieve a list of divisions. self.division_method = :list @@ -176,12 +183,7 @@ def header_info_labels_custom # The header name of the division column to be displayed. # Returns the class name of the division objects. def division_header - divs = divisions - if divs.respond_to?(:klass) - divs.klass.model_name.human - else - respond_to?(:division_header) ? division_header : '' - end + divisions.klass.model_name.human end # Returns a two-dimensional Array with helper methods of the evaluator diff --git a/app/domain/evaluations/sortable.rb b/app/domain/evaluations/sortable.rb new file mode 100644 index 000000000..0cbb339b3 --- /dev/null +++ b/app/domain/evaluations/sortable.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +# Copyright (c) 2006-2017, Puzzle ITC GmbH. This file is part of +# PuzzleTime 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/puzzle/puzzletime. + +module Evaluations + # Shared sort_dir handling for Evaluations backed by a sort_conditions hash + # ({'sort' => ..., 'sort_dir' => ...}), e.g. from EvaluatorController#sort_conditions. + module Sortable + def sort_direction + sort_conditions && sort_conditions['sort_dir'] == 'desc' ? :desc : :asc + end + + def sort_multiplier + sort_direction == :desc ? -1 : 1 + end + + private + + # times has exactly one entry: this sort is only offered for a single period. + def period_hours_value(employee, times) + times&.first&.dig(employee.id).to_f + end + end +end diff --git a/app/models/employee.rb b/app/models/employee.rb index e8585a4d2..5fa078ed3 100644 --- a/app/models/employee.rb +++ b/app/models/employee.rb @@ -122,7 +122,7 @@ class Employee < ApplicationRecord } scope :active_employed_current_month, -> { joins(:employments).merge(Employment.active.during(Period.current_month)) } - attr_accessor :remaining_vacations, :sort_col # used for multiabsence and not persisted + attr_accessor :remaining_vacations # used for multiabsence and not persisted # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable diff --git a/app/views/evaluator/_category.html.haml b/app/views/evaluator/_category.html.haml index 0d0a43e05..dca5fe45d 100644 --- a/app/views/evaluator/_category.html.haml +++ b/app/views/evaluator/_category.html.haml @@ -14,16 +14,20 @@ %table#evaluation.table.table-striped.table-condensed.table-hover.table-inline %thead %tr - %th= evaluation.division_header + %th + = evaluation.class.sortable_division_header ? sort_link(:name, evaluation.division_header) : evaluation.division_header -if evaluation.sub_work_items_eval %th - @periods.each do |p| %th.periodHead{colspan: evaluation.class.planned_hours ? 3 : nil} - = p ? p.label : 'Total' + - if evaluation.class.sortable_period_header && @period + = sort_link(:period_hours, p ? p.label : 'Total') + - else + = p ? p.label : 'Total'     - - evaluation.division_supplement(@user).each do |_key, supp| + - evaluation.division_supplement(@user).each do |key, supp| %th - = supp[:sortable] ? sort_link(:vacation, supp[:title]) : supp[:title] + = supp[:sortable] ? sort_link(key, supp[:title]) : supp[:title] - if evaluation.class.planned_hours %tr %th.subhead diff --git a/test/domain/evaluations/absences_eval_test.rb b/test/domain/evaluations/absences_eval_test.rb index 19f3800fb..9c48e2d82 100644 --- a/test/domain/evaluations/absences_eval_test.rb +++ b/test/domain/evaluations/absences_eval_test.rb @@ -84,6 +84,35 @@ def test_absences_detail_pascal assert_count_times 0, 1, 2, 2 end + def test_absences_sorted_by_name + create_absences + + ascending = Evaluations::AbsencesEval.new(nil, { 'sort' => 'name', 'sort_dir' => 'asc' }) + + assert_equal [employees(:mark), employees(:lucien), employees(:pascal)], + ascending.divisions(@period_month, @times) + + descending = Evaluations::AbsencesEval.new(nil, { 'sort' => 'name', 'sort_dir' => 'desc' }) + + assert_equal [employees(:pascal), employees(:lucien), employees(:mark)], + descending.divisions(@period_month, @times) + end + + def test_absences_sorted_by_period_hours + create_absences + times = [{ employees(:pascal).id => 3.0, employees(:lucien).id => 9.0, employees(:mark).id => 18.0 }] + + ascending = Evaluations::AbsencesEval.new(nil, { 'sort' => 'period_hours', 'sort_dir' => 'asc' }) + + assert_equal [employees(:pascal), employees(:lucien), employees(:mark)], + ascending.divisions(@period_month, times) + + descending = Evaluations::AbsencesEval.new(nil, { 'sort' => 'period_hours', 'sort_dir' => 'desc' }) + + assert_equal [employees(:mark), employees(:lucien), employees(:pascal)], + descending.divisions(@period_month, times) + end + def create_absences %i[mark lucien pascal].each do |e| employees(e).employments.create!(start_date: @period_month.start_date, diff --git a/test/domain/evaluations/employees_eval_test.rb b/test/domain/evaluations/employees_eval_test.rb index d47841592..a6021cac9 100644 --- a/test/domain/evaluations/employees_eval_test.rb +++ b/test/domain/evaluations/employees_eval_test.rb @@ -73,5 +73,98 @@ def test_employee_detail_pascal assert_sum_times 3, 3, 5, 6 assert_count_times 1, 1, 2, 3 end + + def test_employees_sorted_by_worktime_commits + stub_employee_relation + employees(:pascal).update!(committed_worktimes_at: Date.new(2020, 6, 1)) + employees(:mark).update!(committed_worktimes_at: Date.new(2021, 6, 1)) + employees(:lucien).update!(committed_worktimes_at: Date.new(2022, 6, 1)) + + ascending = Evaluations::EmployeesEval.new({}, { 'sort' => 'worktime_commits', 'sort_dir' => 'asc' }) + + assert_equal [employees(:pascal), employees(:mark), employees(:lucien)], ascending.divisions(@period_month) + + descending = Evaluations::EmployeesEval.new({}, { 'sort' => 'worktime_commits', 'sort_dir' => 'desc' }) + + assert_equal [employees(:lucien), employees(:mark), employees(:pascal)], descending.divisions(@period_month) + end + + def test_employees_sorted_by_worktime_reviews + stub_employee_relation + employees(:pascal).update!(reviewed_worktimes_at: Date.new(2020, 6, 1)) + employees(:mark).update!(reviewed_worktimes_at: Date.new(2021, 6, 1)) + employees(:lucien).update!(reviewed_worktimes_at: Date.new(2022, 6, 1)) + + ascending = Evaluations::EmployeesEval.new({}, { 'sort' => 'worktime_reviews', 'sort_dir' => 'asc' }) + + assert_equal [employees(:pascal), employees(:mark), employees(:lucien)], ascending.divisions(@period_month) + end + + def test_employees_sorted_by_overtime + stub_employee_objects + employees(:pascal).stubs(:statistics).returns(stub(overtime: 5.0)) + employees(:mark).stubs(:statistics).returns(stub(overtime: -2.0)) + employees(:lucien).stubs(:statistics).returns(stub(overtime: 10.0)) + + ascending = Evaluations::EmployeesEval.new({}, { 'sort' => 'overtime', 'sort_dir' => 'asc' }) + + assert_equal [employees(:mark), employees(:pascal), employees(:lucien)], ascending.divisions(@period_month) + + descending = Evaluations::EmployeesEval.new({}, { 'sort' => 'overtime', 'sort_dir' => 'desc' }) + + assert_equal [employees(:lucien), employees(:pascal), employees(:mark)], descending.divisions(@period_month) + end + + def test_employees_sorted_by_vacations + stub_employee_objects + employees(:pascal).stubs(:statistics).returns(stub(remaining_vacations: 3.0)) + employees(:mark).stubs(:statistics).returns(stub(remaining_vacations: 12.0)) + employees(:lucien).stubs(:statistics).returns(stub(remaining_vacations: 7.0)) + + ascending = Evaluations::EmployeesEval.new({}, { 'sort' => 'vacations', 'sort_dir' => 'asc' }) + + assert_equal [employees(:pascal), employees(:lucien), employees(:mark)], ascending.divisions(@period_month) + end + + def test_employees_sorted_by_period_hours + stub_employee_relation + times = [{ employees(:pascal).id => 3.0, employees(:lucien).id => 9.0, employees(:mark).id => 18.0 }] + + ascending = Evaluations::EmployeesEval.new({}, { 'sort' => 'period_hours', 'sort_dir' => 'asc' }) + + assert_equal [employees(:pascal), employees(:lucien), employees(:mark)], + ascending.divisions(@period_month, times) + + descending = Evaluations::EmployeesEval.new({}, { 'sort' => 'period_hours', 'sort_dir' => 'desc' }) + + assert_equal [employees(:mark), employees(:lucien), employees(:pascal)], + descending.divisions(@period_month, times) + end + + def test_employees_sorted_by_period_hours_defaults_missing_employees_to_zero + stub_employee_relation + # lucien has no entry, i.e. no worktimes at all in the period + times = [{ employees(:pascal).id => 3.0, employees(:mark).id => 1.0 }] + + ascending = Evaluations::EmployeesEval.new({}, { 'sort' => 'period_hours', 'sort_dir' => 'asc' }) + + assert_equal [employees(:lucien), employees(:mark), employees(:pascal)], + ascending.divisions(@period_month, times) + end + + private + + # Restricts Employee.list to a real relation of exactly the 3 employees these + # tests care about, so a real SQL ORDER BY (reorder) can still run on it. + def stub_employee_relation + ids = [employees(:pascal).id, employees(:mark).id, employees(:lucien).id] + Employee.stubs(:list).returns(Employee.where(id: ids)) + end + + # Restricts Employee.list to the exact fixture instances, so a per-instance + # stub (e.g. on #statistics) set on them is still in effect when read back. + def stub_employee_objects + Employee.stubs(:list).returns([employees(:pascal), employees(:mark), employees(:lucien)]) + end end end diff --git a/test/integration/evaluator_overview_test.rb b/test/integration/evaluator_overview_test.rb new file mode 100644 index 000000000..4bdb6909e --- /dev/null +++ b/test/integration/evaluator_overview_test.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +# Copyright (c) 2006-2017, Puzzle ITC GmbH. This file is part of +# PuzzleTime 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/puzzle/puzzletime. + +require 'test_helper' + +# Smoke test for the generic evaluator/_category partial, shared by (almost) every +# evaluation type. It isn't actually rendered by EvaluatorControllerTest, which only +# asserts the chosen template without rendering it. +class EvaluatorOverviewTest < ActionDispatch::IntegrationTest + fixtures :all + + setup { login } + + %w[clients departments employees].each do |evaluation| + test "#{evaluation} overview renders without error" do + visit(evaluator_path(evaluation:)) + + assert_selector('table#evaluation thead th', minimum: 1) + end + end + + test 'only the employees division header is sortable' do + visit(evaluator_path(evaluation: 'clients')) + + assert_no_selector('table#evaluation thead th a') + + visit(evaluator_path(evaluation: 'employees')) + + assert_selector('table#evaluation thead th a', text: 'Member') + end + + # Regression test: sorting by a Ruby-computed column used to crash with SystemStackError. + %w[overtime vacations period_hours].each do |sort_column| + test "employees sorted by #{sort_column} renders without error" do + visit(evaluator_path(evaluation: 'employees', department_id: 0, member_coach_id: 0, + sort: sort_column, sort_dir: 'asc')) + + assert_selector('table#evaluation thead th', minimum: 1) + end + end + + private + + def login + login_as(:mark) + end +end diff --git a/test/integration/member_test.rb b/test/integration/member_test.rb index d4ead0dc0..2fb48acaa 100644 --- a/test/integration/member_test.rb +++ b/test/integration/member_test.rb @@ -48,7 +48,7 @@ class MemberTest < ActionDispatch::IntegrationTest def login login_as(:mark) - visit(evaluator_path(evaluation: 'absences', sort: 'vacation', sort_dir: 'asc')) + visit(evaluator_path(evaluation: 'absences', sort: 'remaining_vacations', sort_dir: 'asc')) end def create_employments