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

Add Django 4.2 support #334

Merged
merged 11 commits into from
Jul 10, 2023
Merged

Add Django 4.2 support #334

merged 11 commits into from
Jul 10, 2023

Conversation

salman2013
Copy link
Contributor

Description:

  • Add support for Django 4.2

Codemod used to upgrade this repo: https://github.com/adamchainz/django-upgrade

Ticket: edx/upgrades#183

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U labels Jun 26, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @salman2013! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by editing the PR title. If the problem persists, you can tag the @openedx/cla-problems team in a comment on your PR for further assistance.

1 similar comment
@openedx-webhooks
Copy link

Thanks for the pull request, @salman2013! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by editing the PR title. If the problem persists, you can tag the @openedx/cla-problems team in a comment on your PR for further assistance.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

@salman2013, I just merged dependency updates. Please rebase on the current master branch.

@@ -28,7 +28,7 @@ jobs:
matrix:
os: [ubuntu-20.04]
python-version: [3.8]
toxenv: [py38, integration, quality, translations]
toxenv: [py38, django32, django42, integration, quality, translations]
Copy link
Member

Choose a reason for hiding this comment

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

py38 is redundant. It would make sense to use both Django 3.2 and 4.2 for integration, quality, and translations, though.

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jun 27, 2023
@itsjeyd
Copy link
Contributor

itsjeyd commented Jun 27, 2023

Hi @salman2013, thank you for this contribution!

It looks like you haven't submitted a signed Contributor Agreement yet. To do that you can follow the instructions from the PR bot's latest post above.

If you have any questions, let us know.

@salman2013 salman2013 closed this Jul 3, 2023
@salman2013 salman2013 reopened this Jul 3, 2023
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@UsamaSadiq UsamaSadiq removed open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 3, 2023
@@ -28,7 +28,7 @@ jobs:
matrix:
os: [ubuntu-20.04]
python-version: [3.8]
toxenv: [py38, integration, quality, translations]
toxenv: [django32, django42, integration-django32, integration-django42, quality-django32, quality-django42, translations-django32, translations-django42]
Copy link
Member

Choose a reason for hiding this comment

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

All these environments are running unit tests. See the CI logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into it.

@itsjeyd itsjeyd added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 5, 2023
@salman2013
Copy link
Contributor Author

@itsjeyd My PR is not an open source contribution, I have verified CLA from Axim.

@itsjeyd
Copy link
Contributor

itsjeyd commented Jul 7, 2023

@salman2013 All pull requests issued against the openedx GitHub organization that are not created by Axim or 2U employees are considered open source contributions :)

Signing the CLA has nothing to do with that. It's a prerequisite for everyone who wants to get changes reviewed and merged into the Open edX code base.

I hope that helps, and if you have any additional questions just let me know.

@UsamaSadiq
Copy link
Member

Hi @itsjeyd I believe there is some confusion here. @salman2013 is a new member in the arbi-bom team under 2U organisation.

UsamaSadiq and others added 2 commits July 7, 2023 18:24
# Conflicts:
#	requirements/quality.txt
#	requirements/test.txt
#	requirements/workbench.txt
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: checked that the CI is passing
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@Agrendalath Agrendalath changed the title Django upgrade 4.2 test: add Django 4.2 Jul 10, 2023
@Agrendalath Agrendalath merged commit 2125ae3 into master Jul 10, 2023
8 checks passed
@Agrendalath Agrendalath deleted the salman/django-4.2-upgrade branch July 10, 2023 11:22
@itsjeyd itsjeyd removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. open-source-contribution PR author is not from Axim or 2U labels Jul 12, 2023
@itsjeyd
Copy link
Contributor

itsjeyd commented Jul 12, 2023

@UsamaSadiq

@salman2013 is a new member in the arbi-bom team under 2U organisation.

OK I see, thanks for clarifying! I removed the open-source-contribution label.

@salman2013 salman2013 changed the title test: add Django 4.2 Add Django 4.2 support Jul 12, 2023
@salman2013
Copy link
Contributor Author

@Agrendalath Could you please bump the version and release this update so that we can close the ticket? We can also do it, Please confirm, thanks

@Agrendalath
Copy link
Member

@salman2013, why should we release it? This only changes the tests - we didn't change any code or base requirements here, so the distributed Python package will be exactly the same as the previous one.

@UsamaSadiq
Copy link
Member

Hi @Agrendalath
To ensure the integration tests are running with respective Django versions (Django 3.2 & Django 4.2) individually, we had to remove the Django dependency from the workbench.txt file.
Releasing a new package version will enable us to test if this change affects anything anywhere down the line or not. I'd like to make sure we can keep this change without affecting any other package or the usage of this package. If it breaks anything, then we'll need to revert this change and add Django 4.2 back in the workbench dependencies.

@Agrendalath
Copy link
Member

Agrendalath commented Jul 17, 2023

@UsamaSadiq, I'd like to understand this use case a bit more to be sure how the changes in the specific XBlock repositories affect things down the line.

The workbench.txt dependencies are used in the following places:

  1. Integration tests of this XBlock.. They are running in the CI pipelines of this repository.
  2. Development dependencies.. This way, you can run the integration tests locally, too.

As you can see in the setup.py, the only requirements used while installing this Python package are in the base.in (with its constraints defined in constraints.txt).

Furthermore, you can check which files are included in the package (pushed to the PyPI) by checking these lines in the setup.py + the MANIFEST.in file. You can verify this by running python setup.py sdist bdist_wheel in this repo's root directory. Then, go to the dist directory and unpack the xblock-drag-and-drop-v2-{VERSION_NUMBER}.tar.gz file. When you list the content of this directory, you will get the following:

.
├── Changelog.md
├── drag_and_drop_v2
│  ├── __init__.py
│  ├── compat.py
│  ├── default_data.py
│  ├── drag_and_drop_v2.py
│  ├── public
│  │  ├── css
│  │  │  ├── drag_and_drop.css
│  │  │  └── drag_and_drop_edit.css
│  │  ├── img
│  │  │  └── triangle.png
│  │  ├── js
│  │  │  ├── drag_and_drop.js
│  │  │  ├── drag_and_drop_edit.js
│  │  │  ├── translations
│  │  │  │  ├── ar
│  │  │  │  │  └── text.js
│  │  │  │  ├── de
│  │  │  │  │  └── text.js
│  │  │  │  ├── en
│  │  │  │  │  └── text.js
│  │  │  │  ├── eo
│  │  │  │  │  └── text.js
│  │  │  │  ├── es_419
│  │  │  │  │  └── text.js
│  │  │  │  ├── fr
│  │  │  │  │  └── text.js
│  │  │  │  ├── he
│  │  │  │  │  └── text.js
│  │  │  │  ├── hi
│  │  │  │  │  └── text.js
│  │  │  │  ├── it
│  │  │  │  │  └── text.js
│  │  │  │  ├── ja
│  │  │  │  │  └── text.js
│  │  │  │  ├── ko
│  │  │  │  │  └── text.js
│  │  │  │  ├── nl
│  │  │  │  │  └── text.js
│  │  │  │  ├── pl
│  │  │  │  │  └── text.js
│  │  │  │  ├── pt_BR
│  │  │  │  │  └── text.js
│  │  │  │  ├── pt_PT
│  │  │  │  │  └── text.js
│  │  │  │  ├── ru
│  │  │  │  │  └── text.js
│  │  │  │  ├── tr
│  │  │  │  │  └── text.js
│  │  │  │  └── zh_CN
│  │  │  │     └── text.js
│  │  │  └── vendor
│  │  │     ├── handlebars-v1.1.2.js
│  │  │     └── virtual-dom-1.3.0.min.js
│  │  └── themes
│  │     └── apros.css
│  ├── templates
│  │  └── html
│  │     ├── drag_and_drop.html
│  │     ├── drag_and_drop_edit.html
│  │     └── js_templates.html
│  ├── translations
│  │  ├── __init__.py
│  │  ├── ar
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── config.yaml
│  │  ├── de
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── en
│  │  │  └── LC_MESSAGES
│  │  │     ├── django.po
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── eo
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── es_419
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── fr
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── fr_CA
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── he
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── hi
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── it
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── ja
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── ko
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── nl
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── pl
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── pt_BR
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── pt_PT
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── rtl
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── ru
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  ├── settings.py
│  │  ├── tr
│  │  │  └── LC_MESSAGES
│  │  │     ├── text.mo
│  │  │     └── text.po
│  │  └── zh_CN
│  │     └── LC_MESSAGES
│  │        ├── text.mo
│  │        └── text.po
│  └── utils.py
├── LICENSE
├── MANIFEST.in
├── PKG-INFO
├── README.md
├── requirements
│  ├── base.in
│  └── constraints.txt
├── setup.cfg
├── setup.py
└── xblock_drag_and_drop_v2.egg-info
   ├── dependency_links.txt
   ├── entry_points.txt
   ├── PKG-INFO
   ├── requires.txt
   ├── SOURCES.txt
   └── top_level.txt

None of the files updated in this PR are included in the package pushed to PyPI. I'm not against releasing a new version, but I'm curious how releasing the package affects your testing process.

If it breaks anything, then we'll need to revert this change and add Django 4.2 back in the workbench dependencies.

As we do not use anything from this PR outside of tests, and the CI is passing, these changes cannot break anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants