Conversation
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
![code-review-doctor[bot]](https://avatars.githubusercontent.com/in/83430?s=60&v=4){kind=small}
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| CertificateManagementTemplateView, self).get_context_data(**kwargs) |
There was a problem hiding this comment.
| 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.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| ProjectCertificateTypeView, self).get_context_data(*kwargs) |
There was a problem hiding this comment.
| ProjectCertificateTypeView, self).get_context_data(*kwargs) | |
| ).get_context_data(*kwargs) |
It's unnecessary to use arguments when calling super for the parent class. More.
| response = self.client.get(reverse( | ||
| 'worksheet-sampledata', kwargs=self.kwargs_worksheet_full)) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEquals( |
There was a problem hiding this comment.
| self.assertEquals( | |
| self.assertEqual( |
self.assertEquals is deprecated. Use self.assertEqual instead. More details.
| zip_response = HttpResponse( | ||
| s.getvalue(), content_type="application/x-zip-compressed") | ||
| zip_response['Content-Disposition'] = \ | ||
| 'attachment; filename={}.zip'.format(file_title) |
There was a problem hiding this comment.
f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.
| class Meta: | ||
| model = CertificateType | ||
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) |
There was a problem hiding this comment.
| 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.
| description = models.TextField( | ||
| help_text=_('Certificate type description - 1000 characters limit.'), | ||
| max_length=1000, | ||
| null=True, | ||
| blank=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.
| class ProjectCertificateType(models.Model): | ||
| """A model to store a certificate type linked to a project""" | ||
|
|
||
| project = models.ForeignKey( |
There was a problem hiding this comment.
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.
| Project, | ||
| on_delete=models.CASCADE | ||
| ) | ||
| certificate_type = models.ForeignKey( |
There was a problem hiding this comment.
Again, with an explicit related_name would be better.
| certificate_type = models.ForeignKey( | ||
| CertificateType, on_delete=models.PROTECT, null=True) |
There was a problem hiding this comment.
| 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.
| 'this name is already exists!') | ||
|
|
||
|
|
||
| class CertifyingOrganisationSearchMixin(object): |
There was a problem hiding this comment.
| 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 Report
@@ 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
Continue to review full report at Codecov.
|
sumandari
force-pushed
the
1379-part2-1377-unapproved-list
branch
from
f35670e to
36730de
Compare
There was a problem hiding this comment.
Looks good. Worth considering though. View full project report here.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| CertificateManagementTemplateView, self).get_context_data(**kwargs) |
There was a problem hiding this comment.
| 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.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| ProjectCertificateTypeView, self).get_context_data(*kwargs) |
There was a problem hiding this comment.
| 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.
| 'this name is already exists!') | ||
|
|
||
|
|
||
| class CertifyingOrganisationSearchMixin(object): |
There was a problem hiding this comment.
| 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.
| response = self.client.get(reverse( | ||
| 'worksheet-sampledata', kwargs=self.kwargs_worksheet_full)) | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEquals( |
There was a problem hiding this comment.
| self.assertEquals( | |
| self.assertEqual( |
self.assertEquals is deprecated. Use self.assertEqual instead. More details.
| zip_response = HttpResponse( | ||
| s.getvalue(), content_type="application/x-zip-compressed") | ||
| zip_response['Content-Disposition'] = \ | ||
| 'attachment; filename={}.zip'.format(file_title) |
There was a problem hiding this comment.
f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.
| name = models.CharField( | ||
| help_text=_('Certificate type.'), | ||
| max_length=200, | ||
| null=False, | ||
| blank=False, | ||
| unique=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| description = models.TextField( | ||
| help_text=_('Certificate type description - 1000 characters limit.'), | ||
| max_length=1000, | ||
| null=True, | ||
| blank=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.
| class ProjectCertificateType(models.Model): | ||
| """A model to store a certificate type linked to a project""" | ||
|
|
||
| project = models.ForeignKey( |
There was a problem hiding this comment.
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.
| Project, | ||
| on_delete=models.CASCADE | ||
| ) | ||
| certificate_type = models.ForeignKey( |
There was a problem hiding this comment.
Likewise, with an explicit related_name would be better.
|
|
||
|
|
||
| class CertifyingOrganisationChecklist(models.Model): | ||
| question = models.ForeignKey( |
There was a problem hiding this comment.
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.
| CertificateChecklist, | ||
| on_delete=models.PROTECT | ||
| ) | ||
| certifying_organisation = models.ForeignKey( |
There was a problem hiding this comment.
Again, with an explicit related_name would be better.
| try: | ||
| return super( | ||
| CertifyingOrganisationUpdateView, self).form_valid(form) | ||
| super(CertifyingOrganisationUpdateView, self).form_valid(form) |
There was a problem hiding this comment.
| 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.
| """ | ||
|
|
||
| context = super( | ||
| CertifyingOrganisationReviewView, self).get_context_data() |
There was a problem hiding this comment.
| CertifyingOrganisationReviewView, self).get_context_data() | |
| ).get_context_data() |
It's unnecessary to use arguments when calling super for the parent class. Read more.
sumandari
force-pushed
the
1379-part2-1377-unapproved-list
branch
from
c7eb2c6 to
f91ffe0
Compare
| class Meta: | ||
| model = CertificateType | ||
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) |
There was a problem hiding this comment.
| 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.
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) | ||
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Description certificate type %s' % n) | |
| lambda n: f'Description certificate type {n}') |
Same as above: Consider using f-string instead.
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) | ||
| wording = factory.sequence( | ||
| lambda n: 'Wording certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Wording certificate type %s' % n) | |
| lambda n: f'Wording certificate type {n}') |
Similarly, Consider using f-string instead.
| 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) |
There was a problem hiding this comment.
| 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.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| CertificateManagementTemplateView, self).get_context_data(**kwargs) |
There was a problem hiding this comment.
| 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.
| name = models.CharField( | ||
| help_text=_('Certificate type.'), | ||
| max_length=200, | ||
| null=False, | ||
| blank=False, | ||
| unique=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| description = models.TextField( | ||
| help_text=_('Certificate type description - 1000 characters limit.'), | ||
| max_length=1000, | ||
| null=True, | ||
| blank=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.
| class ProjectCertificateType(models.Model): | ||
| """A model to store a certificate type linked to a project""" | ||
|
|
||
| project = models.ForeignKey( |
There was a problem hiding this comment.
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.
| Project, | ||
| on_delete=models.CASCADE | ||
| ) | ||
| certificate_type = models.ForeignKey( |
There was a problem hiding this comment.
Same as above: with an explicit related_name would be better.
sumandari
force-pushed
the
1379-part2-1377-unapproved-list
branch
from
f91ffe0 to
06ce99e
Compare
There was a problem hiding this comment.
Looks good. Worth considering though. View full project report here.
| class Meta: | ||
| model = CertificateType | ||
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) |
There was a problem hiding this comment.
| 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.
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) | ||
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Description certificate type %s' % n) | |
| lambda n: f'Description certificate type {n}') |
As above, Consider using f-string instead.
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) | ||
| wording = factory.sequence( | ||
| lambda n: 'Wording certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Wording certificate type %s' % n) | |
| lambda n: f'Wording certificate type {n}') |
Likewise, Consider using f-string instead.
| 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) |
There was a problem hiding this comment.
| 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.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| CertificateManagementTemplateView, self).get_context_data(**kwargs) |
There was a problem hiding this comment.
| 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.
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.
| class ProjectCertificateType(models.Model): | ||
| """A model to store a certificate type linked to a project""" | ||
|
|
||
| project = models.ForeignKey( |
There was a problem hiding this comment.
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.
| Project, | ||
| on_delete=models.CASCADE | ||
| ) | ||
| certificate_type = models.ForeignKey( |
There was a problem hiding this comment.
As above, with an explicit related_name would be better.
| certificate_type = models.ForeignKey( | ||
| CertificateType, on_delete=models.PROTECT, null=True) |
There was a problem hiding this comment.
| 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.
| zip_response = HttpResponse( | ||
| s.getvalue(), content_type="application/x-zip-compressed") | ||
| zip_response['Content-Disposition'] = \ | ||
| 'attachment; filename={}.zip'.format(file_title) |
There was a problem hiding this comment.
f-string is easier to read, write, and less computationally expensive than legacy string formatting. More.
There was a problem hiding this comment.
Looks good. Worth considering though. View full project report here.
| applicant_response = models.CharField( | ||
| help_text=_('Response from applicant.'), | ||
| max_length=1000, | ||
| blank=True, | ||
| null=True | ||
| ) |
There was a problem hiding this comment.
| 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.
sumandari
force-pushed
the
1379-part2-1377-unapproved-list
branch
from
aca2039 to
89603b7
Compare
| class Meta: | ||
| model = CertificateType | ||
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) |
There was a problem hiding this comment.
| 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.
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) | ||
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Description certificate type %s' % n) | |
| lambda n: f'Description certificate type {n}') |
Again, Consider using f-string instead.
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) | ||
| wording = factory.sequence( | ||
| lambda n: 'Wording certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Wording certificate type %s' % n) | |
| lambda n: f'Wording certificate type {n}') |
Likewise, Consider using f-string instead.
| 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) |
There was a problem hiding this comment.
| 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.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| CertificateManagementTemplateView, self).get_context_data(**kwargs) |
There was a problem hiding this comment.
| 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.
| question = models.ForeignKey( | ||
| CertificateChecklist, | ||
| on_delete=models.PROTECT | ||
| ) |
There was a problem hiding this comment.
As above, with an explicit related_name would be better.
| is_checked = models.BooleanField( | ||
| help_text=_('Is the answer is yes or no'), | ||
| null=True | ||
| ) |
There was a problem hiding this comment.
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.
| name = models.CharField( | ||
| help_text=_('Certificate type.'), | ||
| max_length=200, | ||
| null=False, | ||
| blank=False, | ||
| unique=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| description = models.TextField( | ||
| help_text=_('Certificate type description - 1000 characters limit.'), | ||
| max_length=1000, | ||
| null=True, | ||
| blank=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.
| class ProjectCertificateType(models.Model): | ||
| """A model to store a certificate type linked to a project""" | ||
|
|
||
| project = models.ForeignKey( |
There was a problem hiding this comment.
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.
| Project, | ||
| on_delete=models.CASCADE | ||
| ) | ||
| certificate_type = models.ForeignKey( |
There was a problem hiding this comment.
Similarly, with an explicit related_name would be better.
sumandari
force-pushed
the
1379-part2-1377-unapproved-list
branch
from
89603b7 to
228563a
Compare
| class Meta: | ||
| model = CertificateType | ||
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) |
There was a problem hiding this comment.
| 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.
|
|
||
| name = factory.sequence(lambda n: 'Test certificate type name %s' % n) | ||
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Description certificate type %s' % n) | |
| lambda n: f'Description certificate type {n}') |
As above, Consider using f-string instead.
| description = factory.sequence( | ||
| lambda n: 'Description certificate type %s' % n) | ||
| wording = factory.sequence( | ||
| lambda n: 'Wording certificate type %s' % n) |
There was a problem hiding this comment.
| lambda n: 'Wording certificate type %s' % n) | |
| lambda n: f'Wording certificate type {n}') |
Again, Consider using f-string instead.
| 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) |
There was a problem hiding this comment.
| 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.
| # Navbar data | ||
| self.project_slug = self.kwargs.get('project_slug', None) | ||
| context = super( | ||
| CertificateManagementTemplateView, self).get_context_data(**kwargs) |
There was a problem hiding this comment.
| CertificateManagementTemplateView, self).get_context_data(**kwargs) | |
| ).get_context_data(**kwargs) |
It's unnecessary to use arguments when calling super for the parent class. More.
| name = models.CharField( | ||
| help_text=_('Certificate type.'), | ||
| max_length=200, | ||
| null=False, | ||
| blank=False, | ||
| unique=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| description = models.TextField( | ||
| help_text=_('Certificate type description - 1000 characters limit.'), | ||
| max_length=1000, | ||
| null=True, | ||
| blank=True, | ||
| ) |
There was a problem hiding this comment.
| 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.
| 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 | ||
| ) |
There was a problem hiding this comment.
| 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.
| class ProjectCertificateType(models.Model): | ||
| """A model to store a certificate type linked to a project""" | ||
|
|
||
| project = models.ForeignKey( |
There was a problem hiding this comment.
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.
| Project, | ||
| on_delete=models.CASCADE | ||
| ) | ||
| certificate_type = models.ForeignKey( |
There was a problem hiding this comment.
As above, with an explicit related_name would be better.
2 participants
for creating image