django
code-review-doctorbotwould suggest these changes right inside your pull requests on GitHub.
config/settings.py
41
41
42
42
# https://docs.djangoproject.com/en/dev/ref/settings/#append-slash
43
43
APPEND_SLASH = True

Stating defaults add complexity when reading the code but does not change Django's behaviour.

Read more
Suggested changes
-
APPEND_SLASH = True
44
44
45
45
# Application definition
62
62
    'whitenoise.middleware.WhiteNoiseMiddleware',

The order of middleware affections the outcome. Some middleware are dependant on the functionality of other middleware. For example a middleware that requires usage of request.session should come after the SessionMiddleware.

Read more

Your website is vulnerable to a number of common hacker attacks because MIDDLEWARE setting is missing django.middleware.security.SecurityMiddleware.

Read more
Suggested changes
+
    'django.middleware.security.SecurityMiddleware',
Expand 12 lines ...
63
63
    'django.contrib.sessions.middleware.SessionMiddleware',
64
64
    'django.middleware.cache.UpdateCacheMiddleware',
65
65
    'django.middleware.common.CommonMiddleware',
66
66
    'django.middleware.cache.FetchFromCacheMiddleware',
67
67
    'django.middleware.csrf.CsrfViewMiddleware',
68
68
    'django.contrib.auth.middleware.AuthenticationMiddleware',
69
69
    'django.contrib.messages.middleware.MessageMiddleware',
70
70
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
71
71
]
72
72
73
73
ROOT_URLCONF = 'config.urls'
74
74
75
75
TEMPLATES = [

DIRS must be absolute paths. Relative paths will not work.

Read more
76
76
    {
77
77
        'BACKEND': 'django.template.backends.django.DjangoTemplates',
78
78
        'DIRS': ['core/templates'],
79
79
        'APP_DIRS': True,
80
80
        'OPTIONS': {
81
81
            'context_processors': [
82
82
                'django.template.context_processors.debug',
83
83
                'django.template.context_processors.request',
84
84
                'django.template.context_processors.i18n',
85
85
                'django.contrib.auth.context_processors.auth',
86
86
                'django.contrib.messages.context_processors.messages',
87
87
                'staticpages.context_processors.get_pages',
88
88
                'staticpages.context_processors.get_categories',
89
89
                'staticpages.context_processors.get_urls',
90
90
            ],
91
91
        },
92
92
    },
93
93
]
94
94
95
95
# Static files served with Whitenoise and AWS Cloudfront
119
119
SECURE_CONTENT_TYPE_NOSNIFF = True
120
120
121
121
USE_I18N = True

Stating defaults add complexity when reading the code but does not change Django's behaviour.

Read more
Suggested changes
-
USE_I18N = True
122
122
123
123
DIRECTORY_CONSTANTS_URL_GREAT_DOMESTIC = env.str(
148
148
SSO_PROFILE_URL = 'https://profile.com'

Your website is vulnerable because the CSRF_COOKIE_SECURE setting is not set - so hackers have an easier time stealing your CSRF cookies on HTTP connections, allowing them to circumvent your CSRF protection.

Read more

Your website is vulnerable because the SESSION_COOKIE_SECURE setting is not set - so hackers have an easier time stealing your users' session cookies on HTTP connections.

Read more
Suggested changes
+
+
+
SESSION_COOKIE_SECURE = os.getenv('SESSION_COOKIE_SECURE_ENABLED') != 'False'
+
+
CSRF_COOKIE_SECURE = os.getenv('CSRF_COOKIE_SECURE_ENABLED') != 'False'
apps/aarvark/students.py
5
5
6
6
class StudentPerformance(models.Model):
7
7
    stident_id = models.CharField(max_length=100, primary_key=True, unique=False)

Primary key must be unique by definition: it's a field that uniquely identifies the record.

Read more

Stating defaults add complexity when reading the code but does not change Django's behaviour.

Read more
Suggested changes
-
    stident_id = models.CharField(max_length=100, primary_key=True, unique=False)
+
    stident_id = models.CharField(max_length=100, primary_key=True)
Expand 2 lines ...
8
8
    registration_id = models.CharField(max_length=10, db_index=True)
9
9
    academic_year = models.IntegerField()
14
14
    courses_registration_validated = models.NullBooleanField(null=True)

NullBooleanField is deprecated and will be removed a future version of Django.

Read more
Suggested changes
-
    courses_registration_validated = models.NullBooleanField(null=True)
+
    courses_registration_validated = models.BooleanField(null=True)
apps/aarvark/widgets.py
3
3
from django.forms import widgets
4
4
5
5
from config import settings

Using from django.conf import settings simplifies the code and makes it more maintainable.

Read more
Suggested changes
-
from config import settings
+
from django.conf import settings
6
6
7
7
18
18
        return widgets.Media(
19
19
            js=(
20
20
                'admin/js/vendor/jquery/jquery{extra}.js',

An f-string will not work if the f prefix is missing.

Read more
Suggested changes
-
                'admin/js/vendor/jquery/jquery{extra}.js',
+
                f'admin/js/vendor/jquery/jquery{extra}.js',
Expand 2 lines ...
config/urls.py
31
31
    path("submit_job_raw/", views.View3.as_view(), name="submit-job"),

URL names must be unique otherwise reverse('url_name') and {% url 'url_name' %} will link to the "wrong" page half of the time.

Read more
apps/aarvark/api.py
16
16
        user = PlatformUser.objects.get(pk=1)
17
17
18
18
        if user.group.id != get_request_user_organisation_id(request):

When working with foreign keys, accessing the related field will result in a database read. That can be eliminated by using *_id, which is the foreign key value that Django has already cached on the object to make this scenario more efficient.

Read more
Suggested changes
-
        if user.group.id != get_request_user_organisation_id(request):
+
        if user.group_id != get_request_user_organisation_id(request):
Expand 3 lines ...
19
19
            raise PermissionDenied()
20
20
21
21
        return Response()
25
25
26
26
    def post(self, request, pk):
27
27
        if PlatformUser.objects.filter(group=1).count() > 0:

Comparing queryset.count() is less efficient than checking queryset.exists(), so use querySet.count() if you only want the count, and use queryset.exists() if you only want to find out if at least one result exists.

Read more
Suggested changes
-
        if PlatformUser.objects.filter(group=1).count() > 0:
+
        if PlatformUser.objects.filter(group=1).exists():
Expand 2 lines ...
28
28
            raise PermissionDenied()
29
29
        return Response()
30
30
34
34
35
35
    def post(self, request, pk):
36
36
        if PlatformUser.objects.filter(group=1):

This can load every row in the database table into memory because the queryset is evaluated. Checking if a queryset is truthy/falsey is much less efficient than checking queryset.exists().

Read more
Suggested changes
-
        if PlatformUser.objects.filter(group=1):
+
        if PlatformUser.objects.filter(group=1).exists():
Expand 2 lines ...
37
37
            raise PermissionDenied()
38
38
        return Response()
39
39
45
45
        user = PlatformUser.objects.all().order_by('?')[0]

Using order_by('?') can be very inefficient if you have lots of rows in the table. Moving the randomness to the application layer will probably give significant a performance improvement.

Read more
Suggested changes
-
        user = PlatformUser.objects.all().order_by('?')[0]
+
        user = PlatformUser.objects.all()[random.randint(0, PlatformUser.objects.count() - 1)]
apps/aarvark/migrations/0014_migrate_legacy_unlocks.py
23
23
    operations = [migrations.RunPython(migrate_unlocks)]

It's good to, as a minimum, specify noop in RunPython so the migration can be skipped when going backwards, and even better to specify a function that undoes the migration.

Read more
Suggested changes
-
    operations = [migrations.RunPython(migrate_unlocks)]
+
    operations = [migrations.RunPython(migrate_unlocks, migrations.RunPython.noop)]
Use our GitHub pull request bot to prevent issue like these getting into your codebase.

Filter

Issues