Skip to content

Adds staff and instructor users of the master course to CCX - #208

Closed
amir-qayyum-khan wants to merge 215 commits into
masterfrom
enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue
Closed

Adds staff and instructor users of the master course to CCX#208
amir-qayyum-khan wants to merge 215 commits into
masterfrom
enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue

Conversation

@amir-qayyum-khan

Copy link
Copy Markdown

Background

fixes #126, fixes #155, fixes #115 and fixes #205

This is an updated version of https://github.com/edx/edx-platform/pull/10877 which had to be reverted.

What is done in this PR

Studio Updates:

  • Remove ccx from staff dashboard inside studio. Because a master course can have unlimited ccx, staff's dashboard can be populated with a huge list of ccx along with master course

LMS Updates:

  • This is for course staff to see what ccx coaches are doing with course.
  • Now when a coach creates a new ccx, I am assigning staff role on this ccx course to all staff of master course.
  • Added instructors of master course as instructor(s) in ccx.
  • Also adds the master course staff users to new CCX created via the CCX REST APIs
  • Note: This code will add/enrol staff and instructors in ccx and now max student limit for ccx depends on number of staff and admins.
  • Fixed view as student issue on ccx, now staff can view as student and see how ccx view will appear to student
  • Fixed display name on ccx coach dashboard. Previously it was show display name of master course on coach dashboard.
To run migration manually on devstack
  • python manage.py lms migrate ccx 0003 --settings=devstack
    To reverse migration manually on devstack
  • python manage.py lms migrate ccx 0002 --settings=devstack

@pdpinch @giocalitri

  • Staff on Master course
    screen shot 2015-11-30 at 5 12 54 pm
  • Staff on CCX
    screen shot 2015-11-30 at 5 13 04 pm
  • Admin/instructors on master course
    screen shot 2015-12-10 at 5 10 18 pm
  • Admin/instructors on ccx
    screen shot 2015-12-10 at 5 10 22 pm

Fixed view as student

screen shot 2015-12-17 at 7 21 37 pm 2
screen shot 2015-12-17 at 7 21 53 pm 2
screen shot 2015-12-17 at 7 22 05 pm 2

Fixed display name of ccx on coach dashboard.

screen shot 2015-12-18 at 3 29 34 pm
screen shot 2015-12-18 at 3 29 44 pm
screen shot 2015-12-18 at 3 29 55 pm

@amir-qayyum-khan
amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch 2 times, most recently from 8ef712f to e5a6253 Compare February 23, 2016 10:37
@amir-qayyum-khan amir-qayyum-khan self-assigned this Feb 23, 2016
@pdpinch

pdpinch commented Feb 23, 2016

Copy link
Copy Markdown
Member

This looks to have all the changes I asked for, but the diff isn't correct because master doesn't include the revert from bf5d203

Can you update the cover letter to explain that this updates & restores 91bf48f which was reverted by bf5d203. Also, we need to cut the title down. It's way too long.

@pdpinch

pdpinch commented Feb 23, 2016

Copy link
Copy Markdown
Member

@giocalitri has some concerns about the reverse migration which he brought up over chat.

I am now confident that the 0002 reverse migration cannot work as it is 
because during the migration all the `lazy` decorated methods of the 
CCX model cannot be found.

this is how the reverse migration should be to work:
def reverse_add_master_course_staff_to_ccx_for_existing_ccx(apps, schema_editor):
    """
    Add all staff and admin of master course to respective CCX(s).
    """
    CustomCourseForEdX = apps.get_model("ccx", "CustomCourseForEdX")
    list_ccx = CustomCourseForEdX.objects.all()
    store = modulestore()
    for ccx in list_ccx:
        with store.bulk_operations(ccx.course_id):
            course = store.get_course(ccx.course_id)
        ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
        reverse_add_master_course_staff_to_ccx(course, ccx_locator, ccx.display_name)

@giocalitri

Copy link
Copy Markdown

@pdpinch since we are talking about this, he may wants to make the migration self contained and not import the functions from utils

he also needs to import modulestore

@giocalitri

Copy link
Copy Markdown

@pdpinch I also would like to take a deep look at this PR once you give your thumbsup.

@giocalitri

Copy link
Copy Markdown

This PR should include the entire code currently in #207, given that most of that PR is about tests missing in the previous code and the actual changes in the rest APIs are trivial.

@giocalitri

Copy link
Copy Markdown

@pdpinch what do you think? ^^

@pdpinch

pdpinch commented Feb 24, 2016 via email

Copy link
Copy Markdown
Member

@amir-qayyum-khan

Copy link
Copy Markdown
Author

@pdpinch @giocalitri done with adding tests for studio. (Test to check that ccx course filter out in studio dashboard).

@amir-qayyum-khan
amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from 1f2ae22 to 2b18e9c Compare March 10, 2016 11:56
Douglas Hall and others added 4 commits March 10, 2016 07:24
…_edx_proctoring_0_12_13

SOL-1653 SOL-1691 Upgrading edx-proctoring requirement to 0.12.13
Conflicts:
	lms/djangoapps/instructor/tests/test_api.py
	lms/djangoapps/instructor/utils.py
	lms/djangoapps/instructor/views/api.py
	lms/djangoapps/instructor/views/api_urls.py
	lms/djangoapps/instructor/views/instructor_dashboard.py
	lms/djangoapps/instructor_task/api.py
	lms/djangoapps/instructor_task/tasks.py
	lms/djangoapps/instructor_task/tasks_helper.py
	lms/djangoapps/instructor_task/tests/test_api.py
	lms/djangoapps/instructor_task/tests/test_tasks.py
	lms/djangoapps/instructor_task/tests/test_tasks_helper.py
	lms/envs/aws.py
	lms/envs/common.py
	lms/static/coffee/src/instructor_dashboard/data_download.coffee
	lms/templates/instructor/instructor_dashboard_2/data_download.html

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the convention is to use a single _

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • @giocalitri i think we use single _ for gettext and for unused variables we sue __,

  • pylintrc take __ and _ as good name,

    please see

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i just remembered

@giocalitri

Copy link
Copy Markdown

this is good to go on edx 👍

@giocalitri

Copy link
Copy Markdown

please squash before opening the PR against edx

cpennington and others added 9 commits March 10, 2016 10:32
…n-studio-based-on-xblockasides

XBlockAside improvement. Difficulty selectbox in Studio (based on new XBlockAside functionality)
WIP: Asynchronous download button for ORA2 data
clear_student_state(), a method defined by ORA, requires information
about the user making the request. Since this xblock is not bound,
we must provide that information explicitly.
Improve accessibility for dashboard course settings dropdown
Always uninstall python packages on the uninstall list
Update translations (autogenerated message)
@amir-qayyum-khan
amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from 2b18e9c to 79a0a67 Compare March 11, 2016 07:40
@amir-qayyum-khan
amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from 79a0a67 to eecb6d4 Compare March 11, 2016 07:46
@amir-qayyum-khan

Copy link
Copy Markdown
Author

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