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

Create an XBlock API and implementation to get all users for a course #115

Open
2 tasks
pdpinch opened this issue Aug 24, 2015 · 14 comments
Open
2 tasks

Create an XBlock API and implementation to get all users for a course #115

pdpinch opened this issue Aug 24, 2015 · 14 comments

Comments

@pdpinch
Copy link
Member

pdpinch commented Aug 24, 2015

As an XBlock developer, I would like to have an API to get a list of all the students from the run time (edX or otherwise). For the SGA XBlock running in edX, this would avoid SGA making a direct call to Student Module.

The XBlock user API suggests how to approach this.

  1. Create a not-implemented function in XBlock
  2. Implement it in edX so it overrides the XBlock function

See PR #6013 in edx-platform and #273 in XBlock

Presumably, the code for the function in edX would be similar to the existing code for getting all users in SGA.

Part of #58

@ShawnMilo
Copy link

How do I get started on this? So far, I have devstack installed and was able to run paver devstack lms and then see edX running at http://localhost:8000/.

  • Which github repository are XBlocks in? They don't seem to be in the repo attached to this ticket.
  • Is there another XBlock I can copy/paste from as a starting point?
  • Do I need to start a new repo for this, or create new files or functions in an existing repo?
  • Do all Xblocks have a common test suite? If so, how do I run it?
  • How am I to push any changes I make? The Vagrant box doesn't have my Github user anywhere in it. Am I supposed to clone a repo to my own account and do a PR from there?
  • How do I make the software running in Vagrant use my version of the code -- do I have to add a "remote" to github, checkout a new repo instead, or should my github user have permission to the mitodl repository to push a new branch?

This is a huge and confusing ecosystem and I have no idea what to do next.

@carsongee
Copy link

  1. https://github.com/edx/XBlock/ issue is https://github.com/edx/XBlock/pull/273 that added the ref of the user service.
  2. You won't be making a new xblock, just modifying the reference
  3. It is included in that repo in the README
  4. The devstack checks out repos on your local machine and shares them into the VM, so if you have git setup on your local machine, it should all work, just do your git work locally
  5. See my notes from the talk: http://mitodl.github.io/edx-dev-intro-slides/#9

@ShawnMilo
Copy link

@pdpinch

There are six ORM calls in the SGA code right now. One is to the SubmissionsStudent model, and five are to the StudentModule model.

It appears that this ticket is only concerned with the single call to SubmissionsStudent based on the description, because the ticket concerns users and SubmissionsStudent returns actual users. StudentModule seems to be some sort of many-to-many relationship manager. However, the title of #58 is contains StudentModule, and not SubmissionsStudent.

Please clarify. It seems that this ticket is to address the single call to SubmissionsStudent, and that ticket #58 needs an additional task to eliminate direct access to the StudentModule model in favor of a new xblock (not the DjangoXBlockUserService referred to in this ticket). Removing all ORM calls in SGA seems to be the intent of #58, but currently there is no task to deal with the many-to-many relationship lookups.

@ShawnMilo
Copy link

Overview

Update edx-sga to retrieve a student list from the
xblock user service instead of
the Django ORM. (Students are users in this context.)

This is part of a refactoring effort for SGA, because it currently violates the specification
of an xblock by using the Django ORM instead
of consuming another xblock which exposes user data.

This issue does not complete the refactor, because there are ORM calls to the StudentModule model. Those will be left for another issue.
This issue specifically deals with removing the accessing of
the SubmissionsStudent model.

Tasks

  • Add a new function to the xblock user service for retrieving users. Should only raise a NotImplemented exception. This is just so it can be extended later within edx-platform.
  • Update edx-platform to flesh out the functionality (copy use of the SubmissionsStudent model from current edx-sga,
    which is doing it "illegally" now.).

Afterwards, issue #116 will take the next step, removing the reference to the SubmissionsStudent
model from SGA and replacing it with a call to the updated DjangoXBlockUserService.
Both #115 and #116 are
part of #58.

Task details

  • Clone the xblock repository
  • Create a virtualenv and pip install -r requirements.txt from xblock.
  • Run tests (nosetests) to confirm they work.
  • Add a stub to the existing xblock user service to retrieve users.
  • Add a test for the stub.
  • Ensure tests pass, discuss, and submit PR.
  • Locally modify edx to use modified xblock commit as a dependency (instructions below).
  • Go into edx-platform code and copy usage of SubmissionsStudent from SGA to extend the stub added to xblock.

Question: Once edx-platform is updated as described here, and SGA has not
yet been updated by #116, how do we test it?

The edx-platform work will be in the same place as the work
done here to consume
the user service already. The existing code should be usable as an example.

Specifically, look for code which imports from xblock_django.user_service import DjangoXBlockUserService.

Using modified xblocks

XBlocks are in the Vagrant image, not checked out locally, so to use a
locally-modified version:

  • Clone or symlink modified xblocks repo into the ./templates folder of vagrant project.
  • ssh into the devstack Vagrant box
  • sudo su edxapp
  • pip uninstall xblocks
  • pip install -e <path to xblocks folder under ./templates>

The -e flag for pip install causes pip to point to the source directory
instead of copying the files into the virtualenv, allowing development to
continue on the package without needing to refresh or re-install the package.

Other notes

These notes are not technically relevant to this ticket, but are part of the
training notes from the meeting which produced the rest of this document, so
I'm leaving them here for reference.

XBlock Studio

Used to develop xblocks. Runs locally on your machine in
a virtualenv. edx and devstack are not required.

An xblock is supposed to be a platform-agnostic standard, running on
edx-platform, CourseBuilder, and any other LMS that comes along.

Staff Graded Assignments

SGA is (almost) an xblock (imports from xblock, extends stuff). It currently
does things xblocks aren't allowed to do, such as talking directly to Django
models.

repos

xblock is in edx, but it has been forked to mitodl. I should create
a new branch in that repository.

Running edx

This will run both LMS and Studio.

# locally
cd <folder containing Vagrantfile>
vagrant up
vagrant ssh

# inside Vagrant box
sudo su edxapp
paver run lms

Login

username: [email protected]

password: edx

management commands

python manage.py lms --settings=devstack help

You can use set_staff, or just run shell just like in Django.

Django admin exists on both LMS and studio.

Creating a new course

  • create course
  • create section

Must add SGA manually in advanced settings so it shows up in course admin. This
ultimately just edits a JSON config file.

Use themes folder to dump projects/scripts/whatever that I want to access
from within the Vagrant box.

@pdpinch: Please review for accuracy.

@giocalitri: Please review and let me know if this makes sense to a fellow newbie.

@pdpinch
Copy link
Member Author

pdpinch commented Aug 26, 2015

@ShawnMilo looks accurate to me. I added a couple of links and edited it to refer to the "xblock user service" (it's an API) instead of a user service xblock. Seems clearer to me, but maybe not.

@ShawnMilo
Copy link

Thanks, @pdpinch. I don't think the changes are confusing.

Do you have an answer for the question in it? (Search for "question" in the comment.)

@pdpinch
Copy link
Member Author

pdpinch commented Aug 26, 2015

To answer the question about tests, I expect you'll need to add some more tests to https://github.com/edx/edx-platform/pull/6013/files#diff-40932c351140b7be18e9ee980ffa61df

@ShawnMilo
Copy link

@pdpinch: I have a question about the stub. The current stub is get_current_user, so the obvious name for the new stub is get_all_users.

  • However, should it be get _all_students?
  • The current SGA code filters by course and item. Should those be kwargs for this stub, or should this stub just accept **kwargs so the edx override can deal with it?

@pdpinch
Copy link
Member Author

pdpinch commented Aug 27, 2015

At the end of the day, those are going to be edX's decisions.

For now, I would stick with the current established naming, i.e. get_all_users. And I think **kwargs makes sense.

ShawnMilo added a commit to mitodl/XBlock that referenced this issue Aug 27, 2015
This is for SGA issue 115:

[issue 115](mitodl/edx-sga#115)
@ShawnMilo
Copy link

Question:

  1. I think I need to add code to edx-platform/common/djangoapps/xblock_django/user_service.py, and tests to edx-platform/common/djangoapps/xblock_django/tests/test_user_service.py for this ticket. Is that correct?
  2. What paver command do I run to execute those tests? If I run paver test_python it takes an hour to run. The option paver test_lib looks closest to what I want, (it says it tests stuff in common), but it doesn't execute edx-platform/common/djangoapps/xblock_django/tests/test_user_service.py.

@pdpinch
Copy link
Member Author

pdpinch commented Aug 28, 2015

@ShawnMilo
Copy link

Thanks. It looks like that works for test_system, but not the others.

test_python (no such option):

edxapp@precise64:~/edx-platform$ paver test_python -t common/djangoapps/xblock_django/tests/test_user_service.py
Usage: paver pavelib.tests.test_python [options]

paver: error: no such option: -t

test (no such option):

edxapp@precise64:~/edx-platform$ paver test -t common/djangoapps/xblock_django/tests/test_user_service.py
Usage: paver pavelib.tests.test [options]

paver: error: no such option: -t

@ShawnMilo
Copy link

I'm working on the edx-platform part of this. I have a stub of something I need to push up for review and to ask questions. I can't push to edx-platform (even in my own branch). I checked, and there doesn't seem to be an mitodl fork.

Do I need permission on something, or is there another place I should be pushing my branches?

@pdpinch
Copy link
Member Author

pdpinch commented Aug 28, 2015

The only option with edx/edx-platform is to push a PR. If it's not ready for review by their open source team, you can prefix the title with "WIP" and they'll ignore.

If you just want us to review before edX sees it, you can create a branch on github.com/mitocw/edx-platform (not mitodl, for historical reasons).

@pdpinch pdpinch added the on hold label Sep 7, 2015
@ShawnMilo ShawnMilo removed their assignment Mar 21, 2016
@pdpinch pdpinch modified the milestone: 8/29 Sprint Aug 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants