Refactor maintenance task views to improve display of deleted components #3230 #3231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports MegaLinter is graciously provided by |
Test results 9 files 9 suites 8m 9s ⏱️ Results for commit b2b92d3. ♻️ This comment has been updated with latest results. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3231 +/- ##
==========================================
- Coverage 60.39% 60.33% -0.06%
==========================================
Files 605 606 +1
Lines 43687 43700 +13
Branches 48 48
==========================================
- Hits 26383 26366 -17
- Misses 17292 17322 +30
Partials 12 12 ☔ View full report in Codecov by Sentry. |
lunkwill42
force-pushed
the
feature/maintenance-improve-deleted-component-display
branch
2 times, most recently
from
467367d to
3bd0ebd
Compare
lunkwill42
commented
Nov 22, 2024
lunkwill42
commented
Nov 22, 2024
lunkwill42
commented
Nov 22, 2024
johannaengland
approved these changes
Nov 26, 2024
lunkwill42
force-pushed
the
feature/maintenance-improve-deleted-component-display
branch
from
c403ccc to
d950a1c
Compare
The description field will be mainly to store component descriptions for future use in the case where the original object has been deleted from the NAV database.
This is useful to get a reference to the model class of the referenced component, even if the foreign key itself no longer resolves to an existing record
They way the maintenance UI builds data structures to describe maintenance components (and their "trails") is quite obtuse, and seems to be a strange leftover from the olden days before Django was introduced to NAV. This can be done a lot cleaner, while improving on how missing components are described. These classes and functions represent an alternative way to build component trails to place in the template render context.
These type hints help a lot when working in an IDE
This replaces the usage of the old component trail generating functions with the new ones, and updates the templates accordingly. It also factors out `frag-component-trail.html` to a separate template fragment for clarity.
We would never refer to an IP device as an `ip device` in the UI, `IP` should always be upper case.
There is a definitive canonical URL for Locations, so we should reflect it in the model - and thereby make it easier for the maintenance task subsystem to make links to location components.
The imports were getting chaotic in `views.py`. Used isort to clean it up.
This more or less removes the redundant strings used in the function, by using a list of allowed/expected component keys.
Add each component error from POST data processing as its own error message. It got very unreadable when a bug caused all component keys to generate errors.
Why hardcode the list of component keys to put in the data structure when we already have the definitive list in a constant?
lunkwill42
force-pushed
the
feature/maintenance-improve-deleted-component-display
branch
from
d950a1c to
536b76d
Compare
This replaces `utils.components_for_keys()` with `get_components_from_keydict()`, which works more like the newer `get_components()`, but works on POST data rather than Task object data. The end result is an edit view that builds the component trails use in the maintenance in much the same way as the refactored `view()` view, which means that the HTML template for the `edit()` view can now also re-use the `frag-component-trail.html` template.
These are no longer used by any code.
It's stupid to keep a hardcoded list of component types that only accept integer primary keys, when can be detected from the corresponding model definitions. This takes away the redundancy.
As mentioned in code review, the original name doesn't convey much to new devs.
As noticed in code review, the refactored version of the maintenance UI did not display the same descriptive title tooltips on component trails as the original did. This solves the issue by adding a separate `component_description` template tag.
Adding minor, but useful suggestions from code review Co-authored-by: Johanna England <[email protected]>
lunkwill42
force-pushed
the
feature/maintenance-improve-deleted-component-display
branch
from
536b76d to
b2b92d3
Compare
|
lunkwill42
deleted the
feature/maintenance-improve-deleted-component-display
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #3228, but it was not feasible without a significant refactoring of the maintenance task UI codebase.
The codebase originally stems from a time before Django was added to NAV, and was significantly convoluted compared to what it needs to be. This PR makes it slightly less so, but there is still some way to go:
There was a lot of processing and custom validation code for the component fields of the maintenance task forms, which ended up producing large hierarchies of dicts and lists (some of which were fed directly to templates), instead of re-using ORM functionality. While much of the processing would be best encapsulated in Django Forms, this refactor mostly concerns itself with using ORM objects where we can.
The ultimate goal was really only to be able to display deleted components in a better way, by utilizing a new component
descriptionfield. This is achieved in this PR by replacing the template render data witch actual lists of Django model objects, and by replacing the missing objects with a specialMissingComponentclass.Please do ask for clarifications if necessary.