Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to create and select incident filters #1122

Merged
merged 29 commits into from
Jan 22, 2025

Conversation

podliashanyk
Copy link
Contributor

Closes #1045

@podliashanyk podliashanyk added enhancement New feature or request help wanted Extra attention is needed frontend Affects frontend priority: high HTMx Views, urls, templates... labels Jan 13, 2025
@podliashanyk podliashanyk requested review from a team January 13, 2025 10:57
@podliashanyk podliashanyk self-assigned this Jan 13, 2025
Copy link

github-actions bot commented Jan 13, 2025

Test results

   10 files  1 060 suites   38m 14s ⏱️
  536 tests   535 ✅  1 💤 0 ❌
5 360 runs  5 350 ✅ 10 💤 0 ❌

Results for commit 0cd72b9.

♻️ This comment has been updated with latest results.

@hmpf
Copy link
Contributor

hmpf commented Jan 13, 2025

Tested, works. is there any way to make it fit on the same line as the rest or otherwise make it take up less space?

image

@hmpf
Copy link
Contributor

hmpf commented Jan 13, 2025

We could also make open/closed and acked/unacked take up less space.

@elfjes
Copy link
Collaborator

elfjes commented Jan 14, 2025

We have a condensed multi-checkbox filter like this:
image

Now, this is for a MultipleChoiceField form, but I think you should be able to make it work for the tristate fields

@podliashanyk podliashanyk force-pushed the incidents-filter-selector branch from 68dd3e8 to beba7b2 Compare January 14, 2025 12:37
Comment on lines 149 to 151
filter_pk, filter_obj = request.session.get("selected_filter", None), None
if filter_pk:
filter_obj = get_object_or_404(Filter, pk=filter_pk)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest placing this inside incident_list_filter instead of in the view. This keeps both the view and the filter-plugin interface clean

Copy link
Contributor

@hmpf hmpf Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear. The session stuff or raising the 404 or both?

I'm not too fond of raising 404 directly in incident_list_filter.

Comment on lines 104 to 115
def create_filter(request: HtmxHttpRequest):
filter_name = request.POST.get("filter_name", None)
incident_list_filter = get_filter_function()
filter_form, _ = incident_list_filter(request, None)
if filter_name and filter_form.is_valid():
filterblob = filter_form.to_filterblob()
_, filter_obj = create_named_filter(request, filter_name, filterblob)
if filter_obj:
request.session["selected_filter"] = str(filter_obj.id)
return HttpResponseClientRefresh()
messages.error(request, "Failed to create filter")
return HttpResponseBadRequest()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a nice candidate for extending the filter plugin system (which is currently only get_filter_function). If we can override filter validation and creation of Filters, we can also make use of this endpoint (and others concerning filters). Don't think it'll have to be this pr though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope now, yes, let's make it work first. Also we should have update and delete filter working first I suspect!

@podliashanyk podliashanyk force-pushed the incidents-filter-selector branch from 14e7ed6 to 91a5b79 Compare January 15, 2025 13:21
@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 27.38095% with 61 lines in your changes missing coverage. Please review.

Project coverage is 78.48%. Comparing base (249b681) to head (0cd72b9).

Files with missing lines Patch % Lines
src/argus/htmx/incident/filter.py 28.84% 37 Missing ⚠️
src/argus/htmx/incident/views.py 22.58% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1122      +/-   ##
==========================================
- Coverage   79.08%   78.48%   -0.61%     
==========================================
  Files         141      141              
  Lines        5375     5442      +67     
==========================================
+ Hits         4251     4271      +20     
- Misses       1124     1171      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@podliashanyk podliashanyk marked this pull request as ready for review January 15, 2025 14:54
@podliashanyk podliashanyk requested review from johannaengland, elfjes, hmpf and a team January 15, 2025 14:54
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good! Now it is only nitpicking and polish suggestion for new issues from my side

I think it would be good to add comments to the parts that override request.session["selected_filter"] to explain why that is happening (to persist the choices)

src/argus/htmx/incident/views.py Outdated Show resolved Hide resolved
src/argus/htmx/incident/views.py Show resolved Hide resolved
src/argus/htmx/incident/filter.py Outdated Show resolved Hide resolved
src/argus/htmx/incident/filter.py Show resolved Hide resolved
src/argus/htmx/incident/views.py Outdated Show resolved Hide resolved
@podliashanyk podliashanyk merged commit 605b38e into master Jan 22, 2025
17 checks passed
@podliashanyk podliashanyk deleted the incidents-filter-selector branch January 22, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Affects frontend help wanted Extra attention is needed HTMx Views, urls, templates... priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the select filter/save filter controls
5 participants