Skip to content

Refactor Report summary - #1544

Merged
jazairi merged 3 commits into
mainfrom
etd-706
Nov 3, 2026
Merged

jazairi merged 3 commits into
mainfrom
etd-706

Conversation

@jazairi

@jazairi jazairi commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Why these changes are being introduced:

The former Report summary was inefficient. It
calculated statistics for every degree period on
each dashboard request. Some calculations loaded
thesis records into memory and made additional
queries per thesis. This approach quickly became
unsustainable.

As a stopgap, we reduced the Report index to a
list of links to each individual report. However,
stakeholders suggested that it would be useful
to provide a summary of the most recent terms.

Relevant ticket(s):

How this addresses that need:

  • Restores the five summary tables, showing the latest four degree periods containing thesis
    records.
  • Moves summary counts into SQL to avoid loading ActiveRecord objects into memory.
  • Adds a theses.grad_date index, created concurrently on PostgreSQL to avoid blocking
    writes.

I ran a smoke test locally using a copy of the
prod db. The test made 30 requests each to the
Report index page at 1, 3, and 5 concurrent
requests. All 90 requests were successful, with
median response times ranging from 132ms to 319ms.

Side effects of this change:

Local testing results are encouraging, but we
don't yet know how this will perform in
production.

Developer

Accessibility
  • ANDI or WAVE has been run in accordance to our guide.
  • This PR contains no changes to the view layer.
  • New issues flagged by ANDI or WAVE have been resolved.
  • New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
  • No new accessibility issues have been flagged.
New ENV
  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.
Approval beyond code review
  • UXWS/stakeholder approval has been confirmed.
  • UXWS/stakeholder review will be completed retroactively.
  • UXWS/stakeholder review is not needed.
Additional context needed to review

E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.
Testing
  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

Why these changes are being introduced:

The former Report summary was inefficient. It
calculated statistics for every degree period on
each dashboard request. Some calculations loaded
thesis records into memory and made additional
queries per thesis. This approach quickly became
unsustainable.

As a stopgap, we reduced the Report index to a
list of links to each individual report. However,
stakeholders suggested that it would be useful
to provide a summary of the most recent terms.

Relevant ticket(s):

- [ETD-706](https://mitlibraries.atlassian.net/browse/ETD-706)

How this addresses that need:

- Restores the five summary tables, showing the
latest four degree periods containing thesis
records.
- Moves summary counts into SQL to avoid loading
ActiveRecord objects into memory.
- Adds a theses.grad_date index, created
concurrently on PostgreSQL to avoid blocking
writes.

I ran a smoke test locally using a copy of the
prod db. The test made 30 requests each to the
Report index page at 1, 3, and 5 concurrent
requests. All 90 requests were successful, with
median response times ranging from 132ms to 319ms.

Side effects of this change:

Local testing results are encouraging, but we
don't yet know how this will perform in
production.
@jazairi
jazairi requested a balanced review from Copilot October 30, 2026 19:07
@mitlib
mitlib temporarily deployed to thesis-submit-pr-1544 October 30, 2026 19:07 Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Existing dashboard tests could pass with no student contributions, so regression coverage is needed for positive per-term counts, repeated-edit deduplication, and exclusion of staff/admin, registrar, and out-of-term contributions.

1 open issue
What changed in this PR

Restores the report dashboard’s recent-term summaries while reducing database work and avoiding loading thesis records into memory.

Changes:

  • Displays five summary tables for the latest four degree periods containing theses.
  • Moves summary calculations into SQL and adds a graduation-date index, created concurrently on PostgreSQL.
  • Adds tests for dashboard rendering, term filtering, and record-free aggregation.
File Description
test/​models/​report_test.rb Tests term filtering and aggregation without record instantiation.
test/​controllers/​report_controller_test.rb Checks five tables and links for the latest four periods.
db/​schema.rb Records the new index and refreshes the schema dump.
db/​migrate/​20261029150000_add_grad_date_index_to_theses.rb Adds the graduation-date index with PostgreSQL concurrency support.
app/​views/​report/​index.html.erb Restores summary tables with linked term headings.
app/​models/​report.rb Adds term-scoped SQL aggregates and shared summary helpers.
app/​controllers/​report_controller.rb Selects the latest four periods and loads dashboard data.

Review effort: Balanced

Comment thread app/models/report.rb Resolved
@coveralls

coveralls commented Oct 30, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 98.298% (-0.009%) from 98.307% — etd-706 into main

@jazairi
jazairi requested a balanced review from Copilot November 2, 2026 18:41
@jazairi
jazairi temporarily deployed to thesis-submit-pr-1544 November 2, 2026 18:41 Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The remaining report submenu checks the wrong permission name, leaving authorized processors without a dashboard link to “Theses submitted by students” after the reports list is removed.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Use the plural permission for the student theses submenu link

app/​views/​report/​index.html.erb:9

Removing the available-reports list leaves processors without a dashboard link to “Theses submitted by students,” even though they remain authorized to access it. The remaining link in app/views/shared/_report_submenu.html.erb:14 checks :student_submitted_thesis (singular), but Ability#processor grants :student_submitted_theses (plural). Change that submenu check to the plural permission so processors retain access through the dashboard navigation.

Review effort: Balanced

@jazairi
jazairi requested a balanced review from Copilot November 3, 2026 00:05
@jazairi
jazairi temporarily deployed to thesis-submit-pr-1544 November 3, 2026 00:06 Inactive
Identified in a Copilot review. This only affects
the unused processor role, but we should fix it
nonetheless.
@jazairi
jazairi temporarily deployed to thesis-submit-pr-1544 November 3, 2026 00:07 Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are recorded for the dashboard summary restoration and SQL aggregation changes.

1 open issue

Review effort: Balanced

@JPrevost JPrevost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving with the assumption that only stakeholders can fully confirm this does what they need in production and adjustments will be made if anything is inaccurate or slow.

@jazairi

jazairi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

@JPrevost Yeah, the next steps for me are to promote, then solicit feedback from Mikki and Wilder. I suspect it might be a while before we identify any potential problems, but it's a low-risk feature.

@jazairi
jazairi merged commit 2e3ac58 into main Nov 3, 2026
2 checks passed
@jazairi
jazairi deleted the etd-706 branch November 3, 2026 15:31

This branch was previously deployed

1 inactive deployment
thesis-submit-pr-1544 — 5b041cb0 Deployed Nov 3, 2026 by jazairi
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.

5 participants