Conversation
Links in main dashboard view now got to anchors in the detil view (by id) Renamed templates and CSS files for consistency
Reduces the number of queries required by nearly half.
added level_achieved field, changed templates to use it remaining work: * find a way to update all the ProjectObjectives (call the save()) so that they update their own levels: [ ] when a POC is changed [ ] manually via the admin [ ] when relevant parts of the schema are changed
In the admin change view for a work cycle, there is now an option to "Apply current QI values to this cycle". This will copy all the live QI values to the objects for the selected cycle. Includes basic tests.
* Fixed custom save() signatures * Added some tests
More speed improvements
There was a problem hiding this comment.
Pull Request Overview
This PR improves dashboard performance by implementing several optimizations to the main list view and related data processing workflows. The changes introduce a cached level_achieved field on ProjectObjective to avoid repeated on-the-fly calculations, refactor the quality indicator calculation to use database aggregation instead of Python loops, and implement lazy loading at the row level rather than per-cell.
Key Changes:
- Added
ProjectObjective.level_achievedfield that caches the calculated achievement level, updated on eachProjectObjectiveConditionsave - Refactored quality indicator calculation to use database aggregation with
Sum()andF()expressions - Implemented row-level lazy loading in the project list view using HTMX
Reviewed Changes
Copilot reviewed 37 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dashboard/projects/models.py | Added level_achieved cached field to ProjectObjective, refactored quality indicator calculation to use database aggregation, corrected save() method signatures |
| dashboard/projects/views.py | Added project_row view for lazy loading, new admin view to recalculate all levels, renamed template references for clarity |
| dashboard/projects/templates/projects/project_list.html | Implemented row-level lazy loading with HTMX, restructured table with proper thead/tbody |
| dashboard/projects/templates/projects/partial_project_list_row.html | New template for rendering individual project rows with optimized data fetching |
| dashboard/projects/test_models.py | Added comprehensive tests for quality indicator calculations and level recalculation |
| dashboard/framework/models.py | Added unique=True to name fields, corrected save() method signatures |
| dashboard/framework/views.py | Added admin_apply_qis view for bulk QI updates |
| dashboard/staticfiles/*.css | Corrected "acountability" to "accountability" typo, added sticky header positioning |
| dashboard/test_browser.py | Updated test assertion to reflect expanded test data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nbellol
left a comment
nbellol
left a comment
There was a problem hiding this comment.
I have check all the file changes, looks good.
Thank you for having descriptive commits that also help to keep track on the changes
General Recommendations:
- Try to maintain PR smaller to be able to review them faster
- On the description of the PR if possible mention which changes are applied on which files so the Reviewer can identify that quickly
- Would be nice to develop a QA template so that other reviewers know how to test the changes locally and make sure all other features work
|
|
||
| # admin methods | ||
|
|
||
| def admin_recalculate_all_levels(request): |
There was a problem hiding this comment.
| def admin_recalculate_all_levels(request): | |
| @staff_member_required | |
| @transaction.atomic | |
| def admin_recalculate_all_levels(request): |
if I get the admin_ part correctly.
There was a problem hiding this comment.
also, appropriate @require_http_methods to ensure at least decorator-level consistency with existing functions.
There was a problem hiding this comment.
After discussing with Daniele earlier, we figure it's not a problem if anyone is able to recalculate the levels. I'll double-check the workings tomorrow.
There was a problem hiding this comment.
After looking into this further, I agree with Artem that we should to use @staff_member_required. Without @staff_member_required, the recalculate endpoint could be triggered anonymously or by any user. That doesn't inherently cause any harm, but I think we should restrict to staff members for two reasons:
- The recalculate endpoint modifies the database. I believe it's not good security practice to expose that unnecessarily, e.g., in case of a vulnerability or denial of service attack.
- The UI for triggering a recalculation is only shown to staff members. Conceptually, the endpoint should have a matching restriction.
I've pushed a commit that applies the restriction and updates the test accordingly. See the diff
| from projects.models import Project, QI | ||
|
|
||
|
|
||
| def admin_apply_qis(request, workcycle_id): |
There was a problem hiding this comment.
| def admin_apply_qis(request, workcycle_id): | |
| @staff_member_required | |
| @transaction.atomic | |
| def admin_apply_qis(request, workcycle_id): |
if I get the admin_ part correctly.
There was a problem hiding this comment.
I just looked & tested this in more detail after discussion with @evildmp. The "Apply current QI values to this cycle" action should only appear (and succeed) for users with the "Can change cycle" permission. Originally in this PR, it was available to any user who could view cycles. I've pushed a commit that adds a restriction - see:
84a8015
Tests are now failing because the tests need a user with sufficient permissions. While working on a fix for that, I noticed that the tests don't seem to work as expected. Some tests would pass even without applying QIs, I think because the applied values happen to be 0. I'll take another look tomorrow with a clearer head.
There was a problem hiding this comment.
OK, I looked into this again. The original tests were indeed not working properly. Two of the tests set up projects with QI equal to 0, then applied the QI values, which had no effect. So two of the tests were passing for the wrong reason.
I've fixed the tests using the approach that I think was originally intended. I've also added a user with "Can change cycle" permission to the tests, plus one more test to check that a user with "Can view cycle" permission (only) can't apply QI values. See the diff
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
I've reviewed the code at a high level and tried out the new functionality. I like the improvements!
Approving, but we should wait for Artem's suggestions to be resolved before merging. I agree on squash merging (and I think this should be our default approach, with PRs that are tightly-scoped enough to be summarised by the resulting commit message).
FYI other reviewers and @evildmp - I pushed some changes while reviewing:
- Removed a duplicate return statement, as flagged by @akcano.
- Removed files in
dashboard/staticfilesthat had been copied there temporarily during development but aren't needed now. I realised that our CI can't detect this situation, so I've opened #101 to improve that. - Removed
django-watchfilesfrom requirements, which was also added temporarily during development. - Bumped the rock version to 0.37.
I also captured one of @nbellol's suggestions in #102.
@jahn-junior I see you tagged as a reviewer. Were you wanting to review this PR too?
|
No worries about reviewing @jahn-junior - Daniele mentioned that he'd originally marked you as a review. I'll remove you |
dwilding
changed the title
This PR addresses performance of the main dashboard list view.
Performance
Instead of on-the-fly calculations, a
ProjectObjective.level_achievedfield stores a calculated level that is updated on eachProjectObjectiveConditionsave.The
ProjectObjective.level_achievedis also calculated more efficiently with fewer queries.The
quality_indicatorcalculation now uses database aggregation instead of a Python loop.Lazy loading in the list view is now by row instead of by cell.
New admin options
A manual option to update all levels is added in the admin, to be used for example when the schema changes.
Also in the admin, there's now an option to copy all QI values to the selected cycle.
Improved functionality
Links in the dashboard list view now go to the particular objective anchor in detail view.
Also
Corrects various custom
save()method signatures.The
initial_data.yamlhas been expanded for better performance testing.Tests have been added and updated.
I recommend squashing these commits into one on merge.