-
Notifications
You must be signed in to change notification settings - Fork 151
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
Filter alerts #242
Filter alerts #242
Conversation
Codecov Report
@@ Coverage Diff @@
## master #242 +/- ##
=========================================
Coverage ? 46.62%
=========================================
Files ? 53
Lines ? 2623
Branches ? 0
=========================================
Hits ? 1223
Misses ? 1400
Partials ? 0
Continue to review full report at Codecov.
|
promgen/tasks.py
Outdated
@shared_task | ||
def index_alert(alert_pk): | ||
alert = models.Alert.objects.get(pk=alert_pk) | ||
labels = json.loads(alert.body)["commonLabels"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be written as labels = alert.json.get('commonLabels')
since Alert model defines a .json
property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 9a55893
{% include 'promgen/pagination_short.html' %} | ||
<form class="navbar-form navbar-left" action="/alert"> | ||
<div class="form-group"> | ||
<input name="search" type="text" value="{{ search }}" class="form-control" placeholder="Service, Project"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can write this as value="{{ request.GET.search }}"
and then you do not have to implement get_context_data
in the view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
promgen/views.py
Outdated
|
||
def get_context_data(self, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would probably leave the queryset at the top, and then implement it something like
class AlertList(LoginRequiredMixin, ListView):
paginate_by = 20
queryset = models.Alert.objects.order_by("-created")
def get_queryset(self):
search = self.request.GET.get("search")
if search:
return self.queryset.filter(
Q(alertlabel__name="Service", alertlabel__value__icontains=search)
| Q(alertlabel__name="Project", alertlabel__value__icontains=search)
)
return self.queryset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
May also be worthwhile to implement a management command 1 to allow to queue alerts to be re-indexed. for alert in Alert.objects.all():
task. index_alert.delay(alert.pk) |
9a55893
to
1467399
Compare
I will implement that. |
promgen/views.py
Outdated
|
||
qs = self.queryset | ||
for key, value in self.request.GET.items(): | ||
if key == "page" or key == "search": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably write this as if key in ["page", "search"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
promgen/views.py
Outdated
@@ -1033,8 +1033,15 @@ def get_queryset(self): | |||
return self.queryset.filter( | |||
Q(alertlabel__name="Service", alertlabel__value__contains=search) | |||
| Q(alertlabel__name="Project", alertlabel__value__contains=search) | |||
| Q(alertlabel__name="Job", alertlabel__value__contains=search) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be better to do alertlabel__value__icontains
so that upper/lower case do not matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few minor things I think we can fix up in a future PR, but looks good for now :)
No description provided.