-
Notifications
You must be signed in to change notification settings - Fork 6
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
IA-3433 Tasks view: Add extra filters on type, status and username (launcher) #1889
base: main
Are you sure you want to change the base?
Conversation
And add some indexes to support filtering.
between who did the initial launch, and who did a potential re-launch.
if different from the creator.
add filter on users (creator + launcher).
Extracted datetime helpers into general Iaso utils file.
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.
Good job.
I'd say that you can safely remove 2 redundant indexes and add a relation in the select_related
method.
The rest is optional.
start_date_dt = timezone.make_aware(start_date_dt, timezone.get_default_timezone()) | ||
start_date_dt = start_date_dt.replace(hour=0, minute=0, second=0) | ||
queryset = queryset.filter(analyze__created_at__gte=start_date_dt) | ||
queryset = queryset.filter(analyze__created_at__gte=date_string_to_start_of_day(start_date)) |
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.
We may be able to use a string and only filter on the date
part of the datetime
field.
E.g. (assuming the date format is correct):
queryset.filter(analyze__created_at__date__gte="2021-05-23")
queryset.filter(analyze__created_at__date__lte="2021-05-23")
This may be easier to read and may save some code.
@@ -274,11 +275,18 @@ class Task(models.Model): | |||
|
|||
class Meta: | |||
ordering = ["-created_at"] | |||
indexes = [ | |||
models.Index(fields=["account"]), |
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.
models.Index(fields=["account"]), |
This is not necessary because a database index is automatically created on the ForeignKey
(unless db_index=False
is set).
indexes = [ | ||
models.Index(fields=["account"]), | ||
models.Index(fields=["created_at"]), | ||
models.Index(fields=["created_by"]), |
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.
models.Index(fields=["created_by"]), |
Same thing as above.
@@ -56,6 +56,7 @@ def wrapper(*args, **kwargs): | |||
task = Task() | |||
user = kwargs.pop("user") | |||
task.account = user.iaso_profile.account | |||
task.created_by = user | |||
task.launcher = user |
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.
What I understand is that we assume that the creator and the launcher are always the same person. Is this correct?
class TaskSerializerSerializerTestCase(TestCase): | ||
""" | ||
Test Task serializer. | ||
""" | ||
|
||
maxDiff = None |
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.
TBH I'm always tempted to put that in the base class…
serializer_class = TaskSerializer | ||
results_key = "tasks" | ||
queryset = Task.objects.all() | ||
http_method_names = ["get", "patch", "head", "options", "trace"] | ||
|
||
def get_queryset(self): | ||
profile = self.request.user.iaso_profile | ||
order = self.request.query_params.get("order", "created_at").split(",") | ||
return Task.objects.select_related("launcher").filter(account=profile.account).order_by(*order) | ||
return Task.objects.select_related("created_by", "launcher").filter(account=profile.account) |
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.
We could add user__iaso_profile
because it's used in ordering_fields
.
return Task.objects.select_related("created_by", "launcher").filter(account=profile.account) | |
return Task.objects.select_related("created_by", "launcher", "user__iaso_profile").filter(account=profile.account) |
UsersFilterBackend, | ||
StartEndDateFilterBackend, | ||
StatusFilterBackend, | ||
] |
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.
Just for you information (no need to rewrite that) I like to do that via django-filter.
It has built-in validation and you don't need a bunch of different classes.
You have various examples in the code base.
@@ -1357,7 +1357,8 @@ | |||
"iaso.tasks.killed": "Arrêtée", | |||
"iaso.tasks.killSignalSent": "Signal d'arrêt envoyé", | |||
"iaso.tasks.killTask": "Arrêter la tâche", | |||
"iaso.tasks.launcher": "Utilisateur", | |||
"iaso.tasks.user": "Utilisateur", | |||
"iaso.tasks.last_launch_by": "Dernière execution :", |
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.
Excuse my French 😄
"iaso.tasks.last_launch_by": "Dernière execution :", | |
"iaso.tasks.last_launch_by": "Dernière exécution :", |
This PR improves the tasks. Specifically, it:
created_by
on the Task model to differentiate between who did the initial launch, and who did a potential re-launch.created_at
,started_at
andended_at
filters.py
andserializers.py
Screenshot