Repository navigation
ficus/develop update ficus/master #172
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
Merged
bryanlandia
merged 23 commits into
appsembler/ficus/master
from
appsembler/ficus/develop
Oct 20, 2017
Merged
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
0245db1
badge assertion add cert url as evidence for course groups
bryanlandia 1c1250f
Make sure multi-course course meta badge event groups still send awards
bryanlandia 2bacc1e
hotfix external course task name
melvinsoft cded392
Rework BadgeClass slugs and lookups
bryanlandia fb6c091
Merge badges changes allowing evidence url for single course course g…
bryanlandia 38eaf4f
new user update endpoint
melvinsoft 9b2b566
Merge pull request #167 from appsembler/maxi/maxi/new-user-update-end…
melvinsoft aab9f3e
Merge pull request #160 from appsembler/appsembler/ficus/feature/badg…
bryanlandia 8d82a86
Update advanced settings JS to multichoice settings fields
bryanlandia 2bb20b9
Specify MEDIA_ROOT, MEDIA_URL in cms env (used for Scorm)
bryanlandia 6294d66
set https off if passing http base scheme in env tokens
bryanlandia f6f97d8
Merge pull request #171 from appsembler/appsembler/ficus/hotfix/cms-m…
bryanlandia 87a9aaa
Merge pull request #169 from appsembler/appsembler/ficus/hotfix/fix-m…
bryanlandia fa82658
use AUTHENTICATION_BACKENDS from APPSEMBLER_FEATURES
tkeemon 70c9a5f
Merge pull request #170 from appsembler/appsembler/ficus/feature/auth…
tkeemon 9174386
Fix multichoice adv settings JS
bryanlandia a05f9fa
try to import user-org backend and create obj at registration time
tkeemon 6b25443
bypass auto login after registration with APPSEMBLER_FEATURE flag
tkeemon 425efc5
conditionally import our custom org middleware
tkeemon 0dbd8c5
fix formatting; include exception type in try/except
tkeemon e8fd8a0
remove unnecessary try/except
tkeemon d06202e
Merge pull request #174 from appsembler/appsembler/ficus/feature/nyif…
tkeemon 5356bf2
More pythonic HTTPS conditional from ENV_TOKENS in aws_appsembler
bryanlandia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 have a few issues with this line:
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:I think this makes the conditional more readable and show the expected value of
HTTPS = 'on'.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tkeemon - I defined
BASE_SCHEMEhere https://github.com/noderabbit-team/edx-configs/blob/tkeemon/nyif-ficus-staging/nyif-ficus/staging/files/server-vars.yml#L155The
edxapprole in configuration repo only definesEDXAPP_LMS_BASE_SCHEMEfor 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 toEDXAPP_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.
Uh oh!
There was an error while loading. Please reload this page.
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.
The
HTTPS=onit looks like something fromwsgi, 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 😄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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkeemon @OmarIthawi
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.