-
Notifications
You must be signed in to change notification settings - Fork 60
[FAL-2198] Django 3.2 support #333
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
Conversation
This reverts commit 7ad0335.
edd359d to
e8d4534
Compare
eed95a0 to
400d465
Compare
400d465 to
91ca0ff
Compare
8470f64 to
f87eaa7
Compare
6775b9e to
f422dfc
Compare
f422dfc to
59f5a18
Compare
xitij2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! just one minor issue I see.
I'm testing this right now.
| - ar # Arabic | ||
| - es_419 # Spanish | ||
| - pt_BR # Portuguese | ||
| - zh_CN # Chinese | ||
| - es-419 # Spanish | ||
| - pt-br # Portuguese | ||
| - zh-cn # Chinese | ||
| - fr # French | ||
| - fr_CA # French (Canada) | ||
| - de_DE # German | ||
| - ja_JP # Japanese | ||
| - pl_PL # Polish | ||
| - ko_KR #Korean | ||
| - fr-ca # French (Canada) | ||
| - de-de # German | ||
| - ja-jp # Japanese | ||
| - pl-pl # Polish | ||
| - ko-kr #Korean | ||
| # Add languages here as needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original names were what are used for edx-platform so we should stay consistent with that: https://github.com/edx/edx-platform/blob/0935b5c51fa4115caa6d6d98b0ef65fea4fcdec1/conf/locale/config.yaml#L9-L91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xitij2000 Django3.2 refused to start with the original names. It would throw this error.
I looked at the LANGUAGES setting in edx-platform and they do use the django3.2 compatible names, so I thought it was ok to change this, but I don't really know how translations work with this XBlock.
I didn't test whether translations are working - I don't know if that's possible to do inside xblock-sdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible with xblock-sdk, but I've done it a lot, so I'll have a quick look with one of the affected languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks a lot for checking :)
It's not used anywhere else and doesn't have to be configurable.
e46eb0c to
b755331
Compare
xitij2000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Currently, PB_LANGUAGE_JS_DIRECTORY_MAP is imported from settings and it imports workbench.setting which isn't available on edx-platform. So we should move PB_LANGUAGE_JS_DIRECTORY_MAP to a place where it can be imported from both places, without importing settings.
Other than that, there is one more issue I see:
problem-builder-styling-issue.mp4
The styling is a bit off. However, since there have been a lot of changes in the platform recently, we can leave this to a separate task. It might also just be an issue with the master release and not the current release.
- I tested this: (describe what you tested)
- I read through the code
- [na] I checked for accessibility issues (no changes)
- Includes documentation
- [na] I made sure any change in configuration variables is reflected in the corresponding client's
configuration-securerepository.
|
@mtyaka when I am running Am i missing something? My submodule version that is being shown is |
@saksham115 Yeah, that's a bit of a mess currently. While |
Yeah, that's what I did later (manually installing Django3.2). Cool, glad to know that was the correct approach. |
saksham115
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
Did the steps mentioned in the testing instructions, with force-installing Django3.2 after the sdk installation
- I tested this, as per the testing methods and the steps defined in README
- I read through the code
- Includes documentation
One small suggestion (Don't know if it fits here or not):
- Maybe include the,
git submodule update --initstep in the readme to fetch thexblock-sdk(Not a blocker for merge)


This updates the tests so that they run in an environment with Django 3.2.
It also fixes a few deprecation warnings.
Note: this also incorporates updates from #332.
Test instructions: