Fix planned maintenance filter dropdown#1960
Conversation
Test results 8 files 1 760 suites 3m 16s ⏱️ Results for commit c195a69. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
==========================================
+ Coverage 88.52% 88.82% +0.30%
==========================================
Files 148 152 +4
Lines 7135 7276 +141
==========================================
+ Hits 6316 6463 +147
+ Misses 819 813 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
johannaengland
left a comment
johannaengland
left a comment
There was a problem hiding this comment.
The search does not work correctly - instead of replacing the choices shown with the ones fitting the search the search results are added to the list
aleksfl
force-pushed
the
planned-maintenance-filter-breaks-when-selecting-filter
branch
from
b7bbb9a to
feb2653
Compare
Heh, assumed that was intended behaviour. I have pushed a fix now. |
aleksfl
force-pushed
the
planned-maintenance-filter-breaks-when-selecting-filter
branch
4 times, most recently
from
80d7453 to
e50ff7f
Compare
aleksfl
force-pushed
the
planned-maintenance-filter-breaks-when-selecting-filter
branch
from
e50ff7f to
4ac3067
Compare
lunkwill42
left a comment
lunkwill42
left a comment
There was a problem hiding this comment.
Looks mostly ok, but a few things needs a touch-up:
This 2-character minimum does no longer seem to hold true. Searching works immediately. I guess the point of it originally was to not flood the search results with every filter on earth (in fact, it seems the original limited the number of search results to 20, while the new version has no such limit?)
Also, there is a view SearchFiltersView tied to the search-filters URL that now seems to have gone orphaned - nothing uses it. If the plan is to replace it completely, it should be removed.
Fixed, re-added the search result limit since it seems reasonable to have in any case. |
| | Q(user__last_name__icontains=query) | ||
| ) | ||
| filters = sorted(filters, key=lambda f: (f.user != request.user, f.name))[:20] | ||
| filter_ids = [fid for fid in request.GET.getlist("filters") if fid.isdigit()] |
There was a problem hiding this comment.
Follow-up: validate with a form. (Allows for standardized error handling/logging in case somebody do want to use this for SQL injection or something.)
There was a problem hiding this comment.
That seems reasonable, I made a follow-up issue so that can be done in isolation.
| ) | ||
| filters = sorted(filters, key=lambda f: (f.user != request.user, f.name))[:20] | ||
| filter_ids = [fid for fid in request.GET.getlist("filters") if fid.isdigit()] | ||
| selected_filters = Filter.objects.filter(pk__in=filter_ids).select_related("user") |
There was a problem hiding this comment.
The previous code showed the user's own filters, and sorted them on users. This does not. Why?
There was a problem hiding this comment.
Was probably forgotten in some iteration, I re-added the functionality in the new version and added a test for the future.
|
i moved this to changes requested - it seems @hmpf's questions warrant a response |
aleksfl
force-pushed
the
planned-maintenance-filter-breaks-when-selecting-filter
branch
from
446ac75 to
c195a69
Compare
|
4 participants
Scope and purpose
Fixes #1958.
Important note: This also changes the behaviour of the sources dropdown in the main filter menu so that selected filters stay in the dropdown even when a different search term is entered. This ensures both dropdowns behave consistently.
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.