Fixed progress page update on save ccx and grade book crash issue #13019

amir-qayyum-khan · 2016-07-18T15:13:05Z

Background

fixes mitocw#252 https://openedx.atlassian.net/browse/TNL-4892

What is done in this PR

Studio Updates: None.
LMS Updates:

Add signal on save_ccx api, So that other application can be up to date when ccx contents change,

@pdpinch @giocalitri

To test:

Create a coach
create a student
Login to edX platform as coach, open coach dashboard. change schedule of subsection aka sequential and press save.
Login as student (CCX enrolled student). Go to dashboard then open ccx course and go to progress and see the due date of that subsection(problem).

openedx-webhooks · 2016-07-18T15:13:13Z

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

nedbat · 2016-07-20T19:04:58Z

lms/djangoapps/ccx/tests/test_views.py

@@ -131,6 +134,16 @@ def setup_students_and_grades(context):
                    )


+def unhide(unit):
+    """
+    Recursively unhide a unit and all of its children in the CCX


I know this was a pre-existing function, but why recursion? A simple loop would do! :)

nedbat · 2016-07-20T19:05:55Z

@cahrens @doctoryes CCX stuff often goes to platform, but this seems to affect touch UI, I wonder if TNL should take it?

cahrens · 2016-07-20T19:15:06Z

I can take it-- I've reviewed #11647.

cahrens · 2016-07-20T19:15:56Z

Though note that there are only Python files in this PR... So if @doctoryes WANTS to take the PR, I'm fine with that.

cahrens · 2016-07-20T19:17:53Z

And furthermore, @nasthagiri may wish to review since she opened the bug initially.

doctoryes · 2016-07-25T14:30:36Z

@cahrens I will review it.

cahrens · 2016-07-25T14:34:04Z

Great!

doctoryes · 2016-07-26T15:58:04Z

LGTM. One question:
Are there other places that need to send the course_published signal? For example, there's a create_ccx view - why does it and potentially other create/update operations not need to send this signal for other parts of the system to consume?

pdpinch · 2016-07-26T16:11:51Z

That's a good question.

@amir-qayyum-khan can you confirm that adding this signal to save_ccx catches all potential changes, i.e., changes to the schedule or the grading policy?

The CCX isn't very interested at the point of create_ccx, but it seems like we should add the signal there as well for completeness.

I can't think of any other places whether the coach can modify the ccx overrides.

amir-qayyum-khan · 2016-07-26T17:06:37Z

yes i verified changes for schedule save but need to double check policy
because there is another api in ccx app to save grading policy.

No need to add signal on ccx create api, because when ccx create after
that we enroll students and they see all updates in ccx.

On Jul 26, 2016 9:12 PM, "Peter Pinch" [email protected] wrote:

That's a good question.

@amir-qayyum-khan https://github.com/amir-qayyum-khan can you confirm
that adding this signal to save_ccx catches all potential changes, i.e.,
changes to the schedule or the grading policy?

The CCX isn't very interested at the point of create_ccx, but it seems
like we should add the signal there as well for completeness.

I can't think of any other places whether the coach can modify the ccx
overrides.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/edx/edx-platform/pull/13019#issuecomment-235319405,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJ8rEhNmJv6ztg5IyHl5vPunEu5O1cCpks5qZjHRgaJpZM4JO0Se
.

amir-qayyum-khan · 2016-07-29T13:20:27Z

@doctoryes made some changes, i am sorry I squashed commits. here are the changes listed below:

Added signal in all paths where ccx course is change including rest api
fixed issue related to gradebook and csv files. Made ccx id unicode to fix grade book crash issue when getting course_structure = get_course_blocks(student, course.location)
add test case

cc @pdpinch

amir-qayyum-khan · 2016-07-29T13:57:02Z

jenkins run bokchoy

doctoryes · 2016-07-29T14:12:12Z

lms/djangoapps/ccx/views.py

+    )
+    for rec, response in responses:
+        log.info('Signal fired when course is published. Receiver: %s. Response: %s', rec, response)
+
    url = reverse(
        'ccx_coach_dashboard',
        kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)}


Why does the signal line above need a unicode(ccx.id) but this course_id construction does not?

it should be, but CCXLocator with non unicode also works, it only give issues when we compare same CCXLocator one with ccx.id unicode and one with not.

let me fix through out views

doctoryes · 2016-07-29T14:31:43Z

While this change seems good enough to merge (after tests pass), I'd like to see some follow-on work:

Check all instances of this construction to determine whether ccx.id needs to be wrapped in unicode():
- ccx_key = CCXLocator.from_course_locator(course_key, ccx.id)
- For example, this location (and others) don't do the wrapping:
  - https://github.com/edx/edx-platform/blob/master/lms/djangoapps/ccx/tasks.py#L36
Determine whether tests already exist for checking the correct number of signals are emitted when performing the operations for which course_published signals were added.
- No tests were added which confirm that the added signals are sent at all for some operations and the correct number of signals are sent.

@pdpinch Thoughts?

P.S. Ah - I see @amir-qayyum-khan has already submitted a commit partially addressing my first point (while I was typing this list).

amir-qayyum-khan · 2016-07-29T15:31:18Z

jenkins run bokchoy

amir-qayyum-khan · 2016-07-29T17:26:30Z

@doctoryes all test are complete, can u give thumbs up on this?

doctoryes · 2016-07-29T17:38:48Z

👍

amir-qayyum-khan · 2016-08-08T13:11:44Z

@doctoryes done with verification on stage.

doctoryes · 2016-08-08T13:42:12Z

@amir-qayyum-khan Thanks! I'll mark testing as complete on the release page.

amir-qayyum-khan · 2016-08-08T13:42:45Z

thank you @doctoryes

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

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

Fixed progress page update on save ccx mitocw/edx-platform#253

Merged

nedbat reviewed Jul 20, 2016
View reviewed changes

openedx-webhooks added community manager review and removed needs triage labels Jul 20, 2016

openedx-webhooks added awaiting prioritization and removed community manager review labels Jul 21, 2016

amir-qayyum-khan force-pushed the fix/aq/emit_course_published_event branch from bcdf670 to 71b0174 Compare July 26, 2016 14:45

openedx-webhooks added engineering review waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization engineering review labels Jul 26, 2016

amir-qayyum-khan force-pushed the fix/aq/emit_course_published_event branch 4 times, most recently from 13ca30a to 5a32672 Compare July 29, 2016 13:00

Fixed progress page update on save ccx

d5fcb60

amir-qayyum-khan force-pushed the fix/aq/emit_course_published_event branch from 5a32672 to d5fcb60 Compare July 29, 2016 13:12

amir-qayyum-khan changed the title ~~Fixed progress page update on save ccx~~ Fixed progress page update on save ccx and grade book crash issue Jul 29, 2016

doctoryes reviewed Jul 29, 2016
View reviewed changes

Made ccx id unicode in app

34034aa

amir-qayyum-khan force-pushed the fix/aq/emit_course_published_event branch from a4d3e7d to 34034aa Compare July 29, 2016 14:38

doctoryes merged commit 70929ff into openedx:master Jul 29, 2016

pdpinch deleted the fix/aq/emit_course_published_event branch August 5, 2016 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed progress page update on save ccx and grade book crash issue #13019

Fixed progress page update on save ccx and grade book crash issue #13019

amir-qayyum-khan commented Jul 18, 2016 •

edited

Loading

openedx-webhooks commented Jul 18, 2016

nedbat Jul 20, 2016

nedbat commented Jul 20, 2016

cahrens commented Jul 20, 2016

cahrens commented Jul 20, 2016

cahrens commented Jul 20, 2016

doctoryes commented Jul 25, 2016

cahrens commented Jul 25, 2016

doctoryes commented Jul 26, 2016

pdpinch commented Jul 26, 2016

amir-qayyum-khan commented Jul 26, 2016

amir-qayyum-khan commented Jul 29, 2016 •

edited

Loading

amir-qayyum-khan commented Jul 29, 2016

doctoryes Jul 29, 2016

amir-qayyum-khan Jul 29, 2016

doctoryes commented Jul 29, 2016

amir-qayyum-khan commented Jul 29, 2016

amir-qayyum-khan commented Jul 29, 2016

doctoryes commented Jul 29, 2016

amir-qayyum-khan commented Aug 8, 2016

doctoryes commented Aug 8, 2016

amir-qayyum-khan commented Aug 8, 2016

Fixed progress page update on save ccx and grade book crash issue #13019

Fixed progress page update on save ccx and grade book crash issue #13019

Conversation

amir-qayyum-khan commented Jul 18, 2016 • edited Loading

Background

What is done in this PR

To test:

openedx-webhooks commented Jul 18, 2016

nedbat Jul 20, 2016

Choose a reason for hiding this comment

nedbat commented Jul 20, 2016

cahrens commented Jul 20, 2016

cahrens commented Jul 20, 2016

cahrens commented Jul 20, 2016

doctoryes commented Jul 25, 2016

cahrens commented Jul 25, 2016

doctoryes commented Jul 26, 2016

pdpinch commented Jul 26, 2016

amir-qayyum-khan commented Jul 26, 2016

amir-qayyum-khan commented Jul 29, 2016 • edited Loading

amir-qayyum-khan commented Jul 29, 2016

doctoryes Jul 29, 2016

Choose a reason for hiding this comment

amir-qayyum-khan Jul 29, 2016

Choose a reason for hiding this comment

doctoryes commented Jul 29, 2016

amir-qayyum-khan commented Jul 29, 2016

amir-qayyum-khan commented Jul 29, 2016

doctoryes commented Jul 29, 2016

amir-qayyum-khan commented Aug 8, 2016

doctoryes commented Aug 8, 2016

amir-qayyum-khan commented Aug 8, 2016

amir-qayyum-khan commented Jul 18, 2016 •

edited

Loading

amir-qayyum-khan commented Jul 29, 2016 •

edited

Loading