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

Content libraries MVP #6459

Merged
merged 100 commits into from Jan 13, 2015

Conversation

Projects
None yet
10 participants
@bradenmacdonald
Member

bradenmacdonald commented Jan 6, 2015

Description: This is our minimum viable implementation of content libraries ("Problem Banks") (SOL-11). This PR will be used for QA and bug fixing before we merge this feature branch to master.

Sandbox URL: CMS http://content-libraries.sandbox.opencraft.com:18010/ - LMS http://content-libraries.sandbox.opencraft.com/

Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

Dependencies: Already merged: Support for Content libraries in the modulestore (#6033) and in opaque keys (edx/opaque-keys#46)

Constituent PRs: All the code on this branch has already gone through a code review, except for 76d3f35 and any future bug fixes we make once all the constituent PRs have been merged:

  • Studio support for creating and editing libraries #6046 (SOL-1, SOL-2, SOL-3)
  • LibraryContent - Display content from a library in a course #6155 (SOL-5, SOL-6, SOL-7, SOL-8, SOL-117)
  • Pagination for Content libraries #6163 (SOL-4)
  • Content library permissions & collaboration #6283 (SOL-81)
  • Filter library content by CAPA problem type #6346 (SOL 122)
  • Content Libraries UI Updates #6388 (SOL-80)
  • Content libraries settings overrides #6399 (SOL-46)
  • Content libraries analytics enhancements #6492 (SOL-121) - depends on the one above
  • Sidebar text for Content Library page in Studio #6426
  • Improved test coverage #6499

CC @antoviaque @Kelketek @e-kolpakov @mtyaka

@antoviaque

This comment has been minimized.

Member

antoviaque commented Jan 6, 2015

@bradenmacdonald The jenkins test run doesn't seem to have been automatically triggered on this?

@jzoldak Any idea why this would not happen? Is there a way for us to trigger them manually when this happens? Usually we amend the last commit to attempt to retrigger a jenkins run, but on a feature branch with PRs that depend on it, that would require to rebase all dependent PRs afterwards, which wouldn't be practical.

@bradenmacdonald bradenmacdonald force-pushed the content-libraries branch to 49c4500 Jan 8, 2015

@antoviaque

This comment has been minimized.

Member

antoviaque commented Jan 9, 2015

@jzoldak

This comment has been minimized.

Contributor

jzoldak commented Jan 9, 2015

@antoviaque @bradenmacdonald
We're in the middle of switching to a new jenkins infrastructure. Here are URLs to trigger jobs manually on the old jenkins server and the new one.

bradenmacdonald and others added some commits Oct 29, 2014

Three levels of user permissions for content libraries:
Admin ("Instructor") - Can edit and assign permissions to other users
Normal ("Staff") - Can edit
User - Can view the library and use content from it but cannot edit it or its blocks.
@antoviaque

This comment has been minimized.

Member

antoviaque commented Jan 13, 2015

@e-kolpakov To make it easier to review everything at once for the MVP merge, I've merged that bugfix here too. So the commits to review to be able to merge the current MVP PR to master are: 6ea5a8a 012e6e6
9f6cf15

@chrisndodge

This comment has been minimized.

Contributor

chrisndodge commented Jan 13, 2015

@bradenmacdonald any chance to write - within a reasonable level of effort - a quick automated test regarding the fix at 9f6cf15?

@chrisndodge

This comment has been minimized.

Contributor

chrisndodge commented Jan 13, 2015

Other than my suggestion re: adding a unit test that would have caught the bug that was fixed at 9f6cf15, looks good to me.

Congratulations!

@e-kolpakov

This comment has been minimized.

Contributor

e-kolpakov commented Jan 13, 2015

@chrisndodge I'm writing autotest right now.

@antoviaque

This comment has been minimized.

Member

antoviaque commented Jan 13, 2015

@chrisndodge Thank you for having a look so quickly! You even arrived before @e-kolpakov had had time to add the test : ) It's now done, and I added it to the current PR: 720834c

@chrisndodge

This comment has been minimized.

Contributor

chrisndodge commented Jan 13, 2015

Thanks for the new acceptance test which simulates that previous error condition.

LGTM. +1

@cahrens

This comment has been minimized.

cahrens commented Jan 13, 2015

👍 I have a requested clarification on the test case, but otherwise LGTM. Congrats on finishing up all this work!

@e-kolpakov

This comment has been minimized.

Contributor

e-kolpakov commented Jan 13, 2015

@cahrens yes the text is there only to assert on it later. The approach is reused from other test a couple of lines above.

@bradenmacdonald I'm about logging off for today, so could you please polish the tests if needed?

@bradenmacdonald bradenmacdonald force-pushed the content-libraries branch to bce8ee6 Jan 13, 2015

@bradenmacdonald

This comment has been minimized.

Member

bradenmacdonald commented Jan 13, 2015

@cahrens I have simplified the bok choy test as you requested: bce8ee6#diff-1 . I also squashed the various commits for this last bugfix. Hope that's good? I'd like to merge this once the build is green.

@cahrens

This comment has been minimized.

cahrens commented Jan 13, 2015

Looks good! 👍

bradenmacdonald added a commit that referenced this pull request Jan 13, 2015

@bradenmacdonald bradenmacdonald merged commit 6be35e0 into master Jan 13, 2015

1 check passed

default "Tests passed, generating reports..."
Details
@bradenmacdonald

This comment has been minimized.

Member

bradenmacdonald commented Jan 13, 2015

Content Libraries / Problem Banks MVP is now merged. Congratulations and thanks to everyone who contributed to it!

@chrisndodge

This comment has been minimized.

Contributor

chrisndodge commented Jan 13, 2015

@bradenmacdonald congratulations and thanks for all the hard work!

I just realized that neither @cahrens nor I asked you to squash your commits before merging at the end of the review, which we typically do. Typically, we prefer a single commit before merging.

I'm not sure if I'd suggest reverting this and squashing and re-applying. @cahrens @smagoun what do you think?

@bradenmacdonald

This comment has been minimized.

Member

bradenmacdonald commented Jan 13, 2015

@chrisndodge Ah! Sorry :-/ I didn't realize that was the case. (I personally find that having more of the commit history helps future developers with understanding the code changes that were made in this PR.) I can definitely do that if you'd like, but it will involve resetting master back and thus a force push to master.

But anyhow just let me know - I'm happy to squash this whole thing down to a single commit, re-run the build, and re-merge if you'd like me to.

@chrisndodge

This comment has been minimized.

Contributor

chrisndodge commented Jan 13, 2015

Yea, reverting, squashing, re-merging might be more hassle than it is worth at this point in time. I wouldn't do anything right now, unless anyone else has a major opinion on this.

Going forward, we'll try to remember to remind all contributors to squash commits after the review is done.

@cahrens

This comment has been minimized.

cahrens commented Jan 13, 2015

Yes, @andy-armstrong is now struggling through some rebasing. I'm sorry I forgot to check if commits were squashed.

Note that we don't necessarily require a single commit per PR, but commits should be logical chunks of work (for instance, a commit per PR merged into the feature branch is fine, if the commits represent stories). Commits like "pylint fixes" or "Addressed nits" aren't helpful and increase the merge conflicts.

@bradenmacdonald

This comment has been minimized.

Member

bradenmacdonald commented Jan 13, 2015

Ok, let me know if any action is required from me. I agree that ~one commit per PR merged to this branch would be nice.

@cahrens

This comment has been minimized.

cahrens commented Jan 13, 2015

It would be nice if ENABLE_CONTENT_LIBRARIES existed in the common envs.py file (with default value of False) so it was easier to turn on in sandboxes. I think @andy-armstrong is going to do that as part of cleaning up the conflicts we are having with setting visibility on an xblock on the container page.

@bradenmacdonald and @marcotuts does this look right to you when there are no content libraries?

image

@cahrens

This comment has been minimized.

cahrens commented Jan 13, 2015

@bradenmacdonald and @marcotuts Note also that error messaging should be updated. It isn't possible to have both a library and a course with the same org and course number.

image

@e-kolpakov

This comment has been minimized.

Contributor

e-kolpakov commented Jan 20, 2015

@jzoldak it appears build triggers requires some sort of special permission. At least it doesn't work for me: "e-kolpakov is missing the Task/Build permission", despite I've granted all permissions it asked for. Is that supposed to be used for core devs only, or access is granted on-demand?

@jzoldak

This comment has been minimized.

Contributor

jzoldak commented Jan 20, 2015

@e-kolpakov Requirements are that you need to be a member of the edx org on GitHub and you need to make that membership public.
@antoviaque can you help him with this?

@antoviaque

This comment has been minimized.

Member

antoviaque commented Jan 21, 2015

@jzoldak Thanks for the info - yup, I'll see if we can get Eugeny in that group, perfect.

@Nelapa

This comment has been minimized.

Nelapa commented on cms/static/js/views/utils/view_utils.js in 3e0f08e Aug 6, 2015

validateURLItemEncoding implies that the url item should be required and duplicates work of validateRequiredField. I think it would be better to call validateRequiredField explicitly for field being checked in the proper place then making current function do 2 validations. Found this "smell" while writing 'auto-key-generation' for our sandbox Studio.

This comment has been minimized.

Contributor

e-kolpakov replied Aug 7, 2015

@Nelapa good catch, but as you can see it's there for almost half a year, so we have switched to other things long ago. You have two options here:

  1. Report it as a bug to edX platform, so they can add it to backlog, prioretize and hopefully fix someday (maybe even handing it over to original authors) - which taking into account the impact of the issue is unlikely to happen in near future.
  2. Go ahead and fix it yourself, than send a PR.

If you settle for the second option - don't hesitate asking questions here or on open edX mailing list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment