1371 certification type #13
Conversation
Fix dockerfile and documentation for dev env
…2_fix_update_sustaining_member_500
…3_delete_converner_mechanism
…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}
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( |
There was a problem hiding this comment.
| context = super( | |
| ).get_context_data(*kwargs) |
It's unnecessary to use arguments when calling super for the parent class. More info.
| 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. 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.
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.
Likewise, redundant default arguments can be removed.
| 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 details.
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. More details.
| 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}') |
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}') |
As above, Consider using f-string instead.
| 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. Read more.
Codecov Report
@@ Coverage Diff @@
## develop #13 +/- ##
===========================================
+ Coverage 63.41% 64.74% +1.33%
===========================================
Files 179 182 +3
Lines 9388 9773 +385
Branches 711 1242 +531
===========================================
+ Hits 5953 6328 +375
+ Misses 3205 3195 -10
- Partials 230 250 +20
Continue to review full report at Codecov.
|
…mechanism Add Mechanism to Delete Course Converner
…1_certification_type
for automating building image purpose