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}
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. Explained here.
|
|
||
| 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}') |
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. Read more.
| # 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. More info.
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="". Explained here.
| 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.
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. More info.
| 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-certifications-checklist
branch
from
34b9593 to
1521756
Compare
| # 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. Read 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. 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. Explained 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 info.
|
|
||
| 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.
| 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, | |
| ) |
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 info.
| 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.
| 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.
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
| <title>jQuery UI Sortable - Default functionality</title> | ||
| <link rel="stylesheet" href="//code.jquery.com/ui/1.13.0/themes/base/jquery-ui.css"> | ||
| <link rel="stylesheet" href="/resources/demos/style.css"> |
There was a problem hiding this comment.
Hard-coding /resources/demos/style.css is brittle because the place the files are stored depends on the STATICFILES_STORAGE used - so if in prod the storage backend uploads to S3 or even renames the file then this hard-coded URL will break. Using {% static ... %} solves that as it knows exactly where the files are stored. Remember to {% load static %}. More details.
Codecov Report
@@ Coverage Diff @@
## develop #14 +/- ##
===========================================
+ Coverage 63.41% 64.56% +1.15%
===========================================
Files 179 185 +6
Lines 9388 9852 +464
Branches 711 1254 +543
===========================================
+ Hits 5953 6361 +408
- Misses 3205 3238 +33
- Partials 230 253 +23
Continue to review full report at Codecov.
|
sumandari
force-pushed
the
1379-certifications-checklist
branch
from
1521756 to
32fce95
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}') |
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}') |
Same as above: 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. More info.
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, | |
| ) |
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.
Similarly, with an explicit related_name would be better.
sumandari
force-pushed
the
1379-certifications-checklist
branch
from
247dfc3 to
477db89
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 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}') |
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}') |
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.
| # 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.
| 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.
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="". Explained here.
| 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, | |
| ) |
As above, 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. More details.
| 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-certifications-checklist
branch
from
477db89 to
db63d6e
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}') |
Same as above: 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. Explained 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. 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. Read more.
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="". Explained here.
| 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.
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. Explained here.
| 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.
2 participants
for generate image