Conversation
lms/djangoapps/course_wiki/utils.py
Outdated
There was a problem hiding this comment.
Make this more clear that it's hypothetical
|
Your coverage report has some missing lines: https://jenkins.testeng.edx.org/job/edx-platform-report/383/Diff_Coverage_Report/? Additionally, please rebase to squash commits as necessary, and add the JIRA ticket number to the commit message itself. Once comments are addressed please ping me, I'm happy to re-review to give thumbs up. |
|
I ran a new build on your branch. If you click the "Details" link, you can see the reports. Please review the diff-coverage and diff-quality reports, which you can see on the left-hand side of the page. Please especially review the diff-quality report. You introduce a ton of quality violations that need to be cleaned up. Also be sure you're familiar with https://github.com/edx/edx-platform/wiki/Python-Guidelines |
|
@sarina Thanks! Let me look at all of this. |
There was a problem hiding this comment.
@sarina You commented on this section earlier suggesting this:
if course_slug is None:
return False
if (course_slug.endswith('_') and course_wiki_slug_is_numerical(course_slug[:-1]):
course_slug = course_slug[:-1]
if user_is_staff_on_course_number(user, course_slug):
return True
return False
Consider the course with course number 202_. Its instructor group will be named something like "instructor_mitx/202_/fall" and its course slug will be "202_". Since this ends with an underscore and 202 is a number course_slug will be set to "202" and so the above simplification will not work. Hence the need for the two checks.
However I have refactored this a bit so that there is only one db access.
There was a problem hiding this comment.
OK, great. And thanks for the comment explaining why you need to call user_is_staff_on_course_number twice - that's the part I had the largest concern about.
|
@sarina I have cleaned up all the pylint/pep8 issues and updated according to your suggestions. Can you look at this again? |
There was a problem hiding this comment.
instead of pylint: disable=C0103 you can use the name of the error message, which is clearer. So change these to instead, pylint: disable=invalid-name
|
Reports and refactored code look good. I just had a few more comments, once addressed this looks good to me. |
|
@sarina Refactored tests and fixed the other issues. P.S. With your stringent standards you should be made the chief reviewer for all PRs. :) |
|
@symbolist but now you will be chief awesomest code writer 😁 |
|
Once tests pass, 👍 |
|
👍 2 |
Stricter permissions for the wiki
3 participants
https://edx-wiki.atlassian.net/browse/LMS-1355
CAN_DELETE: Whether the user should be able to (soft) delete an article.
CAN_MODERATE: Actually 'Delete' only hides articles. This setting can allow a user to view 'deleted' articles and purge or restore them. This seems to be turned off for everyone right now so no-one can restore 'deleted' articles.
Every article can be assigned a django user group.
CAN_CHANGE_PERMISSIONS: Whether the user has the ability to change permissions on an article. If allowed the user can make the article unreadable or unwritable to anyone who is not in the article user group.
CAN_ASSIGN: Whether the user can change the owner/user group of the article. This also controls whether the user can lock the article from any edits.
For all four settings the new defaults are:
@cpennington adam says you know most about the wiki so can you look at this?
@adampalay