Move library creator checks to POST-only by efischer19 · Pull Request #16522 · openedx/openedx-platform

efischer19 · 2017-11-09T18:17:36Z

Getting this started on jenkins in case we decide we want it.

Review will be easiest if you open the new file - I essentially reorganized everything insinde library_handler to treat POST vs GET as the primary logic concern.

efischer19 · 2017-11-09T18:56:14Z

As there are no tests here and we're under a bit of a time crunch, I'm going to do a straight revert of https://github.com/edx/edx-platform/pull/16447 first, then un-revert it here to be merged back after I can add the appropriate tests.

OmarIthawi · 2017-11-11T05:56:57Z

@efischer19 I'll keep this on my radar. Let's see if we can bring this back again. I think it's a nice feature to have.

efischer19 · 2017-11-29T21:12:13Z

I've confirmed this fails without the new commit:

============================= test session starts ============================== platform linux2 -- Python 2.7.12, pytest-3.1.3, py-1.4.34, pluggy-0.4.0 Django settings: cms.envs.test (from ini file) rootdir: /edx/app/edxapp/edx-platform/cms, inifile: pytest.ini plugins: xdist-1.20.0, forked-0.2, django-3.1.2, cov-2.5.1, catchlog-1.2.2, attrib-0.1.3, flaky-3.3.0 collected 30 items cms/djangoapps/contentstore/views/tests/test_library.py ........................F..... ===Flaky Test Report=== ===End Flaky Test Report=== ========================== slowest 20 test durations =========================== 7.34s setup djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_advanced_problem_types 0.72s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_get_lib_edit_html 0.63s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_bad_http_verb_1_delete 0.52s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_advanced_problem_types 0.21s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_get_component_templates 0.19s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_no_duplicate_libraries 0.19s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_bad_http_verb_with_lib_key 0.19s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_manage_library_users 0.18s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_no_access 0.16s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_create_library 0.16s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_lib_create_permission 0.15s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_list_libraries 0.15s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_get_lib_info 0.15s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_lib_create_permission_no_course_creator_role_and_no_course_creator_group 0.14s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_library_creator_status_with_no_course_creator_role_and_disabled_nonstaff_course_creation 0.13s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_invalid_keys_3_course_v1_Org_Course_Run 0.10s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_invalid_keys_4_invalid 0.10s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_lib_create_permission_no_course_creator_role_and_course_creator_group 0.10s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_create_library_invalid_1___ 0.10s call djangoapps/contentstore/views/tests/test_library.py::UnitTestLibraries::test_invalid_keys_1_library_v1_Nonexistent_library =================================== FAILURES =================================== UnitTestLibraries.test_library_creator_status_with_no_course_creator_role_and_disabled_nonstaff_course_creation self = <cms.djangoapps.contentstore.views.tests.test_library.UnitTestLibraries testMe...eator_status_with_no_course_creator_role_and_disabled_nonstaff_course_creation> @mock.patch.dict('django.conf.settings.FEATURES', {'DISABLE_COURSE_CREATION': True}) @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_no_course_creator_role_and_disabled_nonstaff_course_creation(self): """ Ensure that `DISABLE_COURSE_CREATION` feature works with libraries as well. """ nostaff_client, nostaff_user = self.create_non_staff_authed_user_client() self.assertFalse(get_library_creator_status(nostaff_user)) response = nostaff_client.get_json(LIBRARY_REST_URL) > self.assertEqual(response.status_code, 200) E AssertionError: 403 != 200 cms/djangoapps/contentstore/views/tests/test_library.py:76: AssertionError ----------------------------- Captured stderr call ----------------------------- INFO:tracking:{"username": "", "event_source": "server", "name": "edx.user.settings.changed", "accept_language": "", "time": "2017-11-29T21:11:00.048404+00:00", "agent": "", "page": null, "host": "", "session": "", "referer": "", "context": {}, "ip": "", "event": {"user_id": 1, "truncated": [], "old": false, "setting": "is_staff", "table": "auth_user", "new": true}, "event_type": "edx.user.settings.changed"} DEBUG:openedx.core.djangoapps.safe_sessions.middleware:SafeCookieData received empty user_id 'None' for session_id 'sacvago2lwl4o1m0zr013q433gm36gzp'. INFO:audit:Login success - testuser (test+courses@edx.org) ERROR:openedx.core.djangoapps.safe_sessions.middleware:SafeCookieData BWC parse error: 'zza3aw48rvur1dva0kn2s6af4u1it0k9'. INFO:xmodule.modulestore.mixed:Creating course run org.29/course_29/Run_29... INFO:xmodule.modulestore.mixed:Course run org.29/course_29/Run_29 created successfully! DEBUG:openedx.core.djangoapps.safe_sessions.middleware:SafeCookieData received empty user_id 'None' for session_id 't9nj3xz98xhc6osnkhbwe7pg3n1wiyer'. INFO:audit:Login success - testuser (test+courses@edx.org) ERROR:openedx.core.djangoapps.safe_sessions.middleware:SafeCookieData BWC parse error: 'f1kjbx0ngzloyms8n97o38x0haynkqn1'. DEBUG:openedx.core.djangoapps.safe_sessions.middleware:SafeCookieData received empty user_id 'None' for session_id 'mxnqm4kzukgutbariwefe8t5yhu8xdtg'. INFO:audit:Login success - teststudent (test+student@edx.org) ERROR:openedx.core.djangoapps.safe_sessions.middleware:SafeCookieData BWC parse error: 'qhg8lswikpmkbxsoplr6tlqaajp4x9kr'. INFO:tracking:{"username": "teststudent", "event_type": "/library/", "ip": "127.0.0.1", "agent": "", "host": "testserver", "referer": "", "accept_language": "", "event": "{\"POST\": {}, \"GET\": {}}", "event_source": "server", "context": {"user_id": 2, "org_id": "", "course_id": "", "path": "/library/"}, "time": "2017-11-29T21:11:00.103500+00:00", "page": null} ------------------------------ Captured log call ------------------------------- logger.py 42 INFO {"username": "", "event_source": "server", "name": "edx.user.settings.changed", "accept_language": "", "time": "2017-11-29T21:11:00.048404+00:00", "agent": "", "page": null, "host": "", "session": "", "referer": "", "context": {}, "ip": "", "event": {"user_id": 1, "truncated": [], "old": false, "setting": "is_staff", "table": "auth_user", "new": true}, "event_type": "edx.user.settings.changed"} middleware.py 230 DEBUG SafeCookieData received empty user_id 'None' for session_id 'sacvago2lwl4o1m0zr013q433gm36gzp'. models.py 2171 INFO Login success - testuser (test+courses@edx.org) middleware.py 82 ERROR SafeCookieData BWC parse error: 'zza3aw48rvur1dva0kn2s6af4u1it0k9'. mixed.py 657 INFO Creating course run org.29/course_29/Run_29... mixed.py 665 INFO Course run org.29/course_29/Run_29 created successfully! middleware.py 230 DEBUG SafeCookieData received empty user_id 'None' for session_id 't9nj3xz98xhc6osnkhbwe7pg3n1wiyer'. models.py 2171 INFO Login success - testuser (test+courses@edx.org) middleware.py 82 ERROR SafeCookieData BWC parse error: 'f1kjbx0ngzloyms8n97o38x0haynkqn1'. middleware.py 230 DEBUG SafeCookieData received empty user_id 'None' for session_id 'mxnqm4kzukgutbariwefe8t5yhu8xdtg'. models.py 2171 INFO Login success - teststudent (test+student@edx.org) middleware.py 82 ERROR SafeCookieData BWC parse error: 'qhg8lswikpmkbxsoplr6tlqaajp4x9kr'. logger.py 50 INFO {"username": "teststudent", "event_type": "/library/", "ip": "127.0.0.1", "agent": "", "host": "testserver", "referer": "", "accept_language": "", "event": "{\"POST\": {}, \"GET\": {}}", "event_source": "server", "context": {"user_id": 2, "org_id": "", "course_id": "", "path": "/library/"}, "time": "2017-11-29T21:11:00.103500+00:00", "page": null} ===================== 1 failed, 29 passed in 13.34 seconds =====================

efischer19 · 2017-11-29T21:15:50Z

@OmarIthawi I think we should be good to go again here; I added a few lines to the tests in the un-revert commit to ensure we cover the situation that arose last time.

@nasthagiri Could you review this when you get a chance? First commit is undoing the OSPR revert made by https://openedx.atlassian.net/browse/EDUCATOR-1724, second is a fix to the library handler logic to prevent the issue from recurring.

OmarIthawi

Looks good to me. Could you please add a test case (or two) to ensure the if request.method == 'POST': logic won't go in a future refactoring?

OmarIthawi · 2017-11-30T02:44:56Z

Thanks @efischer19 for taking the lead on this PR. I've added a comment.

OmarIthawi · 2017-11-30T02:46:16Z

@antoviaque this another attempt to get my library button PR #16447.

nasthagiri · 2017-11-30T14:27:26Z

Nit: this check isn't needed here since it's checked within get_library_creator_status.

efischer19 · 2017-11-30T14:34:22Z

@OmarIthawi I've attempted to make these tests more explicit here

I didn't see that, thanks @efischer19! Looks great then!

efischer19 · 2017-11-30T17:45:06Z

jenkins run lettuce

…ibrary-button"" This reverts commit 7696727.

efischer19 · 2017-12-04T18:13:22Z

jenkins run a11y

edx-pipeline-bot · 2017-12-04T20:38:25Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, December 05, 2017.

edx-pipeline-bot · 2017-12-05T19:27:46Z

EdX Release Notice: This PR has been deployed to the production environment.

efischer19 mentioned this pull request Nov 9, 2017

Hide the create library for non-staff users when DISABLE_COURSE_CREATION #16447

Merged

efischer19 mentioned this pull request Nov 9, 2017

Revert "Merge pull request #16447 from appsembler/omar/hide-library-b… #16524

Merged

efischer19 force-pushed the efischer/EDU-1724 branch from 00246cb to 9aa0ea7 Compare November 29, 2017 21:11

efischer19 commented Nov 29, 2017

View reviewed changes

efischer19 requested a review from nasthagiri November 29, 2017 21:15

OmarIthawi approved these changes Nov 30, 2017

View reviewed changes

nasthagiri approved these changes Nov 30, 2017

View reviewed changes

efischer19 force-pushed the efischer/EDU-1724 branch from 9aa0ea7 to 42f600e Compare November 30, 2017 14:33

efischer19 commented Nov 30, 2017

View reviewed changes

efischer19 force-pushed the efischer/EDU-1724 branch 2 times, most recently from 95c0266 to 5bda6df Compare November 30, 2017 17:00

Eric Fischer added 2 commits December 4, 2017 11:55

Revert "Revert "Merge pull request #16447 from appsembler/omar/hide-l…

cb463c0

…ibrary-button"" This reverts commit 7696727.

Move library creator checks to POST-only

a098666

efischer19 force-pushed the efischer/EDU-1724 branch from 5bda6df to a098666 Compare December 4, 2017 16:55

efischer19 merged commit fae1537 into master Dec 4, 2017

efischer19 deleted the efischer/EDU-1724 branch December 4, 2017 19:17

This was referenced Dec 7, 2018

[WIP] Fix content libraries for non-staff users stvstnfrd/edx-platform#41

Closed

Fix content libraries for non-staff users Stanford-Online/edx-platform#849

Merged

Uh oh!

Conversation

efischer19 commented Nov 9, 2017

Uh oh!

efischer19 commented Nov 9, 2017

Uh oh!

OmarIthawi commented Nov 11, 2017

Uh oh!

efischer19 Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

efischer19 commented Nov 29, 2017

Uh oh!

OmarIthawi left a comment

Choose a reason for hiding this comment

Uh oh!

OmarIthawi commented Nov 30, 2017

Uh oh!

OmarIthawi commented Nov 30, 2017

Uh oh!

nasthagiri Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

efischer19 Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

OmarIthawi Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efischer19 commented Nov 30, 2017

Uh oh!

efischer19 commented Dec 4, 2017

Uh oh!

edx-pipeline-bot commented Dec 4, 2017

Uh oh!

edx-pipeline-bot commented Dec 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OmarIthawi Nov 30, 2017 •

edited

Loading