search

openedx / xblock-image-explorer

GNU Affero General Public License v3.0
9 stars 39 forks source link

[SE-3173] Updates XBlock to Support Python 3 for Juniper Release #85

Closed nizarmah closed 4 years ago

nizarmah commented 4 years ago

Updates XBlock to support Python 3 so that it is compatible with edX's Juniper release.

Changes include PR #84 :+1:

JIRA tickets: SE-3173

Sandbox URL:

Testing instructions:

  1. Login to the LMS with staff
  2. Open the Studio and make sure that you can add an Image XBlock to a Course
  3. Make sure that the XBlock renders properly in Studio
  4. Press View Live and make sure that you can interact with the XBlock on the LMS
  5. Press Preview and make sure that you can preview the XBlock properly

Reviewers

toxinu commented 4 years ago

@nizarmah Looks good to merge 👍🏻

cc @shafqatfarhan @musmanmalik @msaqib52 Would you have to review this pull request please?

shafqatfarhan commented 4 years ago

@nizarmah I already have a similar PR (Python 3 upgrade work for this repo) in my queue that is under review. It seems like both the PRs are targeting Python 3 upgrade work. I will get back to you once I am done reviewing the other PR

cc: @ihtram

nizarmah commented 4 years ago

Hello @shafqatfarhan ! First of all, thank you for your time.

I noticed the other PR, but it has a Merge Conflict. And I wasn't a fan of how some of the changes were made.

It seems like the other PR keeps support for Python 2.7, while supporting Python 3.

This PR removes support for Python 2.7, similarly to what edX has done in many of their repositories lately.

In case you choose to merge this PR, all tox tests work, there are no merge conflicts, and the explorer works.

shafqatfarhan commented 4 years ago

@nizarmah Are you planning to work on other repos of edx-solutions organization on PY3 upgrade work? Just curious, because my team has also planned to work on the same upgrade work for edx-solutions organization as part of Juniper rebase for edx-solutions/edx-platform.

nizarmah commented 4 years ago

Hello @shafqatfarhan. I'm not really sure exactly if we do have other XBlocks we want to upgrade to support Juniper. However, of what I am aware of, we worked on most upgrades, and Image Explorer turned out to be an exception that we forgot.

Would you please be able to share with me a list of the XBlocks you plan to upgrade so that I can inform my peers at OpenCraft about them?

shafqatfarhan commented 4 years ago

Hello @shafqatfarhan. I'm not really sure exactly if we do have other XBlocks we want to upgrade to support Juniper. However, of what I am aware of, we worked on most upgrades, and Image Explorer turned out to be an exception that we forgot.

Would you please be able to share with me a list of the XBlocks you plan to upgrade so that I can inform my peers at OpenCraft about them?

Ideally, we have to upgrade all the XBlocks / extension apps to PY3 that are in edx-solutions organization. If you plan to work on any other XBlock / extension app under this organization, let us know so that there won't be duplication of work.

Thanks

nizarmah commented 4 years ago

If you plan to work on any other XBlock / extension app under this organization, let us know so that there won't be duplication of work.

Will do :slightly_smiling_face: Thanks a lot for your review @shafqatfarhan! :heart: Is someone else's review needed before this can be merged?

nizarmah commented 4 years ago

@shafqatfarhan can you please merge this when you have time? I don't have the necessary permissions

ihtram commented 4 years ago

@nizarmah merging this PR. For now we will using juniper-rebase branch for this Xblock. Once we will be ready with juniper upgrade, we will merge this change into master and will then create a release tag.