add delivery-type, year, quarter filter to admin report #401
Conversation
sravfeyn
commented
Sep 22, 2024
•
sravfeyn
commented
- Refactor to htmx based reports
- Add filters
sravfeyn
force-pushed
the
sr/dsr-filters
branch
from
c406198
to
4f37a6d
Compare
|
|
||
| {% block javascript %} | ||
| {{ block.super }} | ||
| <!-- This is required for select2 'user' field--> |
There was a problem hiding this comment.
what is the select2 user field mentioned? i don't see a reference to select2 or users elsewhere in this PR
There was a problem hiding this comment.
I let this be since it's needed in another PR #349
|
|
||
| app_name = "reports" | ||
|
|
||
| urlpatterns = [ | ||
| path("delivery_stats", views.delivery_stats_report, name="delivery_stats_report"), | ||
| # path("delivery_stats", views.delivery_stats_report, name="delivery_stats_report"), |
There was a problem hiding this comment.
Was this left commented in the code for a specific reason? Seems like we can delete it
There was a problem hiding this comment.
Not really, will remove it
| table_data.append(data) | ||
| table = AdminReportTable(table_data) | ||
| return render(request, "reports/admin.html", context={"table": table}) | ||
| class SuperUserRequiredMixin(LoginRequiredMixin, UserPassesTestMixin): |
There was a problem hiding this comment.
Since this will be useful to any view that needs to enforce superusers, and isn't report specific, it would be great to pull this out of this view file and into a util/helper one.
| @@ -33,7 +35,12 @@ def _get_quarters_since_start(): | |||
| return quarters | |||
|
|
|||
|
|
|||
| def _get_table_data_for_quarter(quarter): | |||
| def _get_table_data_for_quarter(quarter, delivery_type): | |||
There was a problem hiding this comment.
Unrelated to your PR except that you are already touching this code, but it would be great if we could cache this fairly aggressively, since its so expensive, although we may actually want to do that one level up and cache the entire loop over the quarters if we can. If not caching each quarter is probably still great
| year = django_filters.ChoiceFilter(method="filter_by_ignore") | ||
| quarter = django_filters.ChoiceFilter( |
There was a problem hiding this comment.
What is the intent of these filters? It seems unlikely to be useful (especially the quarter once since if you filter to year, you have already filtered down to only four rows), but I might be missing something you had in mind
| @@ -30,6 +30,9 @@ django-cors-headers | |||
| # DRF-spectacular for api documentation | |||
| drf-spectacular | |||
| django-tables2 | |||
| django-filter | |||
There was a problem hiding this comment.
the changes in here won't take effect until you compile requirements inv requirements
| filterset_class = DeliveryReportFilters | ||
|
|
||
| def get_template_names(self): | ||
| if self.request.htmx: |
There was a problem hiding this comment.
rather than add this new dependency and middleware, how would you feel about having the table just render asynchronously through a separate view call, like our other tables. That has the added benefit of not blocking the page render in the browser, which for one this slow, can be a much nicer experience, and hopefully requires less template duplication
There was a problem hiding this comment.
render asynchronously through a separate view call, like our other tables.
May be I am not following this, this is an async call. The table gets re-rendered when the filters are selected/modified.
I think the advantage of this is that we just need to define a single view that has all the logic in it. I agree this particular view is slow, but may be the priority to should be to improve that?
| @@ -0,0 +1,87 @@ | |||
| {% extends "django_tables2/bootstrap5.html" %} | |||
There was a problem hiding this comment.
is it possible to reuse (or inherit) from the base report table we already defined?
There was a problem hiding this comment.
This template is just the table part, the base report table template includes the filters and other nav-sections etc..
| {% endif %} | ||
| {% endblock table.thead %} | ||
|
|
||
| {% block pagination %} |
There was a problem hiding this comment.
Do we have pagination in this report? I was unable to find it in the view.
There was a problem hiding this comment.
Yeah, we don't have pagination for this report, but this template is basically the common template that can be used everywhere. Taken from #349 (if this were merged, then this would have just been a DRY reference)
| return self.request.user.is_superuser | ||
|
|
||
|
|
||
| class DeliveryReportFilters(django_filters.FilterSet): |
There was a problem hiding this comment.
It seems like we are using django-filters in a pretty nontraditional way, based on all the overrides (filter_by_none, NonModelFilterView, etc), what functionality are we getting from it?
There was a problem hiding this comment.
This was a bit of a non-traditional filter (in that it's not based on model fields), you can how useful it is in the Events PR #349
Additionally, even in this case, we don't have to worry about creating form/input and extracting it etc.
sravfeyn
force-pushed
the
sr/dsr-filters
branch
from
4f37a6d
to
d218ad4
Compare
sravfeyn
force-pushed
the
sr/currency
branch
2 times, most recently
from
35ced03
to
60b0e7b
Compare
