Skip to content

Feature project report list username grouping#135

Closed
maciejSamerdak wants to merge 14 commits intofeature-project-report-listfrom
feature-project-report-list-username-grouping
Closed

Feature project report list username grouping#135
maciejSamerdak wants to merge 14 commits intofeature-project-report-listfrom
feature-project-report-list-username-grouping

Conversation

@maciejSamerdak
Copy link
Collaborator

@maciejSamerdak maciejSamerdak commented Apr 15, 2019

Introduces reports per user grouping in project-report-list view.

Related #133

Do not delete this branch until #136 is merged

@classmethod
def get_queryset(cls, pk: int) -> QuerySet:
return Report.objects.filter(project=pk).order_by("-date")
def get_queryset(cls, project_pk: int, author_pk: int) -> QuerySet:
Copy link
Contributor

@rwrzesien rwrzesien Apr 17, 2019

Choose a reason for hiding this comment

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

This method should not accept any extra parameters because it might be used in other places in this view and they will not be aware of that. Instead of that do in get() self.project = get_object_or_404(Project, pk=pk) and here use self.project.pk and ...

...user.pk, that comes from a different query. But why? It can be done in single query. Reports should be accessible directly from CustomUser like user.report_set.all or user.report_set.all() depending if you are using it in template or python code. So it is enough to pass result of project.members.all() to template. But you can also do this directly in template. So it is only enough to pass Project object to html template, and get everything what is needed from it then.

But probably you paid attention and remember that I have said that data should be prepared in backend and queries should not be done in template. That applies here too. That is why you can use select_related (https://docs.djangoproject.com/pl/2.1/ref/models/querysets/#select-related) to fetch for all required related data in the backend.

Please treat it as exercise on how you can limit amount of database queries from N to 1 and complexity of the view from building complex mapping structure to passing single object to template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case of this particular view class (APIView, extending Django's View), which is the lowest level, it has no get_queryset method of its own, so this is in fact it's definition and thus I have full control over it's usage in the view. This is the reason why I allowed myself to deviate from convention and modify it a little to suit my needs.

To be honest, I completely forgot about the select_related mechanism during development, despite it being quite obvious and me using it frequently in previous project. I haven't thought of it for some reason, so thanks for the feedback along with providing documentation highlighting it's performance benefits. 😄

So if I understand correctly, it's perfectly acceptable to pass single object (Project) to template and THEN call it's related objects with aforementioned method IN the template itself?

Copy link
Contributor

@rwrzesien rwrzesien Apr 22, 2019

Choose a reason for hiding this comment

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

So if I understand correctly, it's perfectly acceptable to pass single object (Project) to template and THEN call it's related objects with aforementioned method IN the template itself?

It is already evaluated, because it was retrieved using .get().

Please simplify the view and apply select_related().

Copy link
Contributor

@rwrzesien rwrzesien left a comment

Choose a reason for hiding this comment

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

Please read my comment and try to apply it :)

@maciejSamerdak maciejSamerdak force-pushed the feature-project-report-list branch from 62dbd03 to e4c4490 Compare May 7, 2019 12:44
@maciejSamerdak maciejSamerdak force-pushed the feature-project-report-list-username-grouping branch from 900745c to b5f330c Compare May 7, 2019 12:44
@maciejSamerdak
Copy link
Collaborator Author

maciejSamerdak commented May 8, 2019

So far, I was only able to completely discard get_queryset method and incorporate the prefetch_related mechanism in the backend, hopefully reducing the amount of hits on database.

Because in rendered view we want to display date as a single cell, during the rendering I must know which reports belong to said date and how many of them are there. Therefore, unless I'll learn how to do that with a queryset alone without any additional javascripts for table reformatting, I cannot parse the Project alone to the template and I must stick to the data structure I'm currently providing.

@kbeker kbeker force-pushed the feature-project-report-list branch from e4c4490 to d5c2c57 Compare May 9, 2019 07:02
@kbeker kbeker force-pushed the feature-project-report-list branch from 566b69b to 8fc548b Compare May 10, 2019 10:38
@kbeker kbeker force-pushed the feature-project-report-list branch from 8fc548b to 75c93ad Compare May 10, 2019 10:38
@maciejSamerdak maciejSamerdak force-pushed the feature-project-report-list-username-grouping branch from ed60166 to da92de6 Compare May 10, 2019 13:31
@maciejSamerdak
Copy link
Collaborator Author

Works like a charm, at the expense of so little code. I still can't get over it! 😂

@rwrzesien
Copy link
Contributor

Works like a charm, at the expense of so little code. I still can't get over it!

The less code doing more, the better :)

@rwrzesien
Copy link
Contributor

@kbeker We could merge it first to #133 and then apply change to DetailView in that branch.

@kbeker kbeker force-pushed the feature-project-report-list branch from 75c93ad to cd5cc4f Compare May 13, 2019 12:37
@kbeker kbeker closed this May 13, 2019
@kbeker
Copy link
Contributor

kbeker commented May 13, 2019

Accidentally closed. Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants