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.

Sign up for GitHub

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

Jump to bottom

build: add django42 in CI #4101

Merged
merged 2 commits into from
Sep 21, 2023
Merged

build: add django42 in CI #4101

merged 2 commits into from
Sep 21, 2023

Conversation

UsamaSadiq
Copy link
Member

@UsamaSadiq UsamaSadiq commented Sep 20, 2023 •
edited
Loading

Description

  • Before upgrading to Django 4.2, creating an intermediate PR to enable CI tests to run with django 4.2.
  • It'll help us in identifying potential blockers from upgrading to Django 4.2.
  • Some dependencies didn’t have support for both Django32 and Django42 so we had to create separate test requirements files for Django 4.2 support

UsamaSadiq
UsamaSadiq commented Sep 21, 2023
View reviewed changes
course_discovery/apps/course_metadata/models.py
@@ -1256,8 +1256,8 @@ class AbstractLocationRestrictionModel(TimeStampedModel):
(BLOCKLIST, _('Blocklist'))
)

countries = MultiSelectField(choices=COUNTRIES, null=True, blank=True)
states = MultiSelectField(choices=CONTIGUOUS_STATES, null=True, blank=True)
countries = MultiSelectField(choices=COUNTRIES, null=True, blank=True, max_length=len(COUNTRIES))
Copy link
Member Author

Choose a reason for hiding this comment

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

multiselectfield does not have Django>4 support and raises IndexError. An issue has been created against upstream repo and fix PR has also been created.
Since there is no response from the author so attempting the fix suggested in the issue discussion locally to fix the test failures.

UsamaSadiq
UsamaSadiq commented Sep 21, 2023
View reviewed changes
course_discovery/apps/course_metadata/migrations/0334_auto_20230921_1101.py
@@ -0,0 +1,34 @@
# Generated by Django 3.2.20 on 2023-09-21 11:01
Copy link
Member Author

Choose a reason for hiding this comment

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

New migration created because model was updated to handle multiselectfield issue.

UsamaSadiq
UsamaSadiq commented Sep 21, 2023
View reviewed changes
Makefile
@@ -41,6 +41,11 @@ requirements.python: ## Install Python requirements for local development.

requirements: requirements.js requirements.python ## Install Python and JS requirements for local development

requirements.python_dj42: ## Install Python requirements for Django 4.2 env
Copy link
Member Author

Choose a reason for hiding this comment

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

Added temporary targets to provide support for Django 4.2. These can be removed once we upgrade to Django 4.2

@UsamaSadiq
build: add django42 in CI
f56d098 
build: add separate requirements for django 4.2 env

fix: add max_length parameter to fix IndexError in multiselectfield
@UsamaSadiq
Merge branch 'master' into add-django42-ci
c9f14d7
@Ali-D-Akbar Ali-D-Akbar merged commit 5eb5d14 into openedx:master Sep 21, 2023
25 checks passed
This was referenced Sep 21, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ali-D-Akbar Ali-D-Akbar Ali-D-Akbar approved these changes

@awais786 awais786 awais786 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@UsamaSadiq @awais786 @Ali-D-Akbar