ficus/develop update ficus/master - #172
Conversation
when only a single course in course group
backend slugs no longer generated from component and submitted slug because Badgr.io creates its own uuid-based slugs now BadgeClasses unique by slug, and also by course mode w/ course id Criteria passed to Badgr as criteria_text or criteria_url depending look up Badge Classes by slug if passed or if not by mode + id
fix in case custom form doesn't exists docs for the new endpoint remove prints check email integrity
…point-new-user-update-endpoint new user update endpoint
…es-fix-badgr-integration Fixes for Badgr.io badges integration
…edia-root specify media root in CMS and allow http scheme in aws_appsembler by env token
…ultichoice-adv-settings Update advanced settings JS to multichoice settings fields
OmarIthawi
left a comment
OmarIthawi
left a comment
There was a problem hiding this comment.
Not sure what to do here, since the changes have already been reviewed, no?
|
@OmarIthawi Yes, everything has been reviewed. I'm just requesting a review as a final sanity check, but wouldn't expect anybody to need to go over it really closely. Since there are no conflicts we are probably good to go. |
tkeemon
left a comment
tkeemon
left a comment
There was a problem hiding this comment.
I'd just like a quick clarification before I approve.
| MEDIA_ROOT = '/edx/var/edxapp/media' | ||
| MEDIA_URL = '/media/' | ||
|
|
||
| HTTPS = ENV_TOKENS.get('BASE_SCHEME', 'https').lower() == 'http' and 'off' or 'on' |
There was a problem hiding this comment.
I have a few issues with this line:
- It took me a few tries to parse it and figure out exactly what is being assigned here. If
BASE_SCHEMEishttp, thenHTTPS = offbut if it's anything else, thenHTTPS = on. I know it's a bit ugly, but can it be rewritten in a more Pythonic way:
HTTPS = 'on' if ENV_TOKENS.get('BASE_SCHEME', 'https').lower() == 'https' else 'off'
I think this makes the conditional more readable and show the expected value of HTTPS = 'on'.
- I can't find
BASE_SCHEMEdefined anywhere. I've checked Configuration, edx-configs, and a few customer edxapp JSON files. Is this actually being used anywhere?
There was a problem hiding this comment.
On a related note, wouldn't this variable be better as a boolean instead of having a value of on/off?
There was a problem hiding this comment.
Thanks @tkeemon - I defined BASE_SCHEME here https://github.com/noderabbit-team/edx-configs/blob/tkeemon/nyif-ficus-staging/nyif-ficus/staging/files/server-vars.yml#L155
The edxapp role in configuration repo only defines EDXAPP_LMS_BASE_SCHEME for some reason—so, not defined for CMS. I added this in order to be able to specify http/https for CMS as well. Most of the time we will want https here, so I kept it as a kind of optional addition to EDXAPP_ENV_EXTRA.
Agree as far as Boolean vs string but I'm just following what's been done so far in edx-platform. As far as the syntax of that line, I guess that's how I've always done ternaries in Python. I didn't realize it was that uncommon.
If these things are important to fix they should be made with a new PR to the /develop branch. I was trying to get them into /master as they'd been approved.
There was a problem hiding this comment.
@tkeemon: On a related note, wouldn't this variable be better as a boolean instead of having a value of
on/off?
The HTTPS=on it looks like something from wsgi, so it should be fine to keep it as is! Besides it's an OS env variable (AFAIK) and there are no booleans in that land, on string citizens are allowed 😄
@bryanlandia: Agree as far as Boolean vs string but I'm just following what's been done so far in edx-platform. As far as the syntax of that line, I guess that's how I've always done ternaries in Python. I didn't realize it was that uncommon.
It's indeed not Pythonic at all. I should've noticed that. I really don't think it's a big deal. But you're welcome to PR it.
There was a problem hiding this comment.
I know it's a bit ugly, but can it be rewritten in a more Pythonic way
Thanks for pointing out the errors in my ways. I've done it the other way so long it's just habit now. I'd prefer to just keep it as is and not make a new PR, as we are trying to get a large batch of changes merged to /master and I don't want to further complicate this now.
…-backends-from-json use AUTHENTICATION_BACKENDS from APPSEMBLER_FEATURES
mistakenly removed during merge conflict fix
…-reg-workflow Skip login after registration (mostly for NYIF)
|
@tkeemon I updated the conditionals for setting HTTPS value in |
4 participants
Let's see if we can get appsembler/ficus/master updated with some of the latest changes. Looks like we've got...