Conversation
Test results 4 files 524 suites 19m 39s ⏱️ Results for commit bcd6413. ♻️ This comment has been updated with latest results. |
Simrayz
left a comment
There was a problem hiding this comment.
Tested manually, and it works well. Nice 😄
810932f to
6e6d225
Compare
6e6d225 to
badbae2
Compare
6482a4f to
9839d44
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1621 +/- ##
==========================================
+ Coverage 79.64% 80.05% +0.41%
==========================================
Files 132 132
Lines 6120 6167 +47
==========================================
+ Hits 4874 4937 +63
+ Misses 1246 1230 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9eea3be to
f4de5da
Compare
f4de5da to
ecb86ea
Compare
.. also added the name of each involved template to that template as an html comment, for ease of debugging.
ecb86ea to
7030528
Compare
|
There was a problem hiding this comment.
It works great for creating new filters and deleting existing filters, but updating a filter is pretty unintuitive.
Because if I select a filter, then change any of the parameters (e.g. the slider for "acked"), then the name of the filter is updated to "---", which makes sense, because now we are using filter parameters that are not saved in the previously selected filter.
But to update the filter that I previously chose I still have to click on update now (while it's name is not in the filterbox anymore) and it will ask me if I want to update the previously selected filter. This is pretty confusing.
I rather would have a popup or something to ask me which filter I would like to update.
|
|
||
|
|
||
| def _normalize_form_data(request): | ||
| def _normalize_form_data(raw_data): |
There was a problem hiding this comment.
While we're at it, we can even add a type hint:
| def _normalize_form_data(raw_data): | |
| def _normalize_form_data(raw_data: QueryDict): |
| def set_selected_filter(request, filter_obj): | ||
| if filter_obj: | ||
| request.session["selected_filter_pk"] = str(filter_obj.pk) | ||
| request.session["selected_filter_name"] = filter_obj.name | ||
| else: | ||
| request.session["selected_filter_pk"] = None | ||
| request.session.pop("selected_filter_name", None) | ||
|
|
||
|
|
||
| def get_selected_filter(request): | ||
| filter_id = request.session.get("selected_filter_pk", None) | ||
| if filter_id: | ||
| return get_object_or_404(Filter, pk=filter_id, user=request.user) | ||
| return None |
There was a problem hiding this comment.
Beautiful helper functions!
| return HttpResponseClientRefresh() | ||
| messages.error(request, f"Failed to update filter '{filter_obj.name}'.") | ||
| errors = f": {filter_form.errors}" if filter_form.errors else "" | ||
| messages.error(request, f'Failed to update filter "{filter_obj.name}": {errors}') |
There was a problem hiding this comment.
| messages.error(request, f'Failed to update filter "{filter_obj.name}": {errors}') | |
| messages.error(request, f'Failed to update filter "{filter_obj.name}"{errors}') |
| @@ -0,0 +1,60 @@ | |||
| from django.http.response import Http404 | |||
| from django.test import tag, RequestFactory, TestCase | |||
There was a problem hiding this comment.
| from django.test import tag, RequestFactory, TestCase | |
| from django.test import RequestFactory, TestCase, tag |
| from argus.htmx.incident.views import filter_select | ||
| from argus.filter.factories import FilterFactory |
There was a problem hiding this comment.
| from argus.htmx.incident.views import filter_select | |
| from argus.filter.factories import FilterFactory | |
| from argus.filter.factories import FilterFactory | |
| from argus.htmx.incident.views import filter_select |
| pass | ||
|
|
||
|
|
||
| class filter_select_Test(TestCase): |
There was a problem hiding this comment.
| class filter_select_Test(TestCase): | |
| class FilterSelectTest(TestCase): |
| factory = RequestFactory() | ||
|
|
||
| @tag("unit") | ||
| def test_when_no_filter_in_request_should_unset_filter_in_session(self): |
There was a problem hiding this comment.
Given the name of the test I would assume that a filter was saved in the session before, but it looks like none is set and we're just testing that that doesn't change
|
How about keeping the previously selected name somewhere visible and keep the update button simple? Because having a drop-down has always been unintuitive to me. Maybe have a "save" vs. "save as" button instead of update, too? |
I really like the "save" and "save as" idea |
I agree that "Save" and "Save as new" is more intuitive than the current button text, i.e. "Update" -> "Save" and "Save new" -> "Save as new". This changes the behavior of the filter to always modify the currently selected filter, unless you "Save as new" or select a different filter. The current filter name should not be replaced with a placeholder, but remain constant unless changed by the user. |



Scope and purpose
This makes the create/update/delete part of the filter box take up less space and changes the behavior of the widget so that update/delete no longer has a drop down but works on the already selected stored filter.
Screenshots
Before:
After:
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.