Sjang92/advanced settings feedback

[sjang92]

@jrbl @njdup

There were no validations being done in Studio's course advanced settings tab.
Now course instructors receive feed back as a list of settings changes that has formatting/value errors.

[jrbl]

When you're through your feedback, you'll need to squash and recommit. When you do, the commit message needs some editing:

Implemented Validation for Course Advanced Setting

Insert a blank line after the first line.

This commit adds validation for course advanced settings.
When course administrators make invalid changes in the
Settings/Advanced Settings tab, they're not notified through a new modal indow
of the list of invalid settings changes.

Are these two thoughts, or one? If they're part of the same thought, eliminate the internal linebreak. If they're separate, insert an extra linebreak. Also, this sentence saying that instructors aren't notified is unclear to me: are you saying that before this commit, they weren't being notified, or are you saying that as of this commit, they are no longer being notified? This could use a little clarification. Also, window should have a w.

* Extending CourseMetadata
    - Previously, we only had update_from_json method in CourseMetadata.py,
      and it was only validating one field every POST request.
    - Now we have validate_and_update_from_json method that encapsulates the
      functionality of update_from_json into a validation call
    - To avoid discrepency of validation standards between modules, validation
      uses the from_json method implemented to each field in xblock.

This is good. I like it. 'discrepancy' has an 'a' in the middle.

* Different Response in advanced settings ajax requests
    - course.py, upon receiving a POST ajax request, now makes a call to
      validate_and_update_from_json, and sends a json object of either:
        1) valid course metadata model
        2) error objects

Comma splices are controversial. Perhaps you could just say, "POST requests to course.py call to..."?

[jrbl]

There's a UI component here for error reporting that @caesar2164 should look at.

[jrbl]

In these two lines, you refer to request.json[ADVANCED_COMPONENT_POLICY_KEY]["value"] twice; you should probably memoize it, like:

# Check if the user has incorrectl failed to put the value in an iterable.
policy_value = request.json[ADVANCED_COMPONENT_POLICY_KEY]["value"]
if isinstance(policy_key, collections.Iterable):
    if ac_type in policy_key:

[jrbl]

But I'd rather you not use isinstance() if you don't have to.

http://stackoverflow.com/questions/1952464/in-python-how-do-i-determine-if-an-object-is-iterable has some alternatives, including using hasattr() to look for the iter and getitem magic methods, calling iter() on it and catching the exception, or just assuming it's iterable and catching the exception.

If this check isn't here and the thing returned isn't a container type, what happens?

[caesar2164]

UI wise this is good! So that's good, but tests are failing :(

[jrbl]

If you're indenting the code below you should indent the comment too.

[jrbl]

Oh, nice screenshot.

[jrbl]

As I mentioned on @njdup's last PR, you can find a lot of nitpicky stuff by logging into your devstack box, going to the edx-platform directory and running:
$ for file in git diff --name-only HEAD~2 | grep '.py$'; do pep8 $file; done
You're doing pretty well, it only turns up:

cms/djangoapps/contentstore/tests/test_course_settings.py:12:80: E501 line too long (81 characters)
cms/djangoapps/contentstore/tests/test_course_settings.py:28:1: E302 expected 2 blank lines, found 1
cms/djangoapps/contentstore/tests/test_course_settings.py:143:5: E303 too many blank lines (2)
cms/djangoapps/contentstore/tests/test_course_settings.py:460:1: W293 blank line contains whitespace
cms/djangoapps/contentstore/tests/test_course_settings.py:501:101: W291 trailing whitespace
cms/djangoapps/contentstore/views/course.py:26:80: E501 line too long (82 characters)
cms/djangoapps/contentstore/views/course.py:666:1: W293 blank line contains whitespace
cms/djangoapps/contentstore/views/course.py:667:29: W291 trailing whitespace
cms/djangoapps/models/settings/course_metadata.py:5:1: E302 expected 2 blank lines, found 1
cms/djangoapps/models/settings/course_metadata.py:58:80: E501 line too long (81 characters)
cms/djangoapps/models/settings/course_metadata.py:114:1: W293 blank line contains whitespace
cms/djangoapps/models/settings/course_metadata.py:123:41: E231 missing whitespace after ':'
cms/djangoapps/models/settings/course_metadata.py:132:5: E303 too many blank lines (2)

[jrbl]

Python tests are failing for me, but that may be my devstack box. But I notice the Jenkins build failed too.

[jrbl]

Way to go controlling the whitespace.

[nedbat]

@cahrens or @andy-armstrong ?

[cahrens]

@nedbat Studio review of this PR will need to be scheduled-- it's not something we can pick up right away. I will talk to our product owner about it.

https://openedx.atlassian.net/browse/STUD-1995

[sjang92]

Reflected Joe's comments & tests all passing now

[jinpa]

@cahrens just in case this is useful info for scheduling, @sjang92 is a
summer intern, and his last day is Aug 29, so useful to get any needed
changes enough ahead of that so that he can merge before he finishes up.

On Thu, Jul 17, 2014 at 8:22 AM, Christina Roberts <notifications@github.com

wrote:

@nedbat https://github.com/nedbat Studio review of this PR will need to
be scheduled-- it's not something we can pick up right away. I will talk to
our product owner about it.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/4454#issuecomment-49321628.

[jrbl]

I'd put a newline above this line.

[jrbl]

Nitpick on the commit message: 'discrepancy' still has an 'a' in the middle. :-)

[jrbl]

Additional nitpick on commit message: last stanza still contains a comma splice that I find somewhat strange. Rewriting the sentence that begins "course.py, upon receiving a POST ajax request," so that is either in the form 'X done to Y causes Z' or the form 'Z when X done to Y' would probably help it be clearer.

[sjang92]

@cahrens jshint conforming and suggested fixes are in place now.

[andy-armstrong]

Hi @sjang92. I'm taking over this review for Christina as she's on vacation this week. I'm going to try it out now, but I was wondering about the test failure. Is this a problem that you know about and that you can fix, or do you need help? I'd like to get a clean build so I can verify the code quality and coverage reports.

[andy-armstrong]

@sjang92 I've verified the changes and they look great to me. This is an excellent improvement over what we had.

One minor thing that I noticed: the validation of JSON inputs appears to be incomplete. It correctly rejects values of different types, e.g. a number, but it silently rejects parseable but invalid JSON strings. For example, I edited the "Cohort Configuration" field and it silently rejected the string "{ 1 2 3 }". Fixing this probably belongs in a separate PR, but if it is straightforward to fix that would be fantastic.

Once there's a clean build and the coverage looks good then I'll go ahead and merge this.

[sjang92]

@andy-armstrong Could you tell me more about the invalid json string input? Is it supposed to parse it and take it as a valid input?

[sjang92]

I just tried feeding a number of pareseable inputs to List and Dict type fields (ex. Advanced Modules List) and they seem to be parsing correctly before saving. So no Validation error is seen in these cases. Cohort Configuration does fail though. Maybe it doesn't go through parsing? (It says Cohort Configuration is not supported by Edx? )

[andy-armstrong]

@sjang92 Another example is entering [1 2 3] for advanced modules. When you save, it says "Your policy changes have been saved" but yet the field reverts back to []. I think the difference is in whether the value is parseable as the type, versus whether the parsed value is actually valid.

As I said, this is probably out of scope for this PR because I know we need to get it merged. It is just an interesting validation problem.

[sjang92]

Hmm once I get a clean build and get this merged, I'll look into it!

[andy-armstrong]

Sounds great, thanks!

[sjang92]

@andy-armstrong All tests passing :)

[andy-armstrong]

Congrats @sjang92! The build reports show a couple of issues that should be addressed, and then we should be good to go:

1. It looks like there are no tests for JsonResponseBadRequest:

https://jenkins.testeng.edx.org/job/edx-platform-report/6103/Diff_Coverage_Report/

1. There are a few diff quality and PyLint issues (check both tabs):

https://jenkins.testeng.edx.org/job/edx-platform-report/6103/Diff_Quality_Report/

[sjang92]

@andy-armstrong Tests in for JsonResponseBadRequest. For PyLint, I left the incomplete-protocol one. I'm not quite sure if I have to touch that?

[andy-armstrong]

@sjang92 Thanks, that's looking a lot better. I'm okay with ignoring the incomplete-prototocol warning as there are a lot of discussions on the internet about how unreasonable that warning is.

However, can you fix the missing docstring reported by PyLint, and it would also be preferable if your tests caused advanced_settings_handler to return a JsonResponseBadRequest to get coverage on that line too. Wherever possible we prefer to have 100% diff coverage and right now course.py is only at 86.7%

https://jenkins.testeng.edx.org/job/edx-platform-report/6111/Diff_Coverage_Report/

[sjang92]

@andy-armstrong I fixed the coverage and pylint issues. I've been running jenkins to get a clean build but it's one of those flakey tests that keep failing.. Should I keep re-running it?

[andy-armstrong]

@sjang92 Thanks for all the hard work on this. I've merged the PR now.

[sjang92]

@andy-armstrong Thank You!