Adds staff and instructor users of the master course to CCX by amir-qayyum-khan · Pull Request #11787 · openedx/openedx-platform

amir-qayyum-khan · 2016-03-11T07:49:25Z

Background

fixes mitocw#126, fixes mitocw#155, fixes mitocw#115 and fixes mitocw#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
Staff on CCX
Admin/instructors on master course
Admin/instructors on ccx

Fixed view as student

Fixed display name of ccx on coach dashboard.

openedx-webhooks · 2016-03-11T07:49:31Z

Thanks for the pull request, @amir-qayyum-khan! I've created OSPR-1184 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.

macdiesel · 2016-03-21T19:14:18Z

@aamir-khan can you tell me why this was reverted the first time and anything else we should be aware of for this go round?

pdpinch · 2016-03-21T19:51:35Z

Thanks for asking @macdiesel. Migrations failed due to legacy data on stage and edge. The updated PR now skips CCX with deprecated course keys. We have also been working with @edx/devops to remove those legacy CCX.

Also, we added more tests. Unfortunately it looks like the coverage results have gone stale.

pdpinch · 2016-03-21T19:52:25Z

jenkins run python

macdiesel · 2016-03-21T20:00:54Z

Thanks @pdpinch. Since we had issues with the migration last time I would like to get a thumb from @edx/devops as well on this one.

macdiesel · 2016-03-21T20:13:39Z

+        for staff in list_staff:
+            if staff in list_staff_ccx:
+                # allow 'staff' access on ccx to staff of master course
+                revoke_access(course_ccx, staff, 'staff')


I'm confused that the comment says to allow staff access but then the function called is revoke_access. Are these 2 lines congruous?

macdiesel · 2016-03-22T14:13:39Z

Documentation is not aligned correctly here.

Also please add documentation for the Argument:
https://sphinxcontrib-napoleon.readthedocs.org/en/latest/example_google.html

macdiesel · 2016-03-22T15:06:15Z

@amir-qayyum-khan Thank you for the submission. I have finished my first pass on this PR.

jibsheet · 2016-03-24T18:50:21Z

From talking to Max, neither of us can remember code in util vs the migration causing us issues on a rollback. I thought this migration died so hard that it never finished / needed rollback.

Perhaps you folks remember. I will review the migration itself tomorrow.

pdpinch · 2016-03-24T19:02:52Z

Sorry if I confused you all. The concern about the rollback code came from our developers.

My recollection is the same as yours -- the migration on stage.edx.org failed early enough that no rollback was needed.

In any case, @amir-qayyum-khan has already DRYed everything out, so the migration imports the util code.

Do you think the guard against deprecated course keys is sufficient to merge?

pdpinch · 2016-03-25T10:43:59Z

@macdiesel @jibsheet can we merge this?

I'd very much like to get this PR into the release getting cut this morning.

jibsheet · 2016-03-25T13:36:22Z

@pdpinch this doesn't look DRY to me? migrations/0003 contains a complete copy of add_master_course_staff_to_ccx from utils.py except the send_email is set to false by default.

Maybe I've misunderstood the conversation you had with @macdiesel but it seems weird to me to copy when nobody can remember why it would be necessary.

pdpinch · 2016-03-25T13:48:19Z

You're right. I was looking at the wrong thing.

@amir-qayyum-khan please remove the duplicate code.

We're not going to make the release, are we?

jibsheet · 2016-03-25T13:50:07Z

@pdpinch given test runs, probably not. You can reach out to @sanfordstudent if you think you can make the changes this morning. I have meetings until 11:30 unfortunately, so won't be able to look again after 10 (and my team is in those meetings with me). Sorry I didn't have time to look yesterday, was buried in other work but had carved out this morning for it.

pdpinch · 2016-03-25T13:56:20Z

FWIW, the reason to do the copy was discussed in https://github.com/edx/edx-platform/pull/11787/files/98421e3f1dfb5a4de62ed3b7619c8bb80529cca0#r57002351 https://github.com/edx/edx-platform/pull/11787/files/98421e3f1dfb5a4de62ed3b7619c8bb80529cca0#r57002351

We had two PRs in progress that made changes to ccx/utils.py. When this migration had a dependency on it, you couldn't successfully roll it back.

Maybe I've misunderstood the conversation you had with @macdiesel https://github.com/macdiesel but it seems weird to me to copy when nobody can remember why it would be necessary.

amir-qayyum-khan · 2016-03-25T17:29:47Z

@macdiesel @jibsheet should we or you merge this PR? I think it is ready.

jibsheet · 2016-03-25T17:37:41Z

@pdpinch unfortunately, github tells me those files/comments were force pushed away
@amir-qayyum-khan I'll try to review now, but I have a meeting starting in 20 minutes.

pdpinch · 2016-03-25T17:48:38Z

Thanks. We missed the cut anyway, so whenever you have time.

jibsheet · 2016-03-25T17:56:47Z

It looks like ccx_course is also a lift from utils.py, back from when the with ccx_course(ccx_key) as course_ccx existed in the migration, so that should also go away. You can also clean up the unused import.

jibsheet · 2016-03-25T17:57:35Z

@amir-qayyum-khan see my one note about other code I believe can go away, after that does, 👍
I believe Peter can merge once you check on that.

…ated to ccx

amir-qayyum-khan · 2016-03-25T22:21:42Z

@pdpinch Done with changes, just rebased now.

macdiesel · 2016-03-28T22:49:42Z

LGTM. 👍

amir-qayyum-khan · 2016-04-12T15:02:33Z

@macdiesel done with verification of this PR on stage. Every thing working as expected.
Thanks

cc @pdpinch

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Mar 11, 2016

amir-qayyum-khan mentioned this pull request Mar 11, 2016

Adds staff and instructor users of the master course to CCX mitocw/edx-platform#208

Closed

openedx-webhooks added awaiting prioritization and removed needs triage labels Mar 15, 2016

macdiesel reviewed Mar 21, 2016
View reviewed changes

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from eecb6d4 to 98421e3 Compare March 22, 2016 11:00

macdiesel reviewed Mar 22, 2016
View reviewed changes

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch 4 times, most recently from 3f3a660 to 41f6796 Compare March 24, 2016 11:37

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from c77ffe9 to e01fc5a Compare March 25, 2016 16:33

jibsheet reviewed Mar 25, 2016
View reviewed changes

openedx-webhooks added engineering review and removed awaiting prioritization labels Mar 25, 2016

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from e01fc5a to b594a77 Compare March 25, 2016 19:33

Added master course staff and admins to ccx and fixed same issues rel…

795ead8

…ated to ccx

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch from b594a77 to 795ead8 Compare March 25, 2016 22:19

macdiesel merged commit 1dff9d4 into openedx:master Mar 28, 2016

pdpinch mentioned this pull request Mar 29, 2016

Add staff and instructors users as staff of a CCX created via REST APIs mitocw/edx-platform#205

Closed

pdpinch deleted the enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue branch March 29, 2016 13:53

amir-qayyum-khan mentioned this pull request Apr 13, 2016

Fixed course not found against ccx issue on migration 0003 #12137

Merged

amir-qayyum-khan mentioned this pull request Apr 18, 2016

CCX display name should override course display name when view coach dashboard mitocw/edx-platform#115

Closed

Uh oh!

Conversation

amir-qayyum-khan commented Mar 11, 2016

Background

What is done in this PR

To run migration manually on devstack

To reverse migration manually on devstack

Fixed view as student

Fixed display name of ccx on coach dashboard.

Uh oh!

openedx-webhooks commented Mar 11, 2016

Uh oh!

macdiesel commented Mar 21, 2016

Uh oh!

pdpinch commented Mar 21, 2016

Uh oh!

pdpinch commented Mar 21, 2016

Uh oh!

macdiesel commented Mar 21, 2016

Uh oh!

macdiesel Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

macdiesel Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

macdiesel commented Mar 22, 2016

Uh oh!

jibsheet commented Mar 24, 2016

Uh oh!

pdpinch commented Mar 24, 2016

Uh oh!

pdpinch commented Mar 25, 2016

Uh oh!

jibsheet commented Mar 25, 2016

Uh oh!

pdpinch commented Mar 25, 2016

Uh oh!

jibsheet commented Mar 25, 2016

Uh oh!

pdpinch commented Mar 25, 2016

Uh oh!

amir-qayyum-khan commented Mar 25, 2016

Uh oh!

jibsheet commented Mar 25, 2016

Uh oh!

pdpinch commented Mar 25, 2016

Uh oh!

jibsheet Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

jibsheet commented Mar 25, 2016

Uh oh!

amir-qayyum-khan commented Mar 25, 2016

Uh oh!

macdiesel commented Mar 28, 2016

Uh oh!

amir-qayyum-khan commented Apr 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants