Skip to content

Added reports navigation Item in sidebar#254

Closed
AliK070 wants to merge 9 commits intomainfrom
reports-sidebar-navigation
Closed

Added reports navigation Item in sidebar#254
AliK070 wants to merge 9 commits intomainfrom
reports-sidebar-navigation

Conversation

@AliK070
Copy link
Contributor

@AliK070 AliK070 commented Mar 21, 2026

TL;DR

Added icon and navigation for the reports view page.


What is this PR trying to achieve?

To add navigation for viewing the reports page.


How did you achieve it?

Added the Reports sidebar navigation item (using the reports-fill icon) and adds the i18n keys so the label appears correctly in supported locales.

Checklist

  • Changes have been top-hatted locally
  • Tests have been added or updated
  • Documentation has been updated (if applicable)
  • Linked related issues

Copilot AI review requested due to automatic review settings March 21, 2026 02:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a dedicated Reports index page and wires it into the admin sidebar so admins can navigate to and manage generated reports.

Changes:

  • Adds reports#index UI (with action buttons) and updates routes to expose index and destroy, removing the old filter collection route.
  • Moves the “filtering” behavior into reports#new (GET params) and updates the new report form/tests accordingly.
  • Adds i18n keys (EN/ES) for the new index/destroy UI and introduces a new sidebar icon.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/controllers/reports_controller_test.rb Updates route/auth tests and adds coverage for index and destroy flows.
config/routes.rb Exposes reports#index and reports#destroy; removes filter route.
config/locales/es/reports.es.yml Adds ES strings for reports index and destroy confirmations/success.
config/locales/es/components.es.yml Adds ES sidebar label for “Reports”.
config/locales/en/reports.en.yml Adds EN strings for reports index and destroy confirmations/success.
config/locales/en/components.en.yml Adds EN sidebar label for “Reports”.
app/views/reports/new.html.erb Changes filter form target from removed filter_reports_path to new_report_path.
app/views/reports/index.html.erb Introduces reports listing table + actions (view/edit/delete) and create button.
app/controllers/reports_controller.rb Adds index and destroy; moves filter logic into new.
app/components/sidebar_component.rb Adds “Reports” navigation item using the new icon.
app/assets/images/icons/reports-fill.svg Adds new reports icon asset used by sidebar.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -1,9 +1,33 @@
class ReportsController < ApplicationController
def index
@reports = Report.all
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

@reports = Report.all will load and render every report, and Shared::IndexTableComponent renders all provided records into the DOM (client-side pagination/filtering). This is unbounded and can degrade performance as the reports table grows; consider ordering and adding server-side limiting/pagination (e.g., order(created_at: :desc) plus real pagination) before passing records to the table.

Suggested change
@reports = Report.all
page = params[:page].to_i
page = 1 if page < 1
per_page = 50
@reports = Report.order(created_at: :desc)
.limit(per_page)
.offset((page - 1) * per_page)

Copilot uses AI. Check for mistakes.
@AliK070 AliK070 closed this Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants