Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/assets/images/icons/reports-fill.svg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This icon is not used throughout this PR.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
52 changes: 31 additions & 21 deletions app/controllers/reports_controller.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,33 @@
class ReportsController < ApplicationController
def index
@reports = Report.all
end

def show
@report = Report.find(params[:id])
end

def new; end
def new
start_date = parse_date(params[:start_date])
end_date = parse_date(params[:end_date])

return unless start_date && end_date && start_date <= end_date

@start_date = params[:start_date]
@end_date = params[:end_date]

result = ReportFilterService.new.filter(
start_date: start_date,
end_date: end_date,
project_ids: Array(params[:project_ids]),
subproject_ids: Array(params[:subproject_ids])
)

@projects = result.projects
@selected_project_ids = result.selected_project_ids
@subprojects = result.subprojects
@selected_subproject_ids = result.selected_subproject_ids
end

def edit
@report = Report.find(params[:id])
Expand Down Expand Up @@ -74,28 +98,14 @@ def update
redirect_to report_path(@report), flash: { success: t(".success") }
end

def filter
start_date = parse_date(params[:start_date])
end_date = parse_date(params[:end_date])

if start_date && end_date && start_date <= end_date
@start_date = params[:start_date]
@end_date = params[:end_date]

result = ReportFilterService.new.filter(
start_date: start_date,
end_date: end_date,
project_ids: Array(params[:project_ids]),
subproject_ids: Array(params[:subproject_ids])
)
def destroy
@report = Report.find(params[:id])

@projects = result.projects
@selected_project_ids = result.selected_project_ids
@subprojects = result.subprojects
@selected_subproject_ids = result.selected_subproject_ids
if @report.destroy
redirect_to reports_path, flash: { success: t(".success") }
else
redirect_to reports_path, flash: { error: @report.errors.full_messages.to_sentence }
end

render :new
end

private
Expand Down
28 changes: 28 additions & 0 deletions app/views/reports/index.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<div class="mx-8 my-10">
<div class="flex items-center justify-between mb-4">
<h1 class="text-4xl font-bold"><%= t(".title") %></h1>
<%= render ActionButtonComponent.new(to: new_report_path, icon: "add", colour: :primary, size: :large) do %>
<%= t(".actions.create") %>
<% end %>
</div>
<%= render ContentCardComponent.new do %>
<%= render Shared::IndexTableComponent.new(records: @reports) do |table| %>
<% table.column :start_date, header: t(".columns.start_date") do |report| %>
<%= l(report.start_date) if report.start_date %>
<% end %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the header here. The table component derives it.

<% table.column :end_date, header: t(".columns.end_date") do |report| %>
<%= l(report.end_date) if report.end_date %>
<% end %>
<% table.column :created_at, header: t(".columns.created_at") do |report| %>
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table headers use t(".columns.start_date") / end_date / created_at, but there are no corresponding reports.index.columns.* keys in the locale files (they were not added in this PR). This will display "translation missing" in the UI. Either add those locale keys or omit the explicit header: and let IndexTableComponent fall back to Report.human_attribute_name.

Suggested change
<% table.column :start_date, header: t(".columns.start_date") do |report| %>
<%= l(report.start_date) if report.start_date %>
<% end %>
<% table.column :end_date, header: t(".columns.end_date") do |report| %>
<%= l(report.end_date) if report.end_date %>
<% end %>
<% table.column :created_at, header: t(".columns.created_at") do |report| %>
<% table.column :start_date do |report| %>
<%= l(report.start_date) if report.start_date %>
<% end %>
<% table.column :end_date do |report| %>
<%= l(report.end_date) if report.end_date %>
<% end %>
<% table.column :created_at do |report| %>

Copilot uses AI. Check for mistakes.
<%= l(report.created_at) %>
<% end %>
<% table.column :actions, header: t("shared.index_table_component.actions"), col_size: "90px" do |report| %>
<div class="flex flex-row gap-2 items-center">
<%= render ActionButtonComponent.new(to: report_path(report), icon: "view") %>
<%= render ActionButtonComponent.new(to: edit_report_path(report), icon: "edit", colour: :info) %>
<%= render ActionButtonComponent.new(to: report_path(report), method: :delete, icon: "delete", colour: :error, confirm: t("reports.destroy.confirm")) %>
</div>
<% end %>
<% end %>
<% end %>
</div>
7 changes: 7 additions & 0 deletions config/locales/en/reports.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ en:
create:
invalid: Please provide valid dates and select at least one subproject.
success: Report was successfully generated.
destroy:
confirm: Are you sure you want to delete this report?
success: Report deleted successfully.
edit:
add_aggregated_datum: Add Aggregated Data Box
add_journal: Add Journal
Expand All @@ -18,6 +21,10 @@ en:
save: Save Changes
selected_journals: Selected Journals
title: Edit Report
index:
actions:
create: Create Report
title: Reports
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reports.index translations were added, but the view expects reports.index.columns.start_date/end_date/created_at. Add a columns: block under reports.index (and keep keys consistent with the view), otherwise the index page will show missing-translation placeholders for the column headers.

Suggested change
title: Reports
title: Reports
columns:
start_date: Start Date
end_date: End Date
created_at: Created At

Copilot uses AI. Check for mistakes.
journal_card_component:
author: Author
new:
Expand Down
7 changes: 7 additions & 0 deletions config/locales/es/reports.es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ es:
create:
invalid: Por favor proporcione fechas válidas y seleccione al menos un subproyecto.
success: El reporte fue generado exitosamente.
destroy:
confirm: "¿Está seguro de que desea eliminar este informe?"
success: Informe eliminado exitosamente.
edit:
add_aggregated_datum: Agregar Caja de Datos Agregados
add_journal: Agregar Diario
Expand All @@ -18,6 +21,10 @@ es:
save: Guardar Cambios
selected_journals: Diarios Seleccionados
title: Editar Reporte
index:
actions:
create: Crear informe
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reports.index translations were added, but the index view expects reports.index.columns.start_date/end_date/created_at. Add a columns: block under reports.index with these keys to avoid "translation missing" headers.

Suggested change
create: Crear informe
create: Crear informe
columns:
start_date: Fecha de inicio
end_date: Fecha de fin
created_at: Fecha de creación

Copilot uses AI. Check for mistakes.
title: Informes
journal_card_component:
author: Autor
new:
Expand Down
6 changes: 1 addition & 5 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@
delete "/logout", to: "sessions#destroy"

# Report routes
resources :reports, only: %i[show new create edit update] do
collection do
get :filter
end
end
resources :reports, only: %i[index show new create edit update destroy]
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By removing the filter collection route, filter_reports_path/filter_reports_url no longer exists. The existing app/views/reports/new.html.erb still submits its GET form to filter_reports_path, so the New Report page will raise a routing error. Update that form to submit to new_report_path (and rely on ReportsController#new for filtering), or reintroduce a dedicated filter route/action.

Suggested change
resources :reports, only: %i[index show new create edit update destroy]
resources :reports, only: %i[index show new create edit update destroy] do
collection do
get :filter
end
end

Copilot uses AI. Check for mistakes.
end
end
72 changes: 62 additions & 10 deletions test/controllers/reports_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ class ReportsControllerTest < ActionDispatch::IntegrationTest
end

[
{ route: "index", method: :get, url_helper: :reports_url, needs_report: false },
{ route: "show", method: :get, url_helper: :report_url, needs_report: true },
{ route: "new", method: :get, url_helper: :new_report_url, needs_report: false },
{ route: "edit", method: :get, url_helper: :edit_report_url, needs_report: true },
{ route: "filter", method: :get, url_helper: :filter_reports_url, needs_report: false },
{ route: "update", method: :patch, url_helper: :report_url, needs_report: true }
{ route: "update", method: :patch, url_helper: :report_url, needs_report: true },
{ route: "destroy", method: :delete, url_helper: :report_url, needs_report: true }
].each do |hash|
test "##{hash[:route]} redirects to login route when a user is not authenticated" do
log_out_user
Expand Down Expand Up @@ -64,20 +65,29 @@ class ReportsControllerTest < ActionDispatch::IntegrationTest
assert_match aggregated_datum.additional_text, response.body
end

test "#filter displays projects when valid dates are provided" do
test "#index displays reports" do
report = create(:report)

get reports_path
assert_response :success
assert_match I18n.l(report.start_date.to_date), response.body
assert_match I18n.l(report.end_date.to_date), response.body
end

test "#new displays projects when valid dates are provided" do
subproject = create(:subproject)
create(:log_entry, subproject: subproject, created_at: 1.day.ago)

get filter_reports_path, params: { start_date: 2.days.ago.to_date.to_s, end_date: Time.zone.today.to_s }
get new_report_path, params: { start_date: 2.days.ago.to_date.to_s, end_date: Time.zone.today.to_s }
assert_response :success
assert_match subproject.project.name, response.body
end

test "#filter displays subprojects when projects are selected" do
test "#new displays subprojects when projects are selected" do
subproject = create(:subproject)
create(:log_entry, subproject: subproject, created_at: 1.day.ago)

get filter_reports_path, params: {
get new_report_path, params: {
start_date: 2.days.ago.to_date.to_s,
end_date: Time.zone.today.to_s,
project_ids: [subproject.project.id]
Expand All @@ -86,16 +96,16 @@ class ReportsControllerTest < ActionDispatch::IntegrationTest
assert_match subproject.name, response.body
end

test "#filter handles invalid dates gracefully" do
get filter_reports_path, params: { start_date: "invalid", end_date: "also-invalid" }
test "#new handles invalid dates gracefully" do
get new_report_path, params: { start_date: "invalid", end_date: "also-invalid" }
assert_response :success
end

test "#filter includes log entries from end date" do
test "#new includes log entries from end date" do
subproject = create(:subproject)
create(:log_entry, subproject: subproject, created_at: Time.zone.today.noon)

get filter_reports_path, params: {
get new_report_path, params: {
start_date: 2.days.ago.to_date.to_s,
end_date: Time.zone.today.to_s,
project_ids: [subproject.project.id]
Expand Down Expand Up @@ -168,6 +178,48 @@ class ReportsControllerTest < ActionDispatch::IntegrationTest
assert_equal I18n.t("reports.create.invalid"), flash[:error]
end

test "#create redirects with error when subproject_ids do not exist" do
assert_no_difference("Report.count") do
post reports_path, params: {
start_date: Time.zone.yesterday.to_s,
end_date: Time.zone.today.to_s,
subproject_ids: [0]
}
end

assert_redirected_to new_report_path
assert_equal I18n.t("reports.create.invalid"), flash[:error]
end

test "#destroy redirects to login route when a user is not authenticated" do
report = create(:report)
log_out_user

delete report_path(report)
assert_response :redirect
assert_redirected_to login_path
end

Comment on lines +193 to +201
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#destroy already has unauthenticated/unauthorized coverage via the parameterized route tests at the top of this file. The dedicated #destroy redirects to login route when a user is not authenticated test below is redundant and increases maintenance cost; consider removing it or removing destroy from the parameterized list to keep a single source of truth.

Suggested change
test "#destroy redirects to login route when a user is not authenticated" do
report = create(:report)
log_out_user
delete report_path(report)
assert_response :redirect
assert_redirected_to login_path
end

Copilot uses AI. Check for mistakes.
test "#destroy redirects to root route when a user is not authorized" do
report = create(:report)
create_logged_in_user

delete report_path(report)
assert_response :redirect
assert_redirected_to root_path
end

test "#destroy removes the report and redirects to index" do
report = create(:report)

assert_difference("Report.count", -1) do
delete report_path(report)
end

assert_redirected_to reports_path
assert_equal I18n.t("reports.destroy.success"), flash[:success]
end

test "#update replaces journals and aggregated data in one save" do
report = create(:report)
existing_journal = create(:journal)
Expand Down
Loading