mitodl / edx-sga

Staff Graded Assignment XBlock for the edX platform
GNU Affero General Public License v3.0
34 stars 108 forks source link

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

Open pdpinch opened 9 years ago

pdpinch commented 9 years ago

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 commented 9 years ago

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/.

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

carsongee commented 9 years ago
  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 commented 9 years ago

@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 commented 9 years ago

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

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

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:

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: staff@example.com

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

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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.

pdpinch commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

Does this help? https://github.com/edx/edx-platform/wiki/Test-engineering-FAQ#how-do-i-run-paver-test-for-a-single-file

ShawnMilo commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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).