Skip to content
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

Location to/from locator mapping facilities #661

Merged
merged 50 commits into from
Aug 19, 2013
Merged

Conversation

dmitchell
Copy link
Contributor

was pr 601 but I accidentally blew it away when I rebased --onto master w/ push -f afterward
@cpennington

pbaratta and others added 14 commits August 13, 2013 13:33
Change the `MathJax.Hub.Queue(initializeRequest)` to a simpler function call
(`initializeRequest.call(this)`). This was failing to give a proper context
to initializeRequest, and `this.value` was turning up as `undefined`.

Also add a fallback if we need to display some code before MathJax finishes
its original typesetting.

I was stubbing out `Queue` in my specs, so the tests had to be changed around
a little.
… on the base method that is in xml_module.py to preserve the correct export filesystem hierarchy.
…ch presumes writing to a filesystem, plus it adds a url_name to the attribute set. Also, on __init__ reset the 'category' attribute, on some code paths this can get lost. Not sure why, but this gets all the tests to pass.
Also fixes problems with double-quoted strings (historical artifact).

STUD-640

Conflicts:

	common/lib/xmodule/xmodule/video_module.py
The django_nose it is very useful, even outside the test
environment. For example, it lets you to easily run test from
manage.py without additional changes to the test packages.
Fix grade range names not being editable.
Video: disable speed controls for unsupported browsers.
a singleton accessor.
"""
# pylint: disable=W0603
global _loc_singleton
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than ignoring the pylint errors, why not just make this _LOC_SINGLETON (or, even better, LOCATION_MAPPER)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know those would suffice for pylint. I'm wary of the
non-underscored version as implying "public" accessibility.

On Fri, Aug 16, 2013 at 9:36 AM, Calen Pennington
notifications@github.comwrote:

In common/lib/xmodule/xmodule/modulestore/django.py:

@@ -46,3 +47,18 @@ def modulestore(name='default'):

# Initialize the modulestores immediately

for store_name in settings.MODULESTORE:

modulestore(store_name)

+_loc_singleton = None
+def loc_mapper():

  • """
  • Get the loc mapper which bidirectionally maps Locations to Locators. Used like modulestore() as
  • a singleton accessor.
  • """
  • pylint: disable=W0603

  • global _loc_singleton

Rather than ignoring the pylint errors, why not just make this
_LOC_SINGLETON (or, even better, LOCATION_MAPPER)


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/661/files#r5813741
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing case didn't get rid of the warning. The difference between this and _MODULESTORE above is that this does a direct assignment v changing something inside of it which triggers the need for the global decl, afaik.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok w/ disabling the complaint about global. I was more concerned about a complaint about casing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

W0603 http://pylint-messages.wikidot.com/messages:w0603: Using the global
statement (from http://pylint-messages.wikidot.com/all-messages)

On Fri, Aug 16, 2013 at 10:59 AM, Calen Pennington <notifications@github.com

wrote:

In common/lib/xmodule/xmodule/modulestore/django.py:

@@ -46,3 +47,18 @@ def modulestore(name='default'):

# Initialize the modulestores immediately

for store_name in settings.MODULESTORE:

modulestore(store_name)

+_loc_singleton = None
+def loc_mapper():

  • """
  • Get the loc mapper which bidirectionally maps Locations to Locators. Used like modulestore() as
  • a singleton accessor.
  • """
  • pylint: disable=W0603

  • global _loc_singleton

I'm ok w/ disabling the complaint about global. I was more concerned about
a complaint about casing.


Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/661/files#r5815544
.

Peter Fogg and others added 2 commits August 16, 2013 09:42
…yback.

Because with speed changing the time also changes for Flash playback,
a different way to calculate the current time is needed than for HTML5
playback. I have added conditions for Flash and HTML5 video, and put
old method of calculating time for Flash.

I have tested it on the YouTube video ZwkTiUPN0mg. Both HTML5 mode and
Flash mode have proper video-captions syncing with this fix. NOTE: to view
YouTube video in Flash mode you either have to use an old browser (ex. Firefox
version 18) or hard code in source that state.currentPlayerMode = 'flash' (in
function _setPlayerMode(), file 01_initialize.js).
self.create_map_entry(course_location)
entry = self.location_map.find_one(location_id)
else:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a reasonable place to raise an exception, rather than just returning None

@cpennington
Copy link
Contributor

At a larger scale, it seems like this mapping facility might be better suited to storage in a relational database. Did you consider doing it that way?

adampalay and others added 5 commits August 16, 2013 10:25
in grading, if problem cannot be created, return score as none
Letting xblocks handle scope rather than separating fields into
different attrs. Although, split still shunts content fields to a
different collection than setting and children fields.

The big difference is that content fields will always be a dict and not
sometimes just a string and there's no special casing of 'data' attr.

The other mind change is no more 'metadata' dict.
and fix some comments ref'g old names
Correctly seek in captions with non-1.0 speeds.
@dmitchell
Copy link
Contributor Author

I didn't consider sql b/c I'd assumed that someone made the architectural decision that all course content info belongs in mongo and all user id and progress belongs in sql (transactional data). afaik, there's no objective reason for preferring mongo over sql for any of our data since we don't use any of the doc processing capabilities of mongo (CLOBs could store our definitions just as well in a sql db.)

dmitchell and others added 2 commits August 16, 2013 12:18
xblock fields persist w/o breaking by scope
Add a 'MixedModuleStore' which aggregates between XML and Mongo Modulestores
@dmitchell
Copy link
Contributor Author

Addressed everything but the n^2 name generator (which I addressed w/ sophistry).

@dmitchell
Copy link
Contributor Author

rebased

@cpennington
Copy link
Contributor

👍

@cpennington cpennington reopened this Aug 19, 2013
@cpennington
Copy link
Contributor

The reason we put course content in mongo was so that we could search inside the content at some point in the future (which we couldn't do w/ CLOBs). That doesn't mean that all things course related have to go in mongo. In general, code should use a data store that matches its need. In this case, I'm not sure what it would take to put this in a sql store (for instance, would it introduce django dependencies into the modulestore codebase, or would we move this all out of modulestore into common/djangoapps, or would we add a separate sql mapper...)

In the end, I think let's leave it in mongo for now, and if we have trouble w/ performance, we can migrate it to sql later.

dmitchell added a commit that referenced this pull request Aug 19, 2013
Location to/from locator mapping facilities
@dmitchell dmitchell merged commit 33737d3 into master Aug 19, 2013
@jzoldak
Copy link
Contributor

jzoldak commented Aug 19, 2013

@dmitchell @cpennington this PR made master unstable with too many pep8/pylint violations.

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
@jzoldak jzoldak deleted the dhm/location_mapper branch May 5, 2014 14:55
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
bradenmacdonald referenced this pull request in open-craft/edx-platform Apr 11, 2016
johanseto pushed a commit to nelc/edx-platform that referenced this pull request Jan 22, 2024
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.

8 participants