Made coach dashboard available on ccx in addition to master course by amir-qayyum-khan · Pull Request #10672 · openedx/openedx-platform

amir-qayyum-khan · 2015-11-19T13:14:07Z

Background

From issue mitocw#33

What is done in this PR

Studio Updates: None.

LMS Updates:

Previously ccx coach dashboard was only available on master course. Coach needs to open master course to access coach dashboard
After this fix coach can access his dashboard from both master course and ccx

@pdpinch @pwilkins @giocalitri

MIT PR mitocw#143

openedx-webhooks · 2015-11-19T13:14:24Z

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

pdpinch · 2015-11-19T21:14:41Z

Too many blank lines

sarina · 2015-11-19T21:49:43Z

Hi,

This branch needs to be rebased, and you’ll also need to manually test it again! Please see https://groups.google.com/forum/#!topic/edx-code/hh42xd16wuQ for more detail.

amir-qayyum-khan · 2015-11-20T14:21:23Z

Done with rebase for django 1.8 and test

cc @pdpinch @sarina

pdpinch · 2015-11-20T21:04:05Z

@amir-qayyum-khan this is failing the performance tests. Somehow its executing an additional query. Can you look into this?

amir-qayyum-khan · 2015-11-21T10:52:31Z

sure let me.

amir-qayyum-khan · 2015-11-23T10:49:54Z

issue fix @pdpinch @sarina

pdpinch · 2015-11-23T16:14:08Z

Added WIP to title, as I want to review the logic in this.

sarina · 2015-11-23T22:15:44Z

@amir-qayyum-khan Please try rebasing again, rather than slamming Jenkins with builds.

amir-qayyum-khan · 2015-11-25T12:19:39Z

@pdpinch @sarina can you review this? I am done with working.

pdpinch · 2015-11-25T19:04:33Z

Why did you remove the comma?

pdpinch · 2015-11-25T19:23:21Z

Why is this necessary?

@pdpinch Because code was not able to compare ccxLocator objects though it was able to compare courseLocator.
In sum when this line was comparing ccx keys it always give false even both key are match. Thats why I am deserialising keys and comparing them as strings.

Second option is block this PR and create small PR against ccx locator.py and override __eq__ method. Because we dont have overridden __eq__ method thats why when i compare 2 ccx locator objects it always returns false.

After that resume this PR

http://stackoverflow.com/questions/1227121/compare-object-instances-for-equality-by-their-attributes-in-python

Same question as the other PR: how is this handled elsewhere in edx-platform?

@pdpinch The issue was this
course.id is unicode object but ccx.id is not uni code it is number views.py So when we create ccx_locator i was not fully unicode because ccx_id was number. That why roles.py was not able to compare give ccx_locator with ccx_locator save in CourseAccessRole . See content of input ccx_locator and database value below
(
'course_id: ', CCXLocator(u'FAST', u'CC101', u'dummy_dec_2015', None, None, 10L),
''access_role.course_id: ', CCXLocator(u'FAST', u'CC101', u'dummy_dec_2015', None, None, u'10'),
'is Equal: ', False
)

Fix this issue by making ccx.id unicode
https://github.com/edx/edx-platform/pull/10672/files#diff-faebd8eb1d591f3ad3147fe858e4e849R136

pdpinch · 2015-11-25T19:28:08Z

I'm not sure why this is failing a bunch of CCX enrollment tests...

amir-qayyum-khan · 2015-11-26T10:29:40Z

@pdpinch Because now on ccx create user will be assign ccx_coach role on ccx (other then master course i.e he has already ccx_coach role on master course. Now he has this role on both master and ccx).
Sending email on assigning him ccx_coach role on ccx is pointless because he already receive one email for master course. Thats why I am removing that email.

pdpinch · 2015-12-03T14:32:40Z

I'm confused by the changes to lms/djangoapps/instructor/access.py -- does the existing code send e-mail by default whenever a user is granted access? I've never seen that in production.

pdpinch · 2015-12-03T14:34:26Z

Can we address this upstream in https://github.com/edx/ccx-keys ?

@pdpinch
yeah we can but then we need to hold this PR. it depends on this fix

Ok. Leave it as is, but let's open a PR on ccx-keys too.

pdpinch · 2015-12-03T16:32:33Z

@amir-qayyum-khan this is functioning OK in my devstack, but it's failing a python test locally.

And I still want to understand the changes to https://github.com/edx/edx-platform/pull/10672/files#diff-b289cefe1d85995559153339637dc4bb

amir-qayyum-khan · 2015-12-04T12:25:32Z

@pdpinch. When a user creates ccx I am not sending him email that he is now coach on ccx.
Because he got an email when staff assigned him coach role on master course.

In this PR we are assigning coach role on ccx to a user who is coach on master course.
The code at this point provide developer control that he want to notify user or not on an action

pdpinch · 2015-12-10T00:54:40Z

👍

sarina · 2015-12-10T01:37:17Z

This looks good to me as well. @amir-qayyum-khan could you please rebase atop master one last time? Then I think it's fine for @pdpinch to merge.

amir-qayyum-khan · 2015-12-10T10:40:48Z

Done with rebase @sarina

cc @pdpinch

Made coach dashboard available on ccx in addition to master course

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Nov 19, 2015

amir-qayyum-khan changed the title ~~Added coach role to ccx other then master course~~ Made coach dashboard available on ccx Nov 19, 2015

amir-qayyum-khan mentioned this pull request Nov 19, 2015

WIP Made coach dashboard available on ccx mitocw/edx-platform#143

Closed

amir-qayyum-khan changed the title ~~Made coach dashboard available on ccx~~ Made coach dashboard available on ccx in addition to master course Nov 19, 2015

openedx-webhooks added open edx community review and removed needs triage labels Nov 19, 2015

pdpinch reviewed Nov 19, 2015
View reviewed changes

Comment thread lms/djangoapps/ccx/tests/test_views.py Outdated

pdpinch Nov 19, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Too many blank lines

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch from fe399ec to 5a2db67 Compare November 20, 2015 14:10

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch 2 times, most recently from a96264c to bedeb5a Compare November 23, 2015 10:47

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch 2 times, most recently from 6f35772 to 6aca581 Compare November 23, 2015 15:14

pdpinch changed the title ~~Made coach dashboard available on ccx in addition to master course~~ WIP: Made coach dashboard available on ccx in addition to master course Nov 23, 2015

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch from 6aca581 to 909d264 Compare November 24, 2015 16:12

amir-qayyum-khan changed the title ~~WIP: Made coach dashboard available on ccx in addition to master course~~ Made coach dashboard available on ccx in addition to master course Nov 24, 2015

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch from 909d264 to 7383cf4 Compare November 25, 2015 10:20

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch 3 times, most recently from 08ac5cf to 1644768 Compare November 25, 2015 15:28

pdpinch reviewed Nov 25, 2015
View reviewed changes

Comment thread lms/djangoapps/ccx/views.py Outdated

pdpinch Nov 25, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Why did you remove the comma?

pdpinch reviewed Nov 25, 2015
View reviewed changes

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch 2 times, most recently from 1630187 to 2780629 Compare November 26, 2015 10:26

amir-qayyum-khan mentioned this pull request Nov 30, 2015

Added staff role on ccx to all the staff members of master course. mitocw/edx-platform#148

Closed

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch from 2780629 to 77013f7 Compare December 1, 2015 12:41

pdpinch mentioned this pull request Dec 2, 2015

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

Closed

pdpinch reviewed Dec 3, 2015
View reviewed changes

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch from 77013f7 to 4afd0b9 Compare December 10, 2015 09:32

Fixed display name ccx course on coach dashboard

85c7258

amir-qayyum-khan force-pushed the fix/aq/add_coach_role_on_ccx branch from 4afd0b9 to 85c7258 Compare December 10, 2015 09:34

pdpinch added a commit that referenced this pull request Dec 10, 2015

Merge pull request #10672 from mitocw/fix/aq/add_coach_role_on_ccx

f3285f7

Made coach dashboard available on ccx in addition to master course

pdpinch merged commit f3285f7 into openedx:master Dec 10, 2015

pdpinch deleted the fix/aq/add_coach_role_on_ccx branch December 10, 2015 11:48

pdpinch mentioned this pull request Dec 14, 2015

Access CCX dashboard through CCX (in addition to accessing it through master course) mitocw/edx-platform#33

Closed

Uh oh!

Conversation

amir-qayyum-khan commented Nov 19, 2015

Background

What is done in this PR

Uh oh!

openedx-webhooks commented Nov 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarina commented Nov 19, 2015

Uh oh!

amir-qayyum-khan commented Nov 20, 2015

Uh oh!

pdpinch commented Nov 20, 2015

Uh oh!

amir-qayyum-khan commented Nov 21, 2015

Uh oh!

amir-qayyum-khan commented Nov 23, 2015

Uh oh!

pdpinch commented Nov 23, 2015

Uh oh!

sarina commented Nov 23, 2015

Uh oh!

amir-qayyum-khan commented Nov 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdpinch commented Nov 25, 2015

Uh oh!

amir-qayyum-khan commented Nov 26, 2015

Uh oh!

pdpinch commented Dec 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdpinch commented Dec 3, 2015

Uh oh!

amir-qayyum-khan commented Dec 4, 2015

Uh oh!

pdpinch commented Dec 10, 2015

Uh oh!

sarina commented Dec 10, 2015

Uh oh!

amir-qayyum-khan commented Dec 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants