Feature project list view by MartynaAnnaGottschling · Pull Request #85 · Code-Poets/sheetstorm

MartynaAnnaGottschling · 2019-03-08T13:46:43Z

Resolves #50
Resolves #51

managers/templates/managers/projects_list.html

managers/tests/test_unit_managers_apiview.py

rwrzesien · 2019-03-10T23:37:15Z

managers/templatetags/custom_tags.py

@@ -0,0 +1,38 @@
+from django import template


Template tags with database queries should only be used when necessary. At best - never :) All data required by the template frontend should be prepared in backend, to keep template frontend logic at minimum. The same effect can be achieved by using Django custom model manager, but it has advantage that it can be used in both template and Python code, so there is no need to duplicate this functionality.

Definitely filter_terminated, filter_active and filter_completed can be moved as custom manager methods because they don't accept any extra parameters. About the rest I can't tell because they are not used in this pull request, but I think it is doable to, so please do them all as custom manager methods.

Please check https://simpleisbetterthancomplex.com/tips/2016/08/16/django-tip-11-custom-manager-with-chainable-querysets.html for an example how to do this. If you need help please contact me.

managers/tests/test_unit_managers_apiview.py

rwrzesien · 2019-03-10T23:46:21Z

managers/views.py

+        return Project.objects.all().order_by('id')
+
+    def get(self, request):
+        projects_queryset = self.get_queryset()


The whole logic of this get method is actually selecting queryset, so it should be moved to get_queryset method. This way you can remove get method from here and lean on its default implementation.

rwrzesien · 2019-03-10T23:49:07Z

users/templates/base.html

          <td class="padding"><a href="{% url 'custom-users-list' %}">{% trans 'Employees' %}</a></td>
+          <td class="padding"><a href="{% url 'custom-projects-list' %}">{% trans 'Projects list' %}</a></td>
+            {% endif %}
+            {% if user.user_type == "MANAGER" %}


This is a good candidate for custom template tag. This way we could use "MANAGER" value directly from Python enum, in case if its value would change in the future.

rwrzesien · 2019-03-10T23:55:01Z

managers/views.py

    serializer_class = ProjectSerializer
+
+
+class ProjectsList(APIView):


If you are implement only GET method to retrieve a list, inherit from rest_framework.generics.ListAPIView (if there will be any problems after switching to this, please drop me a comment).

Any more sophisticated API view than the basic one requires extra attributes and/or greatly limits view's flexibility.
Since we will be dropping REST anyway, I'll be switching it to Django's generic ListView. It causes far less problems anyway.

rwrzesien · 2019-03-10T23:57:44Z

managers/views.py

+        projects_queryset = self.get_queryset()
+        if request.user.user_type == CustomUser.UserType.MANAGER.name:
+            projects_queryset = Project.objects.filter(managers__id=request.user.pk)
+        if request.GET.get('sort'):


You can define sorting using pure rest framework functionality https://www.django-rest-framework.org/api-guide/filtering/#orderingfilter .

As for members and members_count it will be tricky, because defined items in ordering_fields must exists on queryset results. But on the other hand, we don't want to run annotate(members_count=Count('members') if it is not needed, as it results in decent extra SQL work. So you need to check if 'members' in request.GET.get('sort') in get_queryset method, and then add counting only then.

managers/views.py

rwrzesien · 2019-03-11T00:00:19Z

sheetstorm/urls.py

    url(r'^admin/', admin.site.urls),
    url(r'^', include('users.urls')),
-    url(r'^managers/', include('managers.urls')),
+    url(r'^', include('managers.urls')),


I think this should stay, as we discussed on project code review, we should keep endpoints separated.

managers/tests/test_unit_templatetags.py

rwrzesien · 2019-03-11T00:02:55Z

managers/tests/test_unit_templatetags.py

@@ -0,0 +1,104 @@
+import datetime


After moving template tags as custom manager methods please remove those tests too. Also I don't think there should be extra tests for custom manager methods, as those will be just some simple querysets.

rwrzesien

Looks good generally, there are some minor issues but also two bigger improvements that should be applied:

using custom model manager instead of template tags
use built in sorting in rest framework view.

I think it would be good to focus on those two first.

rwrzesien · 2019-03-11T00:20:08Z

managers/views.py

+
+class ProjectsList(APIView):
+    renderer_classes = [renderers.TemplateHTMLRenderer]
+    template_name = 'managers/projects_list.html'


I didn't saw it at first, so I have dropped comment in the following pull request, so I will link it here #86 (comment) .

kbeker

Small changes to do:)

managers/tests/test_unit_managers_apiview.py

kbeker · 2019-03-11T14:55:43Z

managers/tests/test_unit_managers_apiview.py

+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, self.project.name)
+        projects_list = response.data['projects_queryset']
+        self.assertEqual(list(manager_project_list), list(projects_list))


Does it have to be converted to a list?

@kbeker Yes, because without that the test returns
AssertionError: <QuerySet [<Project: Example Project>]> != <QuerySet [<Project: Example Project>]>
I don't know why exactly.

managers/tests/test_unit_model.py

kbeker

Ready to merge

Because we need to merge it :)

MartynaAnnaGottschling added the feature New feature label Mar 8, 2019

MartynaAnnaGottschling self-assigned this Mar 8, 2019

MartynaAnnaGottschling requested review from Karrp, Szymiks, kbeker, maciejSamerdak and rwrzesien March 8, 2019 13:46