Skip to content

specify media root in CMS and allow http scheme in aws_appsembler by env token - #171

Merged
bryanlandia merged 2 commits into
appsembler/ficus/developfrom
appsembler/ficus/hotfix/cms-media-root
Oct 6, 2017
Merged

specify media root in CMS and allow http scheme in aws_appsembler by env token#171
bryanlandia merged 2 commits into
appsembler/ficus/developfrom
appsembler/ficus/hotfix/cms-media-root

Conversation

@bryanlandia

@bryanlandia bryanlandia commented Oct 2, 2017

Copy link
Copy Markdown

SCORM XBlock using filestorage requires specifying MEDIA_ROOT in cms as well as lms.
Also, allow specifying HTTPS setting to off via BASE_SCHEME env token from server-vars, for situations where we are using aws_appsembler env on a staging server w/o SSL certs set up. Normally only devstack envs has HTTPS settings off

@bryanlandia

bryanlandia commented Oct 2, 2017

Copy link
Copy Markdown
Author

The last commit 500e7f4 is to fix a subsequent merge conflict resolution that chose the wrong commits. It's not related to the other commits, sorry, but trying to get this resolved quickly.

udpate: this last commit was refactored to another PR.

@OmarIthawi OmarIthawi Oct 3, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bryanlandia Could you please let me know the why you're refactoring (' -> " and the spacing)?

It's a bit difficult to separate the signal from the noise in this file.

@OmarIthawi

Copy link
Copy Markdown

Why are do we have a build if it fails all the time? I'd just remove it.

@OmarIthawi OmarIthawi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's not clear what have changed in advanced.js. Please consider refactoring in a separate PR. Or at least a separate final commit.

@bryanlandia
bryanlandia force-pushed the appsembler/ficus/hotfix/cms-media-root branch from 500e7f4 to 6294d66 Compare October 4, 2017 19:48
@bryanlandia

Copy link
Copy Markdown
Author

@OmarIthawi Thanks. As you suggested, I removed the last commit to refactor as another PR.

@bryanlandia

bryanlandia commented Oct 4, 2017

Copy link
Copy Markdown
Author

@OmarIthawi - The other PR is #169

@OmarIthawi

Copy link
Copy Markdown

Thanks @bryanlandia! Looks much better. One note (it might be unnecessary though). For the following conditions we might have a problem with this PR:

  • Client X needs Scorm and,
  • Requires a multi-edxapp servers setup

In this case the MEDIA_ROOT can't be used. Otherwise a better backend should be used. Therefore w should be able to configure DEFAULT_FILE_STORAGE to something like storages.backends.s3boto3.S3Boto3Storage.

If the latter is not a problem please feel free to merge. Otherwise I think it is worth fixing.

@bryanlandia

bryanlandia commented Oct 5, 2017 via email

Copy link
Copy Markdown
Author

@OmarIthawi

Copy link
Copy Markdown

Thanks Omar. Right now, because of cross domain security issues in the
browser, SCORM assets can't be stored in S3. We would need to use
cloudfront in that case. I haven't worked that out yet.

Not sure that I understand why, please let me know if you'd like me to jump in. I should able to help in such issues.

It looks good to go! Please feel free to merge it.

@bryanlandia
bryanlandia merged commit f6f97d8 into appsembler/ficus/develop Oct 6, 2017
@bryanlandia
bryanlandia deleted the appsembler/ficus/hotfix/cms-media-root branch October 6, 2017 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants