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.

Sign up for GitHub

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

Jump to bottom

Added staff and instructor roles on ccx to all the staff and instructors of the master course plus fixed view as student masquerade and display name of ccx on coach dashboard #10877

Merged
merged 1 commit into from
Feb 18, 2016
Merged

Added staff and instructor roles on ccx to all the staff and instructors of the master course plus fixed view as student masquerade and display name of ccx on coach dashboard #10877

merged 1 commit into from
Feb 18, 2016

Conversation

amir-qayyum-khan
Copy link
Contributor

Background

fixes mitocw#126, mitocw#155 and mitocw#115

What is done in this PR

Studio Updates:

  • Remove ccx from staff dashboard inside studio. Because a master course can have unlimited ccx. If we allow display of all ccx of logged in staff (who is also staff of master course ) in studio dashboard then his (staff's) dashboard can be populate with huge list of ccx along with master course

LMS Updates:

  • Now when a coach creates a new ccx, I am assigning staff role on this ccx course to all staff of master course.
  • Now staff of master course has staff rights on master course as well as ccx courses.
  • This is for course staff to see what ccx coaches are doing with course.
  • Added instructors of master course as instructor(s) in ccx.
  • 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.

@pdpinch @giocalitri @pwilkins

  • 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

@openedx-webhooks
Copy link

Thanks for the pull request, @amir-qayyum-khan! I've created OSPR-1007 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Dec 8, 2015
@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch 3 times, most recently from 9891354 to 42e2f1d Compare December 8, 2015 10:55
@amir-qayyum-khan amir-qayyum-khan mentioned this pull request Dec 8, 2015
Closed
@pdpinch
Copy link
Contributor

pdpinch commented Dec 9, 2015

@amir-qayyum-khan as indicated in mitocw#126 users with the instructor/admin role need to be enrolled in the CCX as well as those with the staff role. It looks like you will need to do that explicitly.

@amir-qayyum-khan amir-qayyum-khan changed the title Added staff role on ccx to all the staff members of master course. Added staff and instructor roles on ccx to all the staff and instructors of the master course. Dec 10, 2015
@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch 9 times, most recently from b227b3c to 35592a0 Compare December 10, 2015 11:59
@amir-qayyum-khan
Copy link
Contributor Author

@pdpinch Done and rebased

@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch 2 times, most recently from bfcdb54 to 2b50955 Compare December 11, 2015 10:00
@pdpinch
Copy link
Contributor

pdpinch commented Dec 11, 2015

@amir-qayyum-khan I've found another issue with this.

Giving master course staff access to the CCX as staff makes the CCX appear in their studio listing. Even worse, the overridden display-name isn't used.

If there's an easy way to do it, I think we should suppress the display of CCXs in the studio listing. I think that would have to be a separate PR. Can you look into this?

An alternative would be to apply the display_name override, but that would still have the issue of polluting the studio listing with potentially hundreds of CCXs.

I'm changing the title to WIP because I don't think this should be merged until this is resolved.

@pdpinch pdpinch changed the title Added staff and instructor roles on ccx to all the staff and instructors of the master course. WIP Added staff and instructor roles on ccx to all the staff and instructors of the master course. Dec 11, 2015
@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 2b50955 to 43db3ed Compare December 14, 2015 11:44
@amir-qayyum-khan
Copy link
Contributor Author

Done @pdpinch

@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 43db3ed to 0f13f9c Compare December 17, 2015 14:15
@amir-qayyum-khan amir-qayyum-khan mentioned this pull request Dec 17, 2015
Closed
@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 0f13f9c to 3bede57 Compare December 18, 2015 10:26
@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from d222ca9 to a64bc9a Compare January 29, 2016 08:33
@amir-qayyum-khan
Copy link
Contributor Author

Done @giocalitri

@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from a64bc9a to a4ed79b Compare January 29, 2016 12:10
doctoryes
doctoryes reviewed Feb 12, 2016
View reviewed changes
cms/djangoapps/contentstore/views/course.py
# ignore deleted or errored courses

# Custom Courses for edX (CCX) is an edX feature for re-using course content.
# # CCXs cannot be edited in Studio (aka cms) and should not be shown in this dashboard.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Extra #.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from a4ed79b to d88bb73 Compare February 15, 2016 19:01
@amir-qayyum-khan
Copy link
Contributor Author

jenkins run js

doctoryes
doctoryes reviewed Feb 16, 2016
View reviewed changes
lms/djangoapps/ccx/tests/test_utils.py

# Create instructor account
self.coach = coach = AdminFactory.create()
self.client.login(username=coach.username, password="test")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Remove the extra coach and change to username=self.coach.username.

@doctoryes
Copy link
Contributor

My first pass is complete.

@edx/devops Please review the data migration in this PR.

@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from eeeee5b to f46afcf Compare February 17, 2016 09:31
@amir-qayyum-khan amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from f46afcf to 91bf48f Compare February 17, 2016 10:54
@pdpinch
Copy link
Contributor

pdpinch commented Feb 17, 2016

@amir-qayyum-khan can you add a comment(s) telling us what you've changed most recently -- or, in the future, you can add another commit so the reviewers can do a diff. If you go that route, you'd have to squash before the merge.

@amir-qayyum-khan
Copy link
Contributor Author

@pdpinch

Oh, I squashed commits after this message https://github.com/edx/edx-platform/pull/10877#issuecomment-184751615,

@jibsheet
Copy link
Contributor

👍 on the migration from DevOps

@pdpinch
Copy link
Contributor

pdpinch commented Feb 18, 2016

👍 from me.

@doctoryes and @giocalitri did Amir address all your concerns?

@doctoryes
Copy link
Contributor

Yes - 👍

@giocalitri
Copy link
Contributor

👍 also from me

pdpinch added a commit that referenced this pull request Feb 18, 2016
@pdpinch
Merge pull request #10877 from mitocw/enhancement/aq/add_master_cours…
dcb04cb 
…e_staff_in_ccx

Added staff and instructor roles on ccx to all the staff and instructors of the master course plus fixed view as student masquerade and display name of ccx on coach dashboard
@pdpinch pdpinch merged commit dcb04cb into openedx:master Feb 18, 2016
@pdpinch pdpinch deleted the enhancement/aq/add_master_course_staff_in_ccx branch February 18, 2016 16:09
This was referenced Feb 18, 2016
Closed
Closed
This was referenced Feb 23, 2016
Closed
Closed
@amir-qayyum-khan amir-qayyum-khan mentioned this pull request Mar 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
engineering review open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When creating a CCX, enroll the master course's instructors, as staff
7 participants
@amir-qayyum-khan @openedx-webhooks @pdpinch @ormsbee @giocalitri @doctoryes @jibsheet