Content libraries MVP - #6459
Conversation
|
@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. |
ba403bb to
49c4500
Compare
|
FYI QA tests are all green: https://openedx.atlassian.net/wiki/display/SOL/Content+Libraries+-+Testing+Results |
|
@antoviaque @bradenmacdonald |
SOL-1, SOL-2, SOL-3
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.
|
@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: edx@6ea5a8a edx@012e6e6 |
|
@bradenmacdonald any chance to write - within a reasonable level of effort - a quick automated test regarding the fix at 9f6cf15? |
|
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! |
|
@chrisndodge I'm writing autotest right now. |
|
@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: edx@720834c |
|
Thanks for the new acceptance test which simulates that previous error condition. LGTM. +1 |
|
👍 I have a requested clarification on the test case, but otherwise LGTM. Congrats on finishing up all this work! |
|
@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? |
720834c to
bce8ee6
Compare
|
@cahrens I have simplified the bok choy test as you requested: edx@bce8ee69#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. |
|
Looks good! 👍 |
|
Content Libraries / Problem Banks MVP is now merged. Congratulations and thanks to everyone who contributed to it! |
|
@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? |
|
@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. |
|
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. |
|
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. |
|
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. |
|
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?
|
|
@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.
|
|
@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? |
|
@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. |
|
@jzoldak Thanks for the info - yup, I'll see if we can get Eugeny in that group, perfect. |
10 participants
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 (openedx/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:
CC @antoviaque @Kelketek @e-kolpakov @mtyaka