fix: TAM-4047: remove static report permissions#10102
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 13d8b76. Configure here.
| WHERE noun = 'StaticReport' | ||
| AND verb = 'run' | ||
| AND deleted_at IS NULL | ||
| `); |
There was a problem hiding this comment.
Blank StaticReport run not migrated
High Severity
The migration only creates ReportDefinition:run rows for StaticReport:run permissions whose object_id is generic-survey-export-line-list, then soft-deletes every remaining StaticReport:run grant. Roles that had a blanket StaticReport:run with no object_id (as in the prior default practitioner template) lose run access and are not given an equivalent ReportDefinition permission.
Reviewed by Cursor Bugbot for commit 13d8b76. Configure here.
| ...getStaticReports(ability, models), | ||
| ...(await getDbReports(ability, models)), | ||
| ]; | ||
| const permittedReports = await getDbReports(ability, models); |
There was a problem hiding this comment.
Disabled report id no longer matches
Medium Severity
disabledReports entries and run requests now use a ReportDefinitionVersion id, but localisation often still lists generic-survey-export-line-list for the generic survey export. That id no longer matches available-report entries or the reportId on run/report-request calls, so disabled-report config no longer hides or blocks that report.
Reviewed by Cursor Bugbot for commit 13d8b76. Configure here.
|
🦸 Review Hero (could not post inline comments — showing here instead)
[BES Requirements] Mutating the Sequelize model instance with
[Design & Architecture]
[Integration tests] This test passes
[Integration tests] The 'should create a new report request' test passes
[Security] The |
|
🦸 Review Hero Summary Below consensus threshold (8 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
|
Android builds 📱
|
🍹
|
Replaces StaticReport:run permissions with ReportDefinition:run via migration: creates a ReportDefinition and ReportDefinitionVersion for generic-survey-export-line-list, inserts new permissions for affected roles, and soft-deletes the old StaticReport permissions. Removes all StaticReport permission references from constants, shared report utilities, and server routes, and updates tests accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…port The migration seeds a ReportDefinition and ReportDefinitionVersion for generic-survey-export-line-list. Update affected tests: - reportRoutes: find testReport by ID instead of checking total list length - ReportRequest: look up version UUID; check reportDefinitionVersionId not reportType - CentralSyncManager.updateLookupTable: truncate ReportDefinition/Version in beforeEach so the seeded record does not appear in sync lookup counts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on UUID The disabledReports localisation config uses stable definition IDs (e.g. generic-survey-export-line-list), but disabled checks were comparing against the version UUID after the static report migration. Fix all five check sites to use reportModule.reportDefinitionId instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Roles with a general StaticReport:run permission (no objectId) had access to all static reports. The migration now also creates a ReportDefinition:run:generic-survey-export-line-list for these roles so they retain access after the static report is removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rohan-bes
force-pushed
the
fix/tam-4047-remove-static-reports
branch
from
ec2509e to
ce58b70
Compare
1 participant
Changes
Removes the
StaticReportpermission noun entirely and migrates the only static report (generic-survey-export-line-list) to a properReportDefinition+ReportDefinitionVersion. A migration creates the DB records, inserts newReportDefinition:runpermissions for any roles that had the oldStaticReport:runpermission, and soft-deletes the old permissions. All static-report-specific code paths (canRunStaticReport, reportDefinitions, reportObjects, the StaticReport schema entries) are removed, and the report module/permission/availability utilities are simplified to handle only DB-defined reports.Auto-Deploy
Options
Tests
Review Hero
.github/review-hero/suppressions.yml. Also runs automatically at the end of any auto-fix run.Remember to...