1379 part2 1377 unapproved list #15

sumandari · 2022-01-21T12:14:45Z

for creating image

Fix dockerfile and documentation for dev env

…2_fix_update_sustaining_member_500

…ning_member_500 Fix raising ValidationError in validate_email_address

…p_in_zip_sampledata extract zipfile inside zipfile in sample data lesson

…d admin

code-review-doctor

Some food for thought. View full project report here.

code-review-doctor · 2022-01-21T12:15:10Z

django_project/certification/views/certificate_management.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            CertificateManagementTemplateView, self).get_context_data(**kwargs)


Suggested change

CertificateManagementTemplateView, self).get_context_data(**kwargs)

).get_context_data(**kwargs)

It's unnecessary to use arguments when calling super for the parent class. More info.

code-review-doctor · 2022-01-21T12:15:10Z

django_project/certification/views/certificate_type.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            ProjectCertificateTypeView, self).get_context_data(*kwargs)


Suggested change

ProjectCertificateTypeView, self).get_context_data(*kwargs)

).get_context_data(*kwargs)

It's unnecessary to use arguments when calling super for the parent class. More.

code-review-doctor · 2022-01-21T12:15:10Z

django_project/lesson/tests/test_worksheet_views.py

+        response = self.client.get(reverse(
+            'worksheet-sampledata', kwargs=self.kwargs_worksheet_full))
+        self.assertEqual(response.status_code, 200)
+        self.assertEquals(


Suggested change

self.assertEquals(

self.assertEqual(

self.assertEquals is deprecated. Use self.assertEqual instead. More details.

code-review-doctor · 2022-01-21T12:15:10Z

django_project/lesson/views/worksheet.py

+            zip_response = HttpResponse(
+                s.getvalue(), content_type="application/x-zip-compressed")
+            zip_response['Content-Disposition'] = \
+                'attachment; filename={}.zip'.format(file_title)


f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.

code-review-doctor · 2022-01-21T12:15:11Z

django_project/certification/tests/model_factories.py

+    class Meta:
+        model = CertificateType
+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)


Suggested change

name = factory.sequence(lambda n: 'Test certificate type name %s' % n)

name = factory.sequence(lambda n: f'Test certificate type name {n}')

f-string is easier to read, write, and less computationally expensive than legacy string formatting. More info.

code-review-doctor · 2022-01-21T12:15:12Z

django_project/certification/models/certificate_type.py

+    description = models.TextField(
+        help_text=_('Certificate type description - 1000 characters limit.'),
+        max_length=1000,
+        null=True,
+        blank=True,
+    )


Suggested change

description = models.TextField(

help_text=_('Certificate type description - 1000 characters limit.'),

max_length=1000,

null=True,

blank=True,

)

description = models.TextField(

help_text=_("Certificate type description - 1000 characters limit."),

max_length=1000,

default="",

blank=True,

)

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More.

code-review-doctor · 2022-01-21T12:15:12Z

django_project/certification/models/certificate_type.py

+    wording = models.CharField(
+        help_text=_(
+            'Wording that will be placed on certificate. '
+            'e.g. "Has attended and completed".'
+        ),
+        max_length=500,
+        null=False,
+        blank=False
+    )


Suggested change

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. '

'e.g. "Has attended and completed".'

),

max_length=500,

null=False,

blank=False

)

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. e.g. "Has attended and completed".'

),

max_length=500,

)

Similarly, redundant default arguments can be removed.

Same as above: redundant default arguments can be removed.

code-review-doctor · 2022-01-21T12:15:13Z

django_project/certification/models/certificate_type.py

+class ProjectCertificateType(models.Model):
+    """A model to store a certificate type linked to a project"""
+
+    project = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. More details.

code-review-doctor · 2022-01-21T12:15:13Z

django_project/certification/models/certificate_type.py

+        Project,
+        on_delete=models.CASCADE
+    )
+    certificate_type = models.ForeignKey(


Again, with an explicit related_name would be better.

code-review-doctor · 2022-01-21T12:15:13Z

django_project/certification/models/course.py

+    certificate_type = models.ForeignKey(
+        CertificateType, on_delete=models.PROTECT, null=True)


Suggested change

certificate_type = models.ForeignKey(

CertificateType, on_delete=models.PROTECT, null=True)

certificate_type = models.ForeignKey(

CertificateType, on_delete=models.PROTECT, null=True, blank=True

)

Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. Explained here.

Expect unwanted behavior if null and blank are different values: null controls if the database allows no value for certificate_type and blank controls if the application allows no value for certificate_type. Consider setting null and blank to the same value for certificate_type. Explained here.

code-review-doctor

Worth considering. View full project report here.

code-review-doctor · 2022-01-21T13:07:41Z

django_project/certification/views/certifying_organisation.py

@@ -651,10 +653,21 @@ def form_valid(self, form):
                'this name is already exists!')


+class CertifyingOrganisationSearchMixin(object):


Suggested change

class CertifyingOrganisationSearchMixin(object):

class CertifyingOrganisationSearchMixin:

CertifyingOrganisationSearchMixin inherits from object by default, so explicitly inheriting from object is redundant. Removing it keeps the code simpler. More details.

codecov-commenter · 2022-01-21T13:18:35Z

Codecov Report

Merging #15 (228563a) into develop (5281142) will increase coverage by 0.57%.
The diff coverage is 67.31%.

@@             Coverage Diff             @@
##           develop      #15      +/-   ##
===========================================
+ Coverage    63.41%   63.98%   +0.57%     
===========================================
  Files          179      185       +6     
  Lines         9388     9980     +592     
  Branches       711     1282     +571     
===========================================
+ Hits          5953     6386     +433     
- Misses        3205     3341     +136     
- Partials       230      253      +23

Impacted Files	Coverage Δ
django_project/certification/urls.py	`100.00% <ø> (ø)`
django_project/core/settings/contrib.py	`100.00% <ø> (ø)`
django_project/core/settings/project.py	`100.00% <ø> (ø)`
django_project/lesson/models/specification.py	`90.32% <ø> (ø)`
django_project/lesson/urls.py	`86.66% <ø> (ø)`
django_project/certification/forms.py	`54.98% <4.34%> (-4.05%)`	⬇️
...ect/certification/views/certifying_organisation.py	`41.56% <14.73%> (-7.97%)`	⬇️
...oject/certification/views/certificate_checklist.py	`39.58% <39.58%> (ø)`
django_project/certification/views/certificate.py	`52.44% <40.00%> (-0.20%)`	⬇️
django_project/certification/models/status.py	`90.00% <50.00%> (-4.45%)`	⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44a9d23...228563a. Read the comment docs.

code-review-doctor

Looks good. Worth considering though. View full project report here.

code-review-doctor · 2022-01-22T10:23:12Z

django_project/certification/views/certificate_management.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            CertificateManagementTemplateView, self).get_context_data(**kwargs)


Suggested change

CertificateManagementTemplateView, self).get_context_data(**kwargs)

).get_context_data(**kwargs)

It's unnecessary to use arguments when calling super for the parent class. Read more.

code-review-doctor · 2022-01-22T10:23:12Z

django_project/certification/views/certificate_type.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            ProjectCertificateTypeView, self).get_context_data(*kwargs)


Suggested change

ProjectCertificateTypeView, self).get_context_data(*kwargs)

).get_context_data(*kwargs)

It's unnecessary to use arguments when calling super for the parent class. Explained here.

code-review-doctor · 2022-01-22T10:23:13Z

django_project/certification/views/certifying_organisation.py

@@ -651,10 +653,21 @@ def form_valid(self, form):
                'this name is already exists!')


+class CertifyingOrganisationSearchMixin(object):


Suggested change

class CertifyingOrganisationSearchMixin(object):

class CertifyingOrganisationSearchMixin:

CertifyingOrganisationSearchMixin inherits from object by default, so explicitly inheriting from object is redundant. Removing it keeps the code simpler. More details.

code-review-doctor · 2022-01-22T10:23:13Z

django_project/lesson/tests/test_worksheet_views.py

+        response = self.client.get(reverse(
+            'worksheet-sampledata', kwargs=self.kwargs_worksheet_full))
+        self.assertEqual(response.status_code, 200)
+        self.assertEquals(


Suggested change

self.assertEquals(

self.assertEqual(

self.assertEquals is deprecated. Use self.assertEqual instead. More details.

code-review-doctor · 2022-01-22T10:23:13Z

django_project/lesson/views/worksheet.py

+            zip_response = HttpResponse(
+                s.getvalue(), content_type="application/x-zip-compressed")
+            zip_response['Content-Disposition'] = \
+                'attachment; filename={}.zip'.format(file_title)


f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.

code-review-doctor · 2022-01-22T10:23:16Z

django_project/certification/models/certificate_type.py

+    name = models.CharField(
+        help_text=_('Certificate type.'),
+        max_length=200,
+        null=False,
+        blank=False,
+        unique=True,
+    )


Suggested change

name = models.CharField(

help_text=_('Certificate type.'),

max_length=200,

null=False,

blank=False,

unique=True,

)

name = models.CharField(help_text=_("Certificate type."), max_length=200, unique=True)

False is the default value Django uses for null, so null=False can be removed. More details.

Again, redundant default arguments can be removed.

code-review-doctor · 2022-01-22T10:23:16Z

django_project/certification/models/certificate_type.py

+    description = models.TextField(
+        help_text=_('Certificate type description - 1000 characters limit.'),
+        max_length=1000,
+        null=True,
+        blank=True,
+    )


Suggested change

description = models.TextField(

help_text=_('Certificate type description - 1000 characters limit.'),

max_length=1000,

null=True,

blank=True,

)

description = models.TextField(

help_text=_("Certificate type description - 1000 characters limit."),

max_length=1000,

default="",

blank=True,

)

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More info.

code-review-doctor · 2022-01-22T10:23:16Z

django_project/certification/models/certificate_type.py

+    wording = models.CharField(
+        help_text=_(
+            'Wording that will be placed on certificate. '
+            'e.g. "Has attended and completed".'
+        ),
+        max_length=500,
+        null=False,
+        blank=False
+    )


Suggested change

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. '

'e.g. "Has attended and completed".'

),

max_length=500,

null=False,

blank=False

)

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. e.g. "Has attended and completed".'

),

max_length=500,

)

Again, redundant default arguments can be removed.

As above, redundant default arguments can be removed.

code-review-doctor · 2022-01-22T10:23:16Z

django_project/certification/models/certificate_type.py

+class ProjectCertificateType(models.Model):
+    """A model to store a certificate type linked to a project"""
+
+    project = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. More details.

code-review-doctor · 2022-01-22T10:23:17Z

django_project/certification/models/certificate_type.py

+        Project,
+        on_delete=models.CASCADE
+    )
+    certificate_type = models.ForeignKey(


Likewise, with an explicit related_name would be better.

code-review-doctor

Some things to consider. View full project report here.

code-review-doctor · 2022-01-27T17:56:54Z

django_project/certification/models/certificate_checklist.py

+
+
+class CertifyingOrganisationChecklist(models.Model):
+    question = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. Explained here.

code-review-doctor · 2022-01-27T17:56:55Z

django_project/certification/models/certificate_checklist.py

+        CertificateChecklist,
+        on_delete=models.PROTECT
+    )
+    certifying_organisation = models.ForeignKey(


Again, with an explicit related_name would be better.

code-review-doctor · 2022-01-27T17:56:55Z

django_project/certification/views/certifying_organisation.py

@@ -643,24 +698,38 @@ def form_valid(self, form):
        """Check that there is no referential integrity error when saving."""

        try:
-            return super(
-                CertifyingOrganisationUpdateView, self).form_valid(form)
+            super(CertifyingOrganisationUpdateView, self).form_valid(form)


Suggested change

super(CertifyingOrganisationUpdateView, self).form_valid(form)

super().form_valid(form)

It's unnecessary to use arguments when calling super for the parent class. Read more.

code-review-doctor

Some food for thought. View full project report here.

code-review-doctor · 2022-01-28T06:34:36Z

django_project/certification/views/certifying_organisation.py

+        """
+
+        context = super(
+            CertifyingOrganisationReviewView, self).get_context_data()


Suggested change

CertifyingOrganisationReviewView, self).get_context_data()

).get_context_data()

It's unnecessary to use arguments when calling super for the parent class. Read more.

code-review-doctor

Some food for thought. View full project report here.

code-review-doctor · 2022-01-28T06:37:54Z

django_project/certification/tests/model_factories.py

+    class Meta:
+        model = CertificateType
+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)


Suggested change

name = factory.sequence(lambda n: 'Test certificate type name %s' % n)

name = factory.sequence(lambda n: f'Test certificate type name {n}')

f-string is easier to read, write, and less computationally expensive than legacy string formatting. Read more.

code-review-doctor · 2022-01-28T06:37:54Z

django_project/certification/tests/model_factories.py

+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)
+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)


Suggested change

lambda n: 'Description certificate type %s' % n)

lambda n: f'Description certificate type {n}')

Same as above: Consider using f-string instead.

code-review-doctor · 2022-01-28T06:37:54Z

django_project/certification/tests/model_factories.py

+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)
+    wording = factory.sequence(
+        lambda n: 'Wording certificate type %s' % n)


Suggested change

lambda n: 'Wording certificate type %s' % n)

lambda n: f'Wording certificate type {n}')

Similarly, Consider using f-string instead.

code-review-doctor · 2022-01-28T06:37:54Z

django_project/certification/views/certificate_checklist.py

+        project_slug = self.kwargs.get('project_slug', None)
+        project = get_object_or_404(Project, slug=project_slug)
+        form.instance.project = project
+        return super(CertificateChecklistCreateView, self).form_valid(form)


Suggested change

return super(CertificateChecklistCreateView, self).form_valid(form)

return super().form_valid(form)

It's unnecessary to use arguments when calling super for the parent class. More info.

code-review-doctor · 2022-01-28T06:37:54Z

django_project/certification/views/certificate_management.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            CertificateManagementTemplateView, self).get_context_data(**kwargs)


Suggested change

CertificateManagementTemplateView, self).get_context_data(**kwargs)

).get_context_data(**kwargs)

It's unnecessary to use arguments when calling super for the parent class. Read more.

code-review-doctor · 2022-01-28T06:37:56Z

django_project/certification/models/certificate_type.py

+    name = models.CharField(
+        help_text=_('Certificate type.'),
+        max_length=200,
+        null=False,
+        blank=False,
+        unique=True,
+    )


Suggested change

name = models.CharField(

help_text=_('Certificate type.'),

max_length=200,

null=False,

blank=False,

unique=True,

)

name = models.CharField(help_text=_("Certificate type."), max_length=200, unique=True)

False is the default value Django uses for null, so null=False can be removed. Explained here.

Likewise, redundant default arguments can be removed.

code-review-doctor · 2022-01-28T06:37:57Z

django_project/certification/models/certificate_type.py

+    description = models.TextField(
+        help_text=_('Certificate type description - 1000 characters limit.'),
+        max_length=1000,
+        null=True,
+        blank=True,
+    )


Suggested change

description = models.TextField(

help_text=_('Certificate type description - 1000 characters limit.'),

max_length=1000,

null=True,

blank=True,

)

description = models.TextField(

help_text=_("Certificate type description - 1000 characters limit."),

max_length=1000,

default="",

blank=True,

)

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". Read more.

code-review-doctor · 2022-01-28T06:37:57Z

django_project/certification/models/certificate_type.py

+    wording = models.CharField(
+        help_text=_(
+            'Wording that will be placed on certificate. '
+            'e.g. "Has attended and completed".'
+        ),
+        max_length=500,
+        null=False,
+        blank=False
+    )


Suggested change

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. '

'e.g. "Has attended and completed".'

),

max_length=500,

null=False,

blank=False

)

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. e.g. "Has attended and completed".'

),

max_length=500,

)

Again, redundant default arguments can be removed.

Same as above: redundant default arguments can be removed.

code-review-doctor · 2022-01-28T06:37:57Z

django_project/certification/models/certificate_type.py

+class ProjectCertificateType(models.Model):
+    """A model to store a certificate type linked to a project"""
+
+    project = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. More.

code-review-doctor · 2022-01-28T06:37:57Z

django_project/certification/models/certificate_type.py

+        Project,
+        on_delete=models.CASCADE
+    )
+    certificate_type = models.ForeignKey(


Same as above: with an explicit related_name would be better.

code-review-doctor

Looks good. Worth considering though. View full project report here.

code-review-doctor · 2022-01-28T08:13:40Z

django_project/certification/tests/model_factories.py

+    class Meta:
+        model = CertificateType
+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)


Suggested change

name = factory.sequence(lambda n: 'Test certificate type name %s' % n)

name = factory.sequence(lambda n: f'Test certificate type name {n}')

f-string is easier to read, write, and less computationally expensive than legacy string formatting. More details.

code-review-doctor · 2022-01-28T08:13:40Z

django_project/certification/tests/model_factories.py

+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)
+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)


Suggested change

lambda n: 'Description certificate type %s' % n)

lambda n: f'Description certificate type {n}')

As above, Consider using f-string instead.

code-review-doctor · 2022-01-28T08:13:41Z

django_project/certification/tests/model_factories.py

+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)
+    wording = factory.sequence(
+        lambda n: 'Wording certificate type %s' % n)


Suggested change

lambda n: 'Wording certificate type %s' % n)

lambda n: f'Wording certificate type {n}')

Likewise, Consider using f-string instead.

code-review-doctor · 2022-01-28T08:13:41Z

django_project/certification/views/certificate_checklist.py

+        project_slug = self.kwargs.get('project_slug', None)
+        project = get_object_or_404(Project, slug=project_slug)
+        form.instance.project = project
+        return super(CertificateChecklistCreateView, self).form_valid(form)


Suggested change

return super(CertificateChecklistCreateView, self).form_valid(form)

return super().form_valid(form)

It's unnecessary to use arguments when calling super for the parent class. More details.

code-review-doctor · 2022-01-28T08:13:41Z

django_project/certification/views/certificate_management.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            CertificateManagementTemplateView, self).get_context_data(**kwargs)


Suggested change

CertificateManagementTemplateView, self).get_context_data(**kwargs)

).get_context_data(**kwargs)

It's unnecessary to use arguments when calling super for the parent class. More info.

code-review-doctor · 2022-01-28T08:13:45Z

django_project/certification/models/certificate_type.py

+    wording = models.CharField(
+        help_text=_(
+            'Wording that will be placed on certificate. '
+            'e.g. "Has attended and completed".'
+        ),
+        max_length=500,
+        null=False,
+        blank=False
+    )


Suggested change

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. '

'e.g. "Has attended and completed".'

),

max_length=500,

null=False,

blank=False

)

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. e.g. "Has attended and completed".'

),

max_length=500,

)

Same as above: redundant default arguments can be removed.

Similarly, redundant default arguments can be removed.

code-review-doctor · 2022-01-28T08:13:45Z

django_project/certification/models/certificate_type.py

+class ProjectCertificateType(models.Model):
+    """A model to store a certificate type linked to a project"""
+
+    project = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. More details.

code-review-doctor · 2022-01-28T08:13:45Z

django_project/certification/models/certificate_type.py

+        Project,
+        on_delete=models.CASCADE
+    )
+    certificate_type = models.ForeignKey(


As above, with an explicit related_name would be better.

code-review-doctor · 2022-01-28T08:13:45Z

django_project/certification/models/course.py

+    certificate_type = models.ForeignKey(
+        CertificateType, on_delete=models.PROTECT, null=True)


Suggested change

certificate_type = models.ForeignKey(

CertificateType, on_delete=models.PROTECT, null=True)

certificate_type = models.ForeignKey(

CertificateType, on_delete=models.PROTECT, null=True, blank=True

)

Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. More.

Expect unwanted behavior if null and blank are different values: null controls if the database allows no value for certificate_type and blank controls if the application allows no value for certificate_type. Consider setting null and blank to the same value for certificate_type. Explained here.

code-review-doctor · 2022-01-28T08:13:46Z

django_project/lesson/views/worksheet.py

+            zip_response = HttpResponse(
+                s.getvalue(), content_type="application/x-zip-compressed")
+            zip_response['Content-Disposition'] = \
+                'attachment; filename={}.zip'.format(file_title)


f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.

code-review-doctor

Looks good. Worth considering though. View full project report here.

code-review-doctor · 2022-02-11T07:37:07Z

django_project/certification/models/certificate_checklist.py

+    applicant_response = models.CharField(
+        help_text=_('Response from applicant.'),
+        max_length=1000,
+        blank=True,
+        null=True
+    )


Suggested change

applicant_response = models.CharField(

help_text=_('Response from applicant.'),

max_length=1000,

blank=True,

null=True

)

applicant_response = models.TextField(

help_text=_('Response from applicant.'), blank=True, default=''

)

TextField might be better used here, instead of CharField with a huge max_length. More details.

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". Explained here.

code-review-doctor

Worth considering. View full project report here.

code-review-doctor · 2022-02-11T07:41:48Z

django_project/certification/tests/model_factories.py

+    class Meta:
+        model = CertificateType
+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)


Suggested change

name = factory.sequence(lambda n: 'Test certificate type name %s' % n)

name = factory.sequence(lambda n: f'Test certificate type name {n}')

f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.

code-review-doctor · 2022-02-11T07:41:48Z

django_project/certification/tests/model_factories.py

+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)
+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)


Suggested change

lambda n: 'Description certificate type %s' % n)

lambda n: f'Description certificate type {n}')

Again, Consider using f-string instead.

code-review-doctor · 2022-02-11T07:41:48Z

django_project/certification/tests/model_factories.py

+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)
+    wording = factory.sequence(
+        lambda n: 'Wording certificate type %s' % n)


Suggested change

lambda n: 'Wording certificate type %s' % n)

lambda n: f'Wording certificate type {n}')

Likewise, Consider using f-string instead.

code-review-doctor · 2022-02-11T07:41:49Z

django_project/certification/views/certificate_checklist.py

+        project_slug = self.kwargs.get('project_slug', None)
+        project = get_object_or_404(Project, slug=project_slug)
+        form.instance.project = project
+        return super(CertificateChecklistCreateView, self).form_valid(form)


Suggested change

return super(CertificateChecklistCreateView, self).form_valid(form)

return super().form_valid(form)

It's unnecessary to use arguments when calling super for the parent class. More info.

code-review-doctor · 2022-02-11T07:41:49Z

django_project/certification/views/certificate_management.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            CertificateManagementTemplateView, self).get_context_data(**kwargs)


Suggested change

CertificateManagementTemplateView, self).get_context_data(**kwargs)

).get_context_data(**kwargs)

It's unnecessary to use arguments when calling super for the parent class. More details.

code-review-doctor · 2022-02-11T07:41:51Z

django_project/certification/models/certificate_checklist.py

+    question = models.ForeignKey(
+        CertificateChecklist,
+        on_delete=models.PROTECT
+    )


As above, with an explicit related_name would be better.

code-review-doctor · 2022-02-11T07:41:51Z

django_project/certification/models/certificate_checklist.py

+    is_checked = models.BooleanField(
+        help_text=_('Is the answer is yes or no'),
+        null=True
+    )


Likewise, consider using a TextField.

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More info.

code-review-doctor · 2022-02-11T07:41:51Z

django_project/certification/models/certificate_type.py

+    name = models.CharField(
+        help_text=_('Certificate type.'),
+        max_length=200,
+        null=False,
+        blank=False,
+        unique=True,
+    )


Suggested change

name = models.CharField(

help_text=_('Certificate type.'),

max_length=200,

null=False,

blank=False,

unique=True,

)

name = models.CharField(help_text=_("Certificate type."), max_length=200, unique=True)

False is the default value Django uses for null, so null=False can be removed. More details.

Same as above: redundant default arguments can be removed.

code-review-doctor · 2022-02-11T07:41:52Z

django_project/certification/models/certificate_type.py

+    description = models.TextField(
+        help_text=_('Certificate type description - 1000 characters limit.'),
+        max_length=1000,
+        null=True,
+        blank=True,
+    )


Suggested change

description = models.TextField(

help_text=_('Certificate type description - 1000 characters limit.'),

max_length=1000,

null=True,

blank=True,

)

description = models.TextField(

help_text=_("Certificate type description - 1000 characters limit."),

max_length=1000,

default="",

blank=True,

)

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More info.

code-review-doctor · 2022-02-11T07:41:52Z

django_project/certification/models/certificate_type.py

+    wording = models.CharField(
+        help_text=_(
+            'Wording that will be placed on certificate. '
+            'e.g. "Has attended and completed".'
+        ),
+        max_length=500,
+        null=False,
+        blank=False
+    )


Suggested change

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. '

'e.g. "Has attended and completed".'

),

max_length=500,

null=False,

blank=False

)

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. e.g. "Has attended and completed".'

),

max_length=500,

)

Again, redundant default arguments can be removed.

Again, redundant default arguments can be removed.

code-review-doctor

Worth considering. View full project report here.

code-review-doctor · 2022-02-11T07:41:58Z

django_project/certification/models/certificate_type.py

+class ProjectCertificateType(models.Model):
+    """A model to store a certificate type linked to a project"""
+
+    project = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. Explained here.

code-review-doctor · 2022-02-11T07:41:58Z

django_project/certification/models/certificate_type.py

+        Project,
+        on_delete=models.CASCADE
+    )
+    certificate_type = models.ForeignKey(


Similarly, with an explicit related_name would be better.

code-review-doctor

Worth considering. View full project report here.

code-review-doctor · 2022-02-11T07:44:31Z

django_project/certification/tests/model_factories.py

+    class Meta:
+        model = CertificateType
+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)


Suggested change

name = factory.sequence(lambda n: 'Test certificate type name %s' % n)

name = factory.sequence(lambda n: f'Test certificate type name {n}')

f-string is easier to read, write, and less computationally expensive than legacy string formatting. Read more.

code-review-doctor · 2022-02-11T07:44:31Z

django_project/certification/tests/model_factories.py

+
+    name = factory.sequence(lambda n: 'Test certificate type name %s' % n)
+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)


Suggested change

lambda n: 'Description certificate type %s' % n)

lambda n: f'Description certificate type {n}')

As above, Consider using f-string instead.

code-review-doctor · 2022-02-11T07:44:31Z

django_project/certification/tests/model_factories.py

+    description = factory.sequence(
+        lambda n: 'Description certificate type %s' % n)
+    wording = factory.sequence(
+        lambda n: 'Wording certificate type %s' % n)


Suggested change

lambda n: 'Wording certificate type %s' % n)

lambda n: f'Wording certificate type {n}')

Again, Consider using f-string instead.

code-review-doctor · 2022-02-11T07:44:32Z

django_project/certification/views/certificate_checklist.py

+        project_slug = self.kwargs.get('project_slug', None)
+        project = get_object_or_404(Project, slug=project_slug)
+        form.instance.project = project
+        return super(CertificateChecklistCreateView, self).form_valid(form)


Suggested change

return super(CertificateChecklistCreateView, self).form_valid(form)

return super().form_valid(form)

It's unnecessary to use arguments when calling super for the parent class. More info.

code-review-doctor · 2022-02-11T07:44:32Z

django_project/certification/views/certificate_management.py

+        # Navbar data
+        self.project_slug = self.kwargs.get('project_slug', None)
+        context = super(
+            CertificateManagementTemplateView, self).get_context_data(**kwargs)


Suggested change

CertificateManagementTemplateView, self).get_context_data(**kwargs)

).get_context_data(**kwargs)

It's unnecessary to use arguments when calling super for the parent class. More.

code-review-doctor · 2022-02-11T07:44:34Z

django_project/certification/models/certificate_type.py

+    name = models.CharField(
+        help_text=_('Certificate type.'),
+        max_length=200,
+        null=False,
+        blank=False,
+        unique=True,
+    )


Suggested change

name = models.CharField(

help_text=_('Certificate type.'),

max_length=200,

null=False,

blank=False,

unique=True,

)

name = models.CharField(help_text=_("Certificate type."), max_length=200, unique=True)

False is the default value Django uses for null, so null=False can be removed. Explained here.

Similarly, redundant default arguments can be removed.

code-review-doctor · 2022-02-11T07:44:34Z

django_project/certification/models/certificate_type.py

+    description = models.TextField(
+        help_text=_('Certificate type description - 1000 characters limit.'),
+        max_length=1000,
+        null=True,
+        blank=True,
+    )


Suggested change

description = models.TextField(

help_text=_('Certificate type description - 1000 characters limit.'),

max_length=1000,

null=True,

blank=True,

)

description = models.TextField(

help_text=_("Certificate type description - 1000 characters limit."),

max_length=1000,

default="",

blank=True,

)

null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More details.

code-review-doctor · 2022-02-11T07:44:35Z

django_project/certification/models/certificate_type.py

+    wording = models.CharField(
+        help_text=_(
+            'Wording that will be placed on certificate. '
+            'e.g. "Has attended and completed".'
+        ),
+        max_length=500,
+        null=False,
+        blank=False
+    )


Suggested change

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. '

'e.g. "Has attended and completed".'

),

max_length=500,

null=False,

blank=False

)

wording = models.CharField(

help_text=_(

'Wording that will be placed on certificate. e.g. "Has attended and completed".'

),

max_length=500,

)

Likewise, redundant default arguments can be removed.

Similarly, redundant default arguments can be removed.

code-review-doctor · 2022-02-11T07:44:35Z

django_project/certification/models/certificate_type.py

+class ProjectCertificateType(models.Model):
+    """A model to store a certificate type linked to a project"""
+
+    project = models.ForeignKey(


Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. Explained here.

code-review-doctor · 2022-02-11T07:44:35Z

django_project/certification/models/certificate_type.py

+        Project,
+        on_delete=models.CASCADE
+    )
+    certificate_type = models.ForeignKey(


As above, with an explicit related_name would be better.

sumandari and others added 30 commits May 17, 2021 10:57

extract zipfile inside zipfile in sample data lesson

c77e25c

update unit test for download sample data lesson

d09aeb5

update unit test

0092fff

fixed dockerfile for development and the documentation

7de9b96

fixed raising ValidationError in validate_email_address

547869c

fixed typo flake8

79e0987

Merge pull request kartoza#1374 from sumandari/fix_dev_docker_and_readme

bfab4ac

Fix dockerfile and documentation for dev env

Merge branch 'develop' of https://github.com/kartoza/prj.app into 137…

f4b45d3

…2_fix_update_sustaining_member_500

Merge pull request kartoza#1376 from sumandari/1372_fix_update_sustai…

7f837e5

…ning_member_500 Fix raising ValidationError in validate_email_address

Merge pull request kartoza#1341 from sumandari/1328_lesson_extract_zi…

c1a2cb5

…p_in_zip_sampledata extract zipfile inside zipfile in sample data lesson

added CertificateType model

613dbe7

added admin class

a1c5b72

added CertificateType as foreign key in Certificate models and update…

3fb6b64

…d admin

updated unittest

af6b582

added project certificate_type

ec6763c

part#2: enable certification manager to manage certificate type list

1f76a37

fixed typo flake8

935b99a

Part#3: added certificate_type in courseform

45ca0e3

removed certificate_type in certificate model

5c798d9

updated queryset in courseform

22db5b5

Part#4 update template

2adf94e

fixed typo flake8

5aeacd6

updated unit test and fix failed test

85bd0b4

updated unit test and fix failed test

cf89c41

checklist app add and move order

2cfa122

archive-activate checklist

32fce95

remove unused files

db63d6e

renamed certificate_checklist.js

0cf6479

fix pagination template

6a35524

add django-sorting-bootstrap dependency

96977b0

updated pending organisations list

a0680ee

code-review-doctor bot suggested changes Jan 21, 2022

View reviewed changes

added search

9c84388

code-review-doctor bot suggested changes Jan 21, 2022

View reviewed changes

inifinite scroll

36730de

sumandari force-pushed the 1379-part2-1377-unapproved-list branch from f35670e to 36730de Compare January 22, 2022 10:22

code-review-doctor bot suggested changes Jan 22, 2022

View reviewed changes

sumandari added 2 commits January 22, 2022 19:20

fix infinite scroll content

39c2904

add checklist on create and update page

646835f

code-review-doctor bot suggested changes Jan 27, 2022

View reviewed changes

code-review-doctor bot suggested changes Jan 28, 2022

View reviewed changes

sumandari force-pushed the 1379-part2-1377-unapproved-list branch from c7eb2c6 to f91ffe0 Compare January 28, 2022 06:37

code-review-doctor bot suggested changes Jan 28, 2022

View reviewed changes

add review

06ce99e

sumandari force-pushed the 1379-part2-1377-unapproved-list branch from f91ffe0 to 06ce99e Compare January 28, 2022 08:13

code-review-doctor bot suggested changes Jan 28, 2022

View reviewed changes

code-review-doctor bot suggested changes Feb 11, 2022

View reviewed changes

sumandari force-pushed the 1379-part2-1377-unapproved-list branch from aca2039 to 89603b7 Compare February 11, 2022 07:41

code-review-doctor bot suggested changes Feb 11, 2022

View reviewed changes

add response text field from applicant

228563a

sumandari force-pushed the 1379-part2-1377-unapproved-list branch from 89603b7 to 228563a Compare February 11, 2022 07:44

code-review-doctor bot suggested changes Feb 11, 2022

View reviewed changes

	CertificateManagementTemplateView, self).get_context_data(**kwargs)
	).get_context_data(**kwargs)

	ProjectCertificateTypeView, self).get_context_data(*kwargs)
	).get_context_data(*kwargs)

	name = factory.sequence(lambda n: 'Test certificate type name %s' % n)
	name = factory.sequence(lambda n: f'Test certificate type name {n}')

		certificate_type = models.ForeignKey(
		CertificateType, on_delete=models.PROTECT, null=True)

		@@ -651,10 +653,21 @@ def form_valid(self, form):
		'this name is already exists!')


		class CertifyingOrganisationSearchMixin(object):

	class CertifyingOrganisationSearchMixin(object):
	class CertifyingOrganisationSearchMixin:



		class CertifyingOrganisationChecklist(models.Model):
		question = models.ForeignKey(

	super(CertifyingOrganisationUpdateView, self).form_valid(form)
	super().form_valid(form)

	CertifyingOrganisationReviewView, self).get_context_data()
	).get_context_data()

	lambda n: 'Description certificate type %s' % n)
	lambda n: f'Description certificate type {n}')

	lambda n: 'Wording certificate type %s' % n)
	lambda n: f'Wording certificate type {n}')

	return super(CertificateChecklistCreateView, self).form_valid(form)
	return super().form_valid(form)

1379 part2 1377 unapproved list #15

Are you sure you want to change the base?

1379 part2 1377 unapproved list #15

Conversation

sumandari commented Jan 21, 2022

code-review-doctor bot left a comment

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

code-review-doctor bot left a comment

Choose a reason for hiding this comment

code-review-doctor bot Jan 21, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jan 21, 2022 • edited Loading

Codecov Report

code-review-doctor bot left a comment

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 22, 2022

Choose a reason for hiding this comment

code-review-doctor bot left a comment

Choose a reason for hiding this comment

code-review-doctor bot Jan 27, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 27, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 27, 2022

Choose a reason for hiding this comment

code-review-doctor bot left a comment

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

Choose a reason for hiding this comment

code-review-doctor bot left a comment

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

Choose a reason for hiding this comment

code-review-doctor bot Jan 28, 2022

codecov-commenter commented Jan 21, 2022 •

edited

Loading