Added staff role on ccx to all the staff members of master course. by amir-qayyum-khan · Pull Request #148 · mitocw/edx-platform

amir-qayyum-khan · 2015-11-30T11:02:02Z

Background

From issue #126

What is done in this PR

Studio Updates: None.

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.
Move code that is not rendering view to utility.

@pdpinch @giocalitri @pwilkins

Staff on Master course
Staff on CCX

amir-qayyum-khan · 2015-11-30T11:03:57Z

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 PRs https://github.com/edx/edx-platform/pull/10672 and https://github.com/mitocw/edx-platform/pull/148/files

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

I don't see a corresponding __eq__ method in https://github.com/edx/opaque-keys/blob/master/opaque_keys/edx/locator.py -- how is this dealt with elsewhere in edX?

pdpinch · 2015-11-30T14:01:05Z

is this a master course key?

No it is master course object

amir-qayyum-khan · 2015-11-30T14:23:48Z

@pdpinch fixed issues

amir-qayyum-khan · 2015-12-01T13:04:18Z

@pdpinch should i create this on upstream?

pdpinch · 2015-12-01T13:24:05Z

Ok. Can you add "wip" to the title, until someone from our team can give it a thumbs up?

I am mobile.

On Dec 1, 2015, at 8:04 AM, Amir Qayyum Khan notifications@github.com wrote:

@pdpinch should i create this on upstream?

—
Reply to this email directly or view it on GitHub.

giocalitri · 2015-12-01T21:21:04Z

I'm not sure this is the right or current way they want the docstrings.

amir-qayyum-khan · 2015-12-02T08:02:55Z

Done @giocalitri

giocalitri · 2015-12-02T15:59:41Z

Sorry to insist on this, but I think the way they want docstrings now is something like this:

Args: course (<the object type here>): Master course instance ccx_key (<the object type here>): CCX course key ... Returns: None # Not sure this is actually necessary

Actually I have seen both, so I am not sure

giocalitri · 2015-12-02T16:06:47Z

I know you put the test in the test_views.py, but I feel like we need another one in test_api.py

amir-qayyum-khan · 2015-12-03T11:52:36Z

@giocalitri Done with moving code and creating test file.
Removed api.py because the function i made is not api but utility function.
So i moved it in utility.
Updated comment as well.

CC @pdpinch

giocalitri · 2015-12-03T14:49:54Z

can please you use the full path for this? Even if it is correct I would avoid using this kind of imports unless the module is in the same directory of the file that imports it

We were logging warnings about group names when no group names were requested. This will clean up Splunk a little bit.

…d-time Release/due times no longer round to half hours.

…jasmine-tests ECOM-3103: Added Jasmine tests for the Financial Assistance form

Fix spurious Zendesk warnings.

…-tests Problem type bok choy and a11y tests

Auto setting of verification deadlines with manual overrides

Fix issue with unicode characters in certificates.

These two directories are not added to sys.path when running the application, so there is no need to add them for pylint. When they are added, they make it impossible to run pylint properly because lms.lib.xblock is imported when trying to import xblock.

Update XBlock to 0.4.4 (just a packaging change)

…_ccx Made coach dashboard available on ccx in addition to master course

…_hashing Use secure salt for retirement username/email hashing.

amir-qayyum-khan added the in progress label Nov 30, 2015

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 3a0b91a to 7a2452f Compare November 30, 2015 11:02

amir-qayyum-khan reviewed Nov 30, 2015
View reviewed changes

amir-qayyum-khan changed the title ~~WIP Added staff role on ccx to all the staff members of master course.~~ Added staff role on ccx to all the staff members of master course. Nov 30, 2015

amir-qayyum-khan added Needs Review CCX and removed in progress labels Nov 30, 2015

pdpinch reviewed Nov 30, 2015
View reviewed changes

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 4b40f84 to 28e8d47 Compare November 30, 2015 14:23

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch 3 times, most recently from 8b720cb to 18028d9 Compare December 1, 2015 09:04

giocalitri reviewed Dec 1, 2015
View reviewed changes

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

giocalitri Dec 1, 2015

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'm not sure this is the right or current way they want the docstrings.

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 18028d9 to 34d9e3d Compare December 2, 2015 07:57

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 34d9e3d to 095cfbd Compare December 2, 2015 08:15

giocalitri reviewed Dec 2, 2015
View reviewed changes

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 095cfbd to 7f173d0 Compare December 3, 2015 11:50

giocalitri reviewed Dec 3, 2015
View reviewed changes

Christine Lytwynec and others added 15 commits December 8, 2015 14:39

Convert some problem type tests to bok choy and add a11y test

a9b308e

Update XBlock to 0.4.4 (just a packaging change)

586dab5

Fix issue with unicode characters in certificates.

f5c2f49

If platform name or "All rights reserved" translation contain non-ascii characters, certificate HTML view would break with "'ascii' codec can't encode character...".

Finished tests

46dc9d7

Fix spurious Zendesk warnings.

2d40c66

We were logging warnings about group names when no group names were requested. This will clean up Splunk a little bit.

Merge pull request openedx#10788 from simbs/mariana/remove-force-roun…

ef4f39f

…d-time Release/due times no longer round to half hours.

Merge pull request openedx#10907 from edx/alasdair/ECOM-3103-fa-form-…

5ecca7c

…jasmine-tests ECOM-3103: Added Jasmine tests for the Financial Assistance form

Merge pull request openedx#10910 from edx/peter-fogg/fix-zendesk-logging

9187ca8

Fix spurious Zendesk warnings.

Merge pull request openedx#10814 from edx/clytwynec/problem-type-a11y…

175241d

…-tests Problem type bok choy and a11y tests

Auto setting of verification deadlines with manual overrides

522095e

Merge pull request openedx#10912 from edx/bderusha/auto-deadlines-sp

3e96357

Auto setting of verification deadlines with manual overrides

Merge pull request openedx#10902 from open-craft/unicode-certs

b9bfe11

Fix issue with unicode characters in certificates.

Merge pull request openedx#10870 from edx/ned/new-xblock

3603ede

Update XBlock to 0.4.4 (just a packaging change)

Fixed display name ccx course on coach dashboard

85c7258

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch 3 times, most recently from 409774a to 4a1c456 Compare December 10, 2015 11:37

Merge pull request openedx#10672 from mitocw/fix/aq/add_coach_role_on…

f3285f7

…_ccx Made coach dashboard available on ccx in addition to master course

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch 6 times, most recently from b227b3c to 35592a0 Compare December 10, 2015 11:59

Added staff role on ccx to staff of master course

bfcdb54

amir-qayyum-khan force-pushed the enhancement/aq/add_master_course_staff_in_ccx branch from 35592a0 to bfcdb54 Compare December 10, 2015 12:27

pdpinch closed this Dec 10, 2015

pdpinch removed the Needs Review label Dec 10, 2015

tehreem-sadat pushed a commit that referenced this pull request Jan 13, 2020

Merge pull request #148 from edx/juliasq/fix_user_retirement_salt_for…

21245f3

…_hashing Use secure salt for retirement username/email hashing.

Uh oh!

Conversation

amir-qayyum-khan commented Nov 30, 2015

Background

What is done in this PR

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!

amir-qayyum-khan commented Nov 30, 2015

Uh oh!

amir-qayyum-khan commented Dec 1, 2015

Uh oh!

pdpinch commented Dec 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amir-qayyum-khan commented Dec 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giocalitri commented Dec 2, 2015

Uh oh!

amir-qayyum-khan commented Dec 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants