add projects #1

pitskod · 2019-02-17T21:35:39Z

No description provided.

twistedFantasy · 2019-02-19T18:40:29Z

ssm/ssm/projects/admin.py

+
+
+class ProjectAdmin(admin.ModelAdmin):
+    list_display = ['name', 'status', 'customer', 'get_members_count']


members may be a better name here

twistedFantasy · 2019-02-19T18:40:46Z

ssm/ssm/projects/admin.py

+    list_display = ['name', 'status', 'customer', 'get_members_count']
+    search_fields = ['name']
+    fieldsets = [
+        (None, {'fields': ['name', 'description', 'specification', 'status', 'customer', 'estimation_in_man_hours']}),


One Details tab for all information?

twistedFantasy · 2019-02-19T18:40:51Z

ssm/ssm/projects/admin.py

+    inlines = [MembershipInline]
+    list_filter = ['customer']
+
+    def get_members_count(self, obj):


members may be a better name here

twistedFantasy · 2019-02-19T18:41:10Z

ssm/ssm/projects/admin.py

+    list_filter = ['customer']
+
+    def get_members_count(self, obj):
+        return len(obj.members.filter(membershipmodel__date_left=None))


Please always use count() instead of len() on QuerySets if all you want to do is determine the number of records in the set. It's much more efficient to handle a count at the database level, using SQL's SELECT COUNT(*), and Django provides a count() method for precisely this reason.

twistedFantasy · 2019-02-19T18:41:18Z

ssm/ssm/projects/apps.py

+
+
+class ProjectsConfig(AppConfig):
+    name = 'projects'


Not needed, please remove

twistedFantasy · 2019-02-19T18:41:52Z

ssm/ssm/projects/migrations/0007_membershipmodel_hours_per_day.py

+            name='hours_per_day',
+            field=models.IntegerField(default=8, verbose_name='Hours per day'),
+        ),
+    ]


Please try to use less amount of migrations before making your pull request, in your case when you do one new app support from scratch you can always remove all migrations for project at the end and recreate them to have clear picture for reviewer without all these changes history. Of course you shouldn't use such deletion migration practice in all other cases, but for current workflow it's quite a good one

twistedFantasy · 2019-02-19T18:42:11Z

ssm/ssm/projects/models.py

+
+
+class Project(BaseModel):
+    name = models.CharField('Name', max_length=32, unique=True)


It doesn't guarantee "true" uniqueness here as "Project" and "pRoject" values will not be recognized as one value, also you don't handle spaces at the beginning or at the end of name, so "Project" and "Project " also will be different names, you will need to design uniqueness rule here, for example use [title()](https://docs.python.org/3/library/stdtypes.html#str.title) and [strip()](https://docs.python.org/3/library/stdtypes.html#str.strip) combination () for name value, the same problem was for Skill model where save(...) method was modified to always force lower() and strip():

def save(self, *args, **kwargs): self.name = self.name.lower().strip() super().save(*args, **kwargs)

twistedFantasy · 2019-02-19T18:43:25Z

ssm/ssm/projects/models.py

+class Project(BaseModel):
+    name = models.CharField('Name', max_length=32, unique=True)
+    description = models.TextField('Description', null=True, blank=True)
+    specification = models.FileField('Specification', null=True, blank=True)


You should store specification files in location which will be mapped to docker volume, otherwise you will loose all data after container deletion/recreation, you should be persistent here, also you will have file name collision here if different projects will have the same name for specification, for example specification.docs, so you need to modify model field with upload_to attribute, for example, models.FileField(upload_to='/var/ssm/data/project_{ID}/'), then create /var/ssm/data/ in Dockerfile with RUN mkdir -p /var/ssm/data/ command and map this location to docker volume in docker-compose-local.yml, something like this

volumes: ssm-data: driver: local ...... ssm: build: context: ../ volumes: - ssm_data:/var/ssm/data/

twistedFantasy · 2019-02-19T18:43:37Z

ssm/ssm/projects/models.py

+    name = models.CharField('Name', max_length=32, unique=True)
+    description = models.TextField('Description', null=True, blank=True)
+    specification = models.FileField('Specification', null=True, blank=True)
+    customer = models.ForeignKey(User, on_delete=models.DO_NOTHING, verbose_name='Customer', null=True, blank=True)


Do we need a separate field for customer? Can he be included in members like all other team with it's own role? What are props and cons of this design approach?

twistedFantasy · 2019-02-19T18:43:46Z

ssm/ssm/projects/models.py

+    description = models.TextField('Description', null=True, blank=True)
+    specification = models.FileField('Specification', null=True, blank=True)
+    customer = models.ForeignKey(User, on_delete=models.DO_NOTHING, verbose_name='Customer', null=True, blank=True)
+    start_date = models.DateField('Start date', null=True, blank=True)


Let's use title() convention for explanation text, so Start date should be changed to Start Date

twistedFantasy · 2019-02-19T18:43:55Z

ssm/ssm/projects/models.py

+    specification = models.FileField('Specification', null=True, blank=True)
+    customer = models.ForeignKey(User, on_delete=models.DO_NOTHING, verbose_name='Customer', null=True, blank=True)
+    start_date = models.DateField('Start date', null=True, blank=True)
+    end_date = models.DateField('End date', null=True, blank=True)


Should be changed to End Date

twistedFantasy · 2019-02-19T18:44:12Z

ssm/ssm/projects/models.py

+
+    class Meta:
+        app_label = 'projects'
+        ordering = ['-modified']


Please stay consistent here with ordering and Meta attributes, should be:

class Meta: app_label = 'projects' verbose_name_plural = 'Projects' ordering = ['-modified']

twistedFantasy · 2019-02-19T18:45:42Z

ssm/ssm/projects/models.py

+    user = models.ForeignKey(User, on_delete=models.CASCADE)
+    project = models.ForeignKey(Project, on_delete=models.CASCADE)
+    role = models.CharField('Role in project', choices=ROLES, max_length=64, blank=False, null=False,
+                            default=ROLES.developer)


Let's try to fit this line in one row, no need for detailed Role in project explanation, simple Role is already enough to clarify for what we use this field, and this change allow us to feet 119 limit which is a standard for us.
Also please stay consistent with order of attributes, it should be:

role = models.CharField('Role', max_length=64, blank=False, null=False, choices=ROLES, default=ROLES.developer)

twistedFantasy · 2019-02-19T18:45:48Z

ssm/ssm/projects/models.py

+    role = models.CharField('Role in project', choices=ROLES, max_length=64, blank=False, null=False,
+                            default=ROLES.developer)
+    hours_per_day = models.IntegerField('Hours per day', blank=False, null=False, default=8)
+    date_joined = models.DateField('Date joined')


Let's also stay consistent here in names, should be joined_date = models.DateField('Joined Date')

twistedFantasy · 2019-02-19T18:45:53Z

ssm/ssm/projects/models.py

+                            default=ROLES.developer)
+    hours_per_day = models.IntegerField('Hours per day', blank=False, null=False, default=8)
+    date_joined = models.DateField('Date joined')
+    date_left = models.DateField('Date of leaving', blank=True, null=True)


Should be left_date = models.DateField('Left Date', null=True, blank=True)

twistedFantasy · 2019-02-19T18:46:50Z

ssm/ssm/projects/models.py

+    class Meta:
+        verbose_name_plural = 'Project members'
+        unique_together = ('user', 'project', 'role')
+        ordering = ['user']


Please stay consistent here with ordering and Meta attributes like in other places:

class Meta: app_label = 'projects' verbose_name_plural = 'Project members' unique_together = ('user', 'project', 'role') ordering = ['user']

twistedFantasy · 2019-02-19T18:46:56Z

ssm/ssm/projects/permissions.py

+
+class IsProjectMemberOrStaff(permissions.BasePermission):
+    def has_object_permission(self, request, view, obj):
+


Empty line, please remove

twistedFantasy · 2019-02-19T18:47:01Z

ssm/ssm/projects/permissions.py

+
+class ReadOnlyOrStaff(permissions.BasePermission):
+    def has_permission(self, request, view):
+        return True if request.user.is_staff else request.method in permissions.SAFE_METHODS


Looks familiar, can we make it DRY?

twistedFantasy · 2019-02-19T18:47:54Z

ssm/ssm/projects/serializers.py

+from rest_framework.serializers import ModelSerializer
+from drf_dynamic_fields import DynamicFieldsMixin
+from rest_framework import serializers
+from drf_dynamic_fields import DynamicFieldsMixin


Please remove duplicate from drf_dynamic_fields import DynamicFieldsMixin import and also stay consistent with selected approach, either import the whole from rest_framework import serializers and use it class MembershipSerializer(serializers.ModelSerializer): or import all needed things at once from rest_framework.serializers import ModelSerializer, ReadOnlyField

twistedFantasy · 2019-02-19T18:48:00Z

ssm/ssm/projects/serializers.py

+
+class ProjectSerializer(DynamicFieldsMixin, ModelSerializer):
+    members = MembershipSerializer(source='membershipmodel_set', many=True, read_only=True)
+    customer = CustomerSerializer(many=False, read_only=True)


many=False already false by default, in most cases no need to specify default values

twistedFantasy · 2019-02-19T18:48:05Z

ssm/ssm/projects/tests.py

@@ -0,0 +1,3 @@
+from django.test import TestCase
+
+# Create your tests here.


Please remove, it's not needed for now

twistedFantasy · 2019-02-19T18:48:12Z

ssm/ssm/projects/views.py

+from rest_framework.filters import OrderingFilter
+from rest_framework.permissions import IsAuthenticated
+from django_filters.rest_framework import DjangoFilterBackend
+from rest_framework.filters import OrderingFilter, SearchFilter


Please change the order of imports, imports from one package should go one by one:

from django_filters.rest_framework import DjangoFilterBackend from rest_framework.viewsets import ModelViewSet from rest_framework.filters import OrderingFilter from rest_framework.permissions import IsAuthenticated from rest_framework.filters import OrderingFilter, SearchFilter

twistedFantasy · 2019-02-19T18:48:18Z

ssm/ssm/projects/views.py

+    filter_fields = ['customer__full_name']
+    ordering = ['name']
+
+    def get_queryset(self, *args, **kwargs):


Please remove custom get_queryset and use filter instead

twistedFantasy · 2019-02-19T18:48:30Z

ssm/ssm/users/views.py

@@ -23,7 +23,7 @@ class SSMTokenObtainPairView(CustomTokenObtainPairView):
 class UserViewSet(ModelViewSet):
    queryset = User.objects.all()
    serializer_class = UserSerializer
-    permission_classes = [ReadOnlyOrStaff, IsCurrentUserOrStaff]
+    permission_classes = [ReadOnlyOrStaff, IsCurrentUserOrStaff, IsAuthenticated]


IsAuthenticated should go first, consistency everywhere

twistedFantasy

Made initial review and added comments for all places which should be changed/discussed. Also note that I pushed some updates in original staff project which you may need to merge in order to not have conflicts later

pitskod · 2019-03-10T19:17:35Z

I finally fixed code after code review)

add projects

7146f2b

twistedFantasy reviewed Feb 19, 2019

View reviewed changes

ssm/ssm/projects/apps.py Outdated

class ProjectsConfig(AppConfig):

name = 'projects'

Copy link

Owner

twistedFantasy Feb 19, 2019

Choose a reason for hiding this comment

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

Not needed, please remove

twistedFantasy reviewed Feb 19, 2019

View reviewed changes

twistedFantasy requested changes Feb 19, 2019

View reviewed changes

pitskod and others added 3 commits February 24, 2019 19:14

merge

c038f6a

some fix

83446b4

fix after code review

a609ebc

twistedFantasy approved these changes Apr 19, 2019

View reviewed changes

twistedFantasy merged commit fa0d3a8 into twistedFantasy:master Apr 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add projects #1

add projects #1

pitskod commented Feb 17, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019 •

edited

Loading

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy Feb 19, 2019

twistedFantasy left a comment •

edited

Loading

pitskod commented Mar 10, 2019



		class ProjectAdmin(admin.ModelAdmin):
		list_display = ['name', 'status', 'customer', 'get_members_count']



		class Project(BaseModel):
		name = models.CharField('Name', max_length=32, unique=True)


		class IsProjectMemberOrStaff(permissions.BasePermission):
		def has_object_permission(self, request, view, obj):

		@@ -0,0 +1,3 @@
		from django.test import TestCase

		# Create your tests here.

add projects #1

add projects #1

Conversation

pitskod commented Feb 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twistedFantasy Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twistedFantasy left a comment • edited Loading

Choose a reason for hiding this comment

pitskod commented Mar 10, 2019

twistedFantasy Feb 19, 2019 •

edited

Loading

twistedFantasy left a comment •

edited

Loading