New locators for content libraries #46

bradenmacdonald · 2014-10-24T08:37:04Z

This PR implements two new types of locators necessary to facilitate content libraries. I'll be posting a separate PR to edx-platform for the content libraries prototype soon, but for now I'd like to start the review process for this locator code.

The new locators are described in detail under the "New Locators" heading at https://openedx.atlassian.net/wiki/display/SOL/Content+Libraries

openedx-webhooks · 2014-10-24T08:37:05Z

Thanks for the pull request, @bradenmacdonald! I've created OSPR-167 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add information to the ticket which can help Product understand the context for the PR - supporting documentation, edx-code email threads, timeline information ('this must be merged by XX date', and why that is), partner information (this is for a course on edx.org, for example), etc.

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

We can't start reviewing your pull request until you've added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information.

singingwolfboy · 2014-10-24T15:42:03Z

Thanks! Can you fix up the pylint violations, so that this branch passes the Travis build?

sarina · 2014-10-24T17:21:27Z

Hey @bradenmacdonald : this makes sense to me. I'm wondering if there's any supporting documentation you might have to add to this (ie did you discuss this on edx-code?) - also be sure to add yourself to AUTHORS for this new repo.

Also check your diff-cover report and see if you can up your coverage:
opaque_keys/edx/locator.py (78.5%): Missing lines 449,454,459-460,464,473,490-495,558,569,579,588,596,608,1093,1097,1109,1126,1138,1144,1150,1172,1177,1181,1186

coveralls · 2014-10-24T21:18:23Z

Coverage decreased (-0.44%) when pulling 602ffb3 on open-craft:library-locators into a70af7c on edx:master.

coveralls · 2014-10-27T01:24:10Z

Coverage increased (+1.4%) when pulling e1bb028 on open-craft:library-locators into a70af7c on edx:master.

coveralls · 2014-10-27T01:27:23Z

Coverage increased (+1.4%) when pulling 0c49a27 on open-craft:library-locators into a70af7c on edx:master.

bradenmacdonald · 2014-10-27T01:39:12Z

Ok, I have fixed the quality issues and increased the coverage to 100%.

@sarina The technical design is at https://openedx.atlassian.net/wiki/display/SOL/Content+Libraries . It was posted to the mailing list but virtually all the discussion happened on that wiki page. I then presented it to the Arch Council and this got the green light after a fruitful discussion.

This is for an edX solutions client and we are hoping to get it approved and ASAP so that we can use it in the content libraries prototype coming soon to edx-platform.

e-kolpakov · 2014-10-27T02:40:11Z

opaque_keys/edx/locator.py

+        block_id = self._parse_block_ref(block_id, False)
+
+        if not all(self.ALLOWED_ID_RE.match(val) for val in (block_type, block_id)):
+            raise InvalidKeyError(self.__class__, "Invalid block_type or block_id")


Would be great if actual block_type and block_id are somehow incorporated into exception, or at least logged

e-kolpakov · 2014-10-27T03:00:43Z

@bradenmacdonald most of the test can be converted to data-driven tests (I've marked some of them explicitly). The benefit of using ddt is that the test code is much DRY-er and allows for adding new test cases much simpler. Other than that - looks good.

coveralls · 2014-10-27T04:46:09Z

Coverage increased (+1.33%) when pulling 6d7533d on open-craft:library-locators into a70af7c on edx:master.

coveralls · 2014-10-27T04:52:30Z

Coverage increased (+1.34%) when pulling 8bfec4d on open-craft:library-locators into a70af7c on edx:master.

sarina · 2014-10-27T14:18:03Z

@bradenmacdonald tests & code looks clean, moving this forward to team review.

dmitchell · 2014-10-27T15:45:10Z

opaque_keys/edx/locator.py

-        ({BRANCH_PREFIX}@(?P<branch>{ALLOWED_ID_CHARS}+)\+?)?
-        ({VERSION_PREFIX}@(?P<version_guid>[A-F0-9]+)\+?)?
-        ({BLOCK_TYPE_PREFIX}@(?P<block_type>{ALLOWED_ID_CHARS}+)\+?)?
+        ((?P<org>{ALLOWED_ID_CHARS}+)\+(?P<course>{ALLOWED_ID_CHARS}+)(\+(?P<run>{ALLOWED_ID_CHARS}+))?(\+(?=.))?)??


I don't understand why you added ?=. which seems to be a noop: "there may be a plus sign here or anything else. If plus sign, eat it, if it's anything else, don't read it in" seems equivalent to "there may be a plus sign here. If so, eat it."

I noticed this pattern would match URLs with a trailing plus, e.g. course-v1:BradenX+PHYS200+2014+ which seems weird - with this change to add a lookahead, a lone trailing + is no longer matched. It's not important for the libraries, just something I saw while re-working to make run optional.

ah, I misread the pattern. Perhaps add a comment string as w/ all the ? it gets hard to humanly read? Anyway, good catch.

I feel like we're getting past the point where a regex for parsing makes this easier to read. I almost implemented imperative to parse the same, but held off. Still, seems like it might be worth considering a different parsing solution (combinators, maybe?)

dmitchell · 2014-10-28T17:42:16Z

opaque_keys/edx/tests/test_library_locators.py

+        self.assertEqual(version_only_lib_key.package_id, None)
+        with self.assertRaises(InvalidKeyError):
+            version_only_lib_key.for_branch("test")
+


at this point my eyes glaze over and I assume you've continued this systematic thoroughness through the rest :-)

dmitchell · 2014-10-28T17:43:22Z

👍

coveralls · 2014-10-29T02:31:50Z

Coverage increased (+1.33%) when pulling 644f1bc on open-craft:library-locators into a70af7c on edx:master.

bradenmacdonald · 2014-10-29T02:35:51Z

Thanks for the review! I've pushed two new commits with the suggested changes. Highlights are:

Made URL_RE_SOURCE regex easier to read, fixed another bug, updated tests
Took the branch='library' default out of LibraryLocator and moved it into split_draft (tested this on the prototype and it's working fine)
Cleaned up LibraryLocator docstring

New locators for content libraries

openedx-webhooks added the needs triage label Oct 24, 2014

openedx-webhooks added community manager review and removed needs triage labels Oct 24, 2014

bradenmacdonald force-pushed the library-locators branch from 587eec2 to 602ffb3 Compare October 24, 2014 21:15

bradenmacdonald force-pushed the library-locators branch 2 times, most recently from 621cef7 to e1bb028 Compare October 27, 2014 01:22

bradenmacdonald added 2 commits October 26, 2014 18:25

Library-related locators and unit tests

e97427d

Add myself to AUTHORS

0c49a27

bradenmacdonald force-pushed the library-locators branch from e1bb028 to 0c49a27 Compare October 27, 2014 01:25

e-kolpakov reviewed Oct 27, 2014
View reviewed changes

Make more of the tests data-driven, address other comments on test code

8bfec4d

bradenmacdonald force-pushed the library-locators branch from 6d7533d to 8bfec4d Compare October 27, 2014 04:49

openedx-webhooks added awaiting prioritization and removed community manager review labels Oct 27, 2014

openedx-webhooks added engineering review and removed awaiting prioritization labels Oct 27, 2014

dmitchell reviewed Oct 27, 2014
View reviewed changes

bradenmacdonald added 2 commits October 28, 2014 09:08

Cleanups based on PR comments

11b814d

Remove DEFAULT_BRANCH from the key - belongs elsewhere

644f1bc

dmitchell reviewed Oct 28, 2014
View reviewed changes

bradenmacdonald mentioned this pull request Oct 29, 2014

Content libraries prototype openedx/edx-platform#5779

Closed

openedx-webhooks added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 30, 2014

dmitchell added a commit that referenced this pull request Oct 30, 2014

Merge pull request #46 from open-craft/library-locators

b124013

New locators for content libraries

dmitchell merged commit b124013 into openedx:master Oct 30, 2014

bradenmacdonald deleted the library-locators branch October 30, 2014 18:29

bradenmacdonald mentioned this pull request Nov 21, 2014

Modulestore Support for Content libraries (Part of SOL-1, SOL-2, SOL-3) openedx/edx-platform#6033

Merged

bradenmacdonald mentioned this pull request Jan 6, 2015

Content libraries MVP openedx/edx-platform#6459

Merged

10 tasks

sarina added the open-source-contribution PR author is not from Axim or 2U label Apr 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New locators for content libraries #46

New locators for content libraries #46

bradenmacdonald commented Oct 24, 2014

openedx-webhooks commented Oct 24, 2014

singingwolfboy commented Oct 24, 2014

sarina commented Oct 24, 2014

coveralls commented Oct 24, 2014

coveralls commented Oct 27, 2014

coveralls commented Oct 27, 2014

bradenmacdonald commented Oct 27, 2014

e-kolpakov Oct 27, 2014

e-kolpakov commented Oct 27, 2014

coveralls commented Oct 27, 2014

coveralls commented Oct 27, 2014

sarina commented Oct 27, 2014

dmitchell Oct 27, 2014

bradenmacdonald Oct 27, 2014

dmitchell Oct 27, 2014

cpennington Oct 27, 2014

dmitchell Oct 28, 2014

dmitchell commented Oct 28, 2014

coveralls commented Oct 29, 2014

bradenmacdonald commented Oct 29, 2014

New locators for content libraries #46

New locators for content libraries #46

Conversation

bradenmacdonald commented Oct 24, 2014

openedx-webhooks commented Oct 24, 2014

singingwolfboy commented Oct 24, 2014

sarina commented Oct 24, 2014

coveralls commented Oct 24, 2014

coveralls commented Oct 27, 2014

coveralls commented Oct 27, 2014

bradenmacdonald commented Oct 27, 2014

e-kolpakov Oct 27, 2014

Choose a reason for hiding this comment

e-kolpakov commented Oct 27, 2014

coveralls commented Oct 27, 2014

coveralls commented Oct 27, 2014

sarina commented Oct 27, 2014

dmitchell Oct 27, 2014

Choose a reason for hiding this comment

bradenmacdonald Oct 27, 2014

Choose a reason for hiding this comment

dmitchell Oct 27, 2014

Choose a reason for hiding this comment

cpennington Oct 27, 2014

Choose a reason for hiding this comment

dmitchell Oct 28, 2014

Choose a reason for hiding this comment

dmitchell commented Oct 28, 2014

coveralls commented Oct 29, 2014

bradenmacdonald commented Oct 29, 2014