Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Upgrade Django to 4.2.x #1538

Merged
merged 2 commits into from
Jul 26, 2024
Merged

chore: Upgrade Django to 4.2.x #1538

merged 2 commits into from
Jul 26, 2024

Conversation

arslanashraf7
Copy link
Contributor

@arslanashraf7 arslanashraf7 commented Apr 23, 2024

What are the relevant tickets?

#1467, https://github.com/mitodl/hq/issues/3747

What's this PR do?

  • Upgrades Django to 4.2. You can check the release notes for Django 4 on https://docs.djangoproject.com/en/5.0/releases/4.0/
  • In the process of upgrading, this PR also upgrades other major packages like postgres, django-hijack, oauth-toolkit, celery, celery-redbeat, django-robots, djangorestframework
  • In the process of upgrading several packages to their major version the PR also adds/removes some existing unused settings
  • It also fixes the existing migrations
  • It removes our dependency on pytz package because pytz has been deprecated and Its support would be removed in Django 5.x.
  • It also updates tests
  • It also updates all the dependencies form ol-django
    NOTE:
  • Django hijack would not work out of the box until it has some specific state being set in the session. So, I have created a new support middleware in order to make sure that Django hijack works properly.

How should this be manually tested?

  • Testing on existing instance
  • Setup your app on the master branch, run seed commands to pre-fill the data (configure_cms & seed_data)
  • Create some bootcamps, BootCamp runs, BootCamp applications, certificates, invitation letters etc
  • run different commands e.g. set_appication_state you can also use this command in testing for setting application state from initial to completion and inbetween

Where should the reviewer start?

Setup a good amount of data on the master branch before moving to this branch. You should have users, profiles, applications, application steps etc.

What GIF best describes this PR or how it makes you feel?

Copy link

gitguardian bot commented Apr 23, 2024

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
433857 Triggered Generic Password 80feb61 authentication/pipeline/user_test.py View secret
433857 Triggered Generic Password 80feb61 authentication/pipeline/user_test.py View secret
433857 Triggered Generic Password 80feb61 authentication/pipeline/user_test.py View secret
433857 Triggered Generic Password 80feb61 authentication/pipeline/user_test.py View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@arslanashraf7 arslanashraf7 marked this pull request as ready for review April 30, 2024 10:29
@arslanashraf7 arslanashraf7 force-pushed the arslan/django4.x branch 3 times, most recently from 1f61471 to ed3e83f Compare July 5, 2024 09:44
@arslanashraf7 arslanashraf7 changed the title feat: Upgrade Django to 4.x chore: Upgrade Django to 4.x Jul 5, 2024
@arslanashraf7 arslanashraf7 force-pushed the arslan/django4.x branch 2 times, most recently from f96dd04 to 7171305 Compare July 10, 2024 12:31
@asadali145 asadali145 self-assigned this Jul 10, 2024
@arslanashraf7 arslanashraf7 force-pushed the arslan/django4.x branch 2 times, most recently from 6f5bc29 to e314fc4 Compare July 24, 2024 07:02
@arslanashraf7 arslanashraf7 changed the title chore: Upgrade Django to 4.x chore: Upgrade Django to 4.2.x Jul 24, 2024
Copy link
Contributor

@asadali145 asadali145 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a couple of minor things.

I have noticed an exception that does not impact any functionality. It exists on the current master.
Simply go to the CMS Dashboard, you will see an exception in local logs.

Will look like:

Traceback (most recent call last):
web-1     |   File "/opt/venv/lib/python3.10/site-packages/django/template/base.py", line 886, in _resolve_lookup
web-1     |     if isinstance(current, BaseContext) and getattr(
web-1     | AttributeError: type object 'Context' has no attribute 'header_controls'
web-1     | 
web-1     | During handling of the above exception, another exception occurred:
web-1     | 
web-1     | Traceback (most recent call last):
web-1     |   File "/opt/venv/lib/python3.10/site-packages/django/template/base.py", line 896, in _resolve_lookup
web-1     |     current = current[int(bit)]
web-1     | ValueError: invalid literal for int() with base 10: 'header_controls'
web-1     | 
web-1     | During handling of the above exception, another exception occurred:
web-1     | 
web-1     | Traceback (most recent call last):
web-1     |   File "/opt/venv/lib/python3.10/site-packages/django/template/base.py", line 903, in _resolve_lookup
web-1     |     raise VariableDoesNotExist(

Comment on lines 5 to 6
from django.urls import re_path
from django.urls import path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from django.urls import re_path
from django.urls import path
from django.urls import path, re_path


This middleware is written to ensure that the request session has accessed=True when the request reaches HijackMiddleware.

We will probably need to add a more robust solution to django-hijack package."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We will probably need to add a more robust solution to django-hijack package."""
We will probably need to add a more robust solution to django-hijack package.
"""

Comment on lines 9 to 14
HijackMiddleware needs the session to be in accessed state in order to work properly.
However, It doesn't work properly with our current architecture and django-hijack implementation
because when the request reaches HijackMiddleware the session is not in accessed state.

This middleware is written to ensure that the request session has accessed=True when the request reaches HijackMiddleware.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we mention that the issue was with the notification injection? I believe that was the only issue?

@@ -75,9 +74,9 @@
name="wagtailimages_serve",
),
re_path(r"^cms/login", cms_login_redirect_view, name="wagtailadmin_login"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be a simple path like below paths.

@arslanashraf7 arslanashraf7 merged commit ffc8179 into master Jul 26, 2024
5 checks passed
@arslanashraf7 arslanashraf7 deleted the arslan/django4.x branch July 26, 2024 10:38
@odlbot odlbot mentioned this pull request Jul 26, 2024
2 tasks
@arslanashraf7 arslanashraf7 mentioned this pull request Aug 12, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants