Wikicleanup #510

Closed
wants to merge 64 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

andre-d commented Aug 27, 2012

For review

Owner

spladug commented Aug 27, 2012

I think we can squash 824c303 and 6bb179e together -- I don't think @bsimpson63 will mind (right? :).

(and 65aca04)

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/example.ini
+
+# Max number of bytes for wiki pages
+wiki_max_page_length_bytes = 262144
+
+# Number of seconds to ratelimit between wiki edits
+wiki_edit_ratelimit_seconds = 60
+
+# Max wiki page name length
+wiki_max_page_name_length = 128
+
+# Max number of seperators in a wiki page name
+wiki_max_page_seperators = 3
+
+# Location (directory) for temp files for diff3 merging
+# Empty will use python default for temp files
+diff3_temp_location = /dev/shm
@kemitche

kemitche Aug 29, 2012

Contributor

Since this is OS specific, and python has a default anyway, I'd say it should be left empty in example.ini (maybe recommend /dev/shm for Ubuntu in the comments)

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -35,9 +35,10 @@
from r2.lib.utils import get_title, sanitize_url, timeuntil, set_last_modified
from r2.lib.utils import query_string, timefromnow, randstr
from r2.lib.utils import timeago, tup, filter_links
-from r2.lib.pages import EnemyList, FriendList, ContributorList, ModList, \
- BannedList, BoringPage, FormPage, CssError, UploadedImage, ClickGadget, \
- UrlParser, WrappedUser, BoringPage
+from r2.lib.pages import (EnemyList, FriendList, ContributorList, ModList,
+ BannedList, WikiBannedList, WikiMayContributeList,
@kemitche

kemitche Aug 29, 2012

Contributor

pep8 nit: the lines should be aligned with the opening parentheses, like so:

from sqlalchemy import (Column, String, DateTime, Date, Float, Integer,
                        Boolean, BigInteger, func as safunc, and_, or_, not_)

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/wiki.py
+from r2.lib.pages.things import default_thing_wrapper
+from r2.lib.pages import BoringPage
+from reddit_base import base_listing
+from r2.models import IDBuilder, LinkListing, DefaultSR
+from validator.validator import VInt, VExistingUname, VRatelimit
+from r2.lib.merge import ConflictException, make_htmldiff
+from pylons.i18n import _
+from r2.lib.pages import PaneStack
+from r2.lib.utils import timesince
+
+import json
+
+page_descriptions = {'config/stylesheet':_("This page is the subreddit stylesheet, changes here apply to the subreddit css"),
+ 'config/sidebar':_("The contents of this page appear on the subreddit sidebar")}
+
+wiki_modactions = {'config/sidebar': "Updated subreddit sidebar"}
@kemitche

kemitche Aug 29, 2012

Contributor

wiki... wiki everywhere! Style note: Part of the purpose of namespaces on imports in python is so that you don't have to prepend every variable name with "wiki" - you can do things like call this variable "modactions" and then do from r2.controllers import wiki so you can reference things as wiki.modactions 😉

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -61,6 +63,10 @@
from r2.controllers.api_docs import api_doc, api_section
from r2.lib.cloudsearch import basic_query
+from r2.models.wiki import WikiPage
+from r2.controllers.wiki import wiki_modactions
@kemitche

kemitche Aug 29, 2012

Contributor

Since wiki_modactions needs to be imported, maybe it belongs in r2.models.wiki? I think we try to avoid importing controllers in other places.

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -521,7 +528,8 @@ def POST_unfriend(self, nuser, iuser, container, type):
# Log this action
if new and type in sr_types:
- action = dict(banned='unbanuser', moderator='removemoderator',
+ action = dict(banned='unbanuser', wikicontributor='removewikicontributor',
@kemitche

kemitche Aug 29, 2012

Contributor

At a glance, this line seems > 80 chars?

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -1179,20 +1189,17 @@ def POST_vote(self, dir, thing, ip, vote_type):
VModhash(),
# nop is safe: handled after auth checks below
stylesheet_contents = nop('stylesheet_contents'),
+ prevstyle = VLength('prevstyle', max_length = 256),
@kemitche

kemitche Aug 29, 2012

Contributor

pep8: no spaces 'round keyword args

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -601,7 +610,8 @@ def POST_friend(self, form, jquery, ip, friend,
cls = dict(friend=FriendList,
moderator=ModList,
contributor=ContributorList,
- banned=BannedList).get(type)
+ wikicontributor=WikiMayContributeList,
+ banned=BannedList,wikibanned=WikiBannedList).get(type)
@kemitche

kemitche Aug 29, 2012

Contributor

need a space after that comma!

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
form.set_html(".errors ul", '')
- stylesheet_contents_parsed = parsed.cssText if parsed else ''
- # if the css parsed, we're going to apply it (both preview & save)
- jquery.apply_stylesheet(stylesheet_contents_parsed)
+ stylesheet_contents_parsed = parsed if parsed else ''
@kemitche

kemitche Aug 29, 2012

Contributor

nit: can be just stylesheet_contents_parsed = parsed or ''

@kemitche kemitche commented on the diff Aug 29, 2012

r2/r2/controllers/api.py
+ c.site.change_css(stylesheet_contents, parsed, prevstyle)
+ form.find('.conflict_box').hide()
+ form.find('.errors').hide()
+ form.set_html(".status", _('saved'))
+ form.set_html(".errors ul", "")
+ # ModAction.create(c.site, c.user, 'wikirevise', description=wiki_modactions.get('config/stylesheet'))
+ except ConflictException as e:
+ form.set_html(".status", _('conflict error'))
+ form.set_html(".errors ul", _('There was a conflict while editing the stylesheet'))
+ form.find('#conflict_box').show()
+ form.set_inputs(conflict_old = e.your, prevstyle=e.new_id, stylesheet_contents = e.new)
+ form.set_html('#conflict_diff', e.htmldiff)
+ form.find('.errors').show()
+ return
+ jquery.apply_stylesheet(stylesheet_contents_parsed)
+ if op == 'preview':
@kemitche

kemitche Aug 29, 2012

Contributor

Any reason this changed from elif op == 'preview' to just if?

@andre-d

andre-d Aug 29, 2012

Contributor

We want to jquery.apply_stylesheet if there is no conflict error from save mode or if the mode is preview.

@kemitche

kemitche Aug 30, 2012

Contributor

Ah. Nearly missed that tucked in there.

@kemitche kemitche and 1 other commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
-
- ModAction.create(c.site, c.user, action='editsettings',
- details='stylesheet')
-
- form.set_html(".status", _('saved'))
- form.set_html(".errors ul", "")
-
- elif op == 'preview':
+ c.site.stylesheet_contents = stylesheet_contents_parsed
+ try:
+ c.site.change_css(stylesheet_contents, parsed, prevstyle)
+ form.find('.conflict_box').hide()
+ form.find('.errors').hide()
+ form.set_html(".status", _('saved'))
+ form.set_html(".errors ul", "")
+ # ModAction.create(c.site, c.user, 'wikirevise', description=wiki_modactions.get('config/stylesheet'))
@kemitche

kemitche Aug 29, 2012

Contributor

This should be uncommented or deleted

@andre-d

andre-d Aug 29, 2012

Contributor

Oh yeah, those all need to be uncommented, they were commented out for labs

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
- form.set_html(".errors ul", "")
-
- elif op == 'preview':
+ c.site.stylesheet_contents = stylesheet_contents_parsed
+ try:
+ c.site.change_css(stylesheet_contents, parsed, prevstyle)
+ form.find('.conflict_box').hide()
+ form.find('.errors').hide()
+ form.set_html(".status", _('saved'))
+ form.set_html(".errors ul", "")
+ # ModAction.create(c.site, c.user, 'wikirevise', description=wiki_modactions.get('config/stylesheet'))
+ except ConflictException as e:
+ form.set_html(".status", _('conflict error'))
+ form.set_html(".errors ul", _('There was a conflict while editing the stylesheet'))
+ form.find('#conflict_box').show()
+ form.set_inputs(conflict_old = e.your, prevstyle=e.new_id, stylesheet_contents = e.new)
@kemitche

kemitche Aug 29, 2012

Contributor

pep8, spaces, kwargs

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -1412,16 +1423,45 @@ def POST_upload_sr_img(self, file, header, sponsor, name, form_id, img_type):
@api_doc(api_section.subreddits)
def POST_site_admin(self, form, jquery, name, ip, sr,
sponsor_text, sponsor_url, sponsor_name, **kw):
+
+ def apply_wikid_field(sr, form, pagename, value, prev, field, error):
+ try:
+ wiki = WikiPage.get(sr.name, pagename)
+ except tdb_cassandra.NotFound:
+ wiki = WikiPage.create(sr.name, pagename)
+ try:
+ if wiki.revise(value, previous=prev, author=c.user.name):
+ pass
+ # ModAction.create(c.site, c.user, 'wikirevise', details=wiki_modactions.get(pagename))
@kemitche

kemitche Aug 29, 2012

Contributor

uncomment or delete?

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/api.py
@@ -1412,16 +1423,45 @@ def POST_upload_sr_img(self, file, header, sponsor, name, form_id, img_type):
@api_doc(api_section.subreddits)
def POST_site_admin(self, form, jquery, name, ip, sr,
sponsor_text, sponsor_url, sponsor_name, **kw):
+
+ def apply_wikid_field(sr, form, pagename, value, prev, field, error):
+ try:
+ wiki = WikiPage.get(sr.name, pagename)
+ except tdb_cassandra.NotFound:
+ wiki = WikiPage.create(sr.name, pagename)
+ try:
+ if wiki.revise(value, previous=prev, author=c.user.name):
@kemitche

kemitche Aug 29, 2012

Contributor

Presumably, this works as an if clause better when the next line about ModAction is uncommented?

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/error.py
@@ -164,7 +164,10 @@ def GET_document(self):
c.response.content = str(code)
return c.response
elif c.render_style == "api":
- c.response.content = "{\"error\": %s}" % code
+ if 'usable_error_content' in request.environ:
@kemitche

kemitche Aug 29, 2012

Contributor

c.response.content = request.environ.get('usable_error_content', "{\"error\": %s}" % code)

@kemitche kemitche and 1 other commented on an outdated diff Aug 29, 2012

r2/r2/models/account.py
@@ -173,12 +173,9 @@ def safe_karma(self):
return max(karma, 1) if karma > -1000 else karma
def can_wiki(self):
- if self.wiki_override is not None:
- return self.wiki_override
- else:
- return (self.link_karma >= g.WIKI_KARMA and
- self.comment_karma >= g.WIKI_KARMA)
-
+ return self.wiki_override if self.wiki_override is False else True
@kemitche

kemitche Aug 29, 2012

Contributor

Why not just return self.wiki_override (or, if it must be a bool, return bool(self.wiki_override)?

@andre-d

andre-d Aug 29, 2012

Contributor

because unset (None) means the user can wiki, only False makes it so the user cannot wiki.

@kemitche

kemitche Aug 30, 2012

Contributor

That's a bit of an unintuitive tri-state; could you please revert the structure to something similar to the original, like the following, so it's clear that the None is a key factor in the logic:

if self.wiki_override is None:
    return True
else:
    return self.wiki_override

(or consider refactoring)

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/validator/wiki.py
+# with Exhibit B.
+#
+# Software distributed under the License is distributed on an "AS IS" basis,
+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License for
+# the specific language governing rights and limitations under the License.
+#
+# The Original Code is reddit.
+#
+# The Original Developer is the Initial Developer. The Initial Developer of
+# the Original Code is reddit Inc.
+#
+# All portions of the code written by reddit are Copyright (c) 2006-2012 reddit
+# Inc. All Rights Reserved.
+###############################################################################
+
+from validator import Validator
@kemitche

kemitche Aug 29, 2012

Contributor

Please be explicit with the imports (i.e., from r2.controllers.validator import Validator)

@kemitche kemitche commented on the diff Aug 29, 2012

r2/r2/controllers/validator/wiki.py
+# software over a computer network and provide for limited attribution for the
+# Original Developer. In addition, Exhibit A has been modified to be consistent
+# with Exhibit B.
+#
+# Software distributed under the License is distributed on an "AS IS" basis,
+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License for
+# the specific language governing rights and limitations under the License.
+#
+# The Original Code is reddit.
+#
+# The Original Developer is the Initial Developer. The Initial Developer of
+# the Original Code is reddit Inc.
+#
+# All portions of the code written by reddit are Copyright (c) 2006-2012 reddit
+# Inc. All Rights Reserved.
+###############################################################################
@kemitche

kemitche Aug 29, 2012

Contributor

imports should be organized per pep8: http://www.python.org/dev/peps/pep-0008/#imports

In order, separated by one blank line:

  1. standard library imports
  2. related third party imports
  3. local application/library specific imports

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/validator/wiki.py
+# All portions of the code written by reddit are Copyright (c) 2006-2012 reddit
+# Inc. All Rights Reserved.
+###############################################################################
+
+from validator import Validator
+from pylons import c, g, request
+from os.path import normpath
+from r2.lib.db import tdb_cassandra
+from r2.models.wiki import WikiPage, WikiRevision
+from pylons.controllers.util import abort, redirect_to
+import datetime
+import json
+
+MAX_PAGE_NAME_LENGTH = g.wiki_max_page_name_length
+
+MAX_SEPERATORS = g.wiki_max_page_seperators
@kemitche

kemitche Aug 29, 2012

Contributor

"separators" has 2 A's and 1 E, please

@kemitche kemitche commented on an outdated diff Aug 29, 2012

r2/r2/controllers/validator/wiki.py
+###############################################################################
+
+from validator import Validator
+from pylons import c, g, request
+from os.path import normpath
+from r2.lib.db import tdb_cassandra
+from r2.models.wiki import WikiPage, WikiRevision
+from pylons.controllers.util import abort, redirect_to
+import datetime
+import json
+
+MAX_PAGE_NAME_LENGTH = g.wiki_max_page_name_length
+
+MAX_SEPERATORS = g.wiki_max_page_seperators
+
+def jsonAbort(code, reason=None, **data):
@kemitche

kemitche Aug 29, 2012

Contributor

This looks like a function that would belong in r2.lib.utils

@kemitche kemitche and 1 other commented on an outdated diff Aug 29, 2012

r2/r2/controllers/validator/wiki.py
+
+MAX_SEPERATORS = g.wiki_max_page_seperators
+
+def jsonAbort(code, reason=None, **data):
+ data['code'] = code
+ data['reason'] = reason if reason else 'UNKNOWN_ERROR'
+ if c.extension == 'json':
+ request.environ['usable_error_content'] = json.dumps(data)
+ abort(code)
+
+def may_revise(page=None):
+ if not c.user_is_loggedin:
+ # Users who are not logged in may not contribute
+ return False
+
+ if c.is_mod:
@kemitche

kemitche Aug 29, 2012

Contributor

If I'm a mod of any subreddit, I can always contribute to all wiki pages?

@andre-d

andre-d Aug 29, 2012

Contributor

should likely be c.is_wiki_mod or something

@kemitche

kemitche Aug 29, 2012

Contributor

That'd help. Still, the function mixes contexts and makes use of globals in a way that could be trouble down the line. Is there any reason not to check here for c.site.is_moderator(c.user) or some such? Or better, refactor to take user and subreddit as arguments, then pass them in, avoiding the use of c in here? That'd also make the function more reusable.

@andre-d

andre-d Sep 4, 2012

Contributor

todo done

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/createsubreddit.html
@@ -72,6 +72,11 @@
%else:
${UserText(None, text="", creating=True, name="public_description", have_form=False)}
%endif
+ <div id="public_description_conflict_box" style="display: none">
+ <div id="public_description_conflict_diff"></div>
+ ${UserText(None, text ="", editable = True, creating = True, name="public_description_conflict_old", have_form = False)}
@chromakode

chromakode Aug 30, 2012

Contributor

Could you please remove the whitespace around the = here?

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/createsubreddit.html
@@ -81,7 +86,18 @@
%else:
${UserText(None, text = "", creating = True, name="description", have_form = False)}
%endif
+ <div id="description_conflict_box" style="display: none">
+ <div id="description_conflict_diff"></div>
+ ${UserText(None, text ="", editable = True, creating = True, name="description_conflict_old", have_form = False)}
@chromakode

chromakode Aug 30, 2012

Contributor

Please remove the whitespace around the = here too.

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikipagesettings.html
+ <h3 class="error" style="display:none" id="usereditallowerror">${_('username does not exist')}</h2>
+ </form>
+ <br/>
+ <ul>
+ %for user in thing.mayedit:
+ <li>
+ ${user}
+ &mdash;&nbsp;
+ ${ynbutton(_("(remove)"), _("done"), "../r/%s/wiki/api/alloweditor/del/%s/%s" % (c.site.name, user, c.page), callback="ReloadPage")}
+ </li>
+ %endfor
+ </ul>
+ </%utils:line_field>
+ %endif
+
+ <input type="submit" onclick="$('#pagesettings').submit();" value="${_('save settings')}" />
@chromakode

chromakode Aug 30, 2012

Contributor

No need for a semicolon in the onclick.

@chromakode chromakode and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/templates/createsubreddit.html
+ _("Wiki is disabled for all users except mods"),
+ (not thing.site or thing.site.wikimode == 'disabled'))}
+ ${utils.radio_type('wikimode', "modonly", _("mod editing"),
+ _("Only mods or those on a pages edit list may edit"),
+ (thing.site and thing.site.wikimode == 'modonly'))}
+ ${utils.radio_type('wikimode', "anyone", _("anyone"),
+ _("Anyone who can submit to the subreddit may edit"),
+ (thing.site and thing.site.wikimode == 'anyone'))}
+ </table>
+ </div>
+ <div class="usertext-edit">
+ <div class="delete-field">
+ <label for="wiki_edit_karma">${_('Subreddit karma required to edit and create wiki pages:')}</label>
+ %if thing.site:
+ <input id="wiki_edit_karma" type="text" name="wiki_edit_karma"
+ value = "${getattr(thing.site, 'wiki_edit_karma', 100)}"/>
@chromakode

chromakode Aug 30, 2012

Contributor

Should this default exist in Subreddit rather than the template?

@andre-d

andre-d Sep 3, 2012

Contributor

It actually does...removing

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/createsubreddit.html
+ </div>
+ <div class="usertext-edit">
+ <div class="delete-field">
+ <label for="wiki_edit_karma">${_('Subreddit karma required to edit and create wiki pages:')}</label>
+ %if thing.site:
+ <input id="wiki_edit_karma" type="text" name="wiki_edit_karma"
+ value = "${getattr(thing.site, 'wiki_edit_karma', 100)}"/>
+ %else:
+ <input id="wiki_edit_karma" type="text" name="wiki_edit_karma" value="100" />
+ %endif
+ </div>
+ <div class="delete-field">
+ <label for="wiki_edit_age">${_('Account age (days) required to edit and create wiki pages:')}</label>
+ %if thing.site:
+ <input id="wiki_edit_age" type="text" name="wiki_edit_age"
+ value = "${getattr(thing.site, 'wiki_edit_age', 0)}"/>
@chromakode

chromakode Aug 30, 2012

Contributor

Same for this default.

Contributor

chromakode commented Aug 30, 2012

Is there a specific version of snudown required for this? Should it be specified in setup.py?

Owner

spladug commented Aug 30, 2012

Good point on snudown. I'm going to call the version that'll have the wiki stuff v1.1.0.

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/controllers/validator/wiki.py
+ return True
+
+ if page and page.restricted and not page.special:
+ # People may not contribute to restricted pages
+ # (Except for special pages)
+ return False
+
+ if c.site.is_wikibanned(c.user):
+ # Users who are wiki banned in the subreddit may not contribute
+ return False
+
+ if page and not may_view(page):
+ # Users who are not allowed to view the page may not contribute to the page
+ return False
+
+ if not c.user.can_wiki():
@kemitche

kemitche Aug 30, 2012

Contributor

Would it be possible to reorganize this function a bit so that the more "global" checks happen first?

@andre-d

andre-d Sep 3, 2012

Contributor

No as some of the more global checks are overridden by some of the more local checks, this is on purpose.

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/validator/wiki.py
+ return c.is_mod
+
+ # In any other obscure level,
+ # (This should not happen but just in case)
+ # nobody may view.
+ return False
+
+def normalize_page(page):
+ # Case insensitive page names
+ page = page.lower()
+
+ # Normalize path
+ page = normpath(page)
+
+ # Chop off initial "/", just in case it exists
+ page = page[1:] if page.startswith('/') else page
@kemitche

kemitche Aug 30, 2012

Contributor

page = page.lstrip('/')

@kemitche kemitche commented on the diff Aug 30, 2012

r2/r2/controllers/validator/wiki.py
+ return True
+
+ if level == 2:
+ # Only mods may view in level 2
+ return c.is_mod
+
+ # In any other obscure level,
+ # (This should not happen but just in case)
+ # nobody may view.
+ return False
+
+def normalize_page(page):
+ # Case insensitive page names
+ page = page.lower()
+
+ # Normalize path
@kemitche

kemitche Aug 30, 2012

Contributor

Yes, normpath normalizes the path.

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/controllers/validator/wiki.py
+
+class VWikiPage(Validator):
+ def __init__(self, param, restricted=True, modonly=False, **kw):
+ self.restricted = restricted
+ self.modonly = modonly
+ Validator.__init__(self, param, **kw)
+
+ def run(self, page):
+ if not page:
+ # If no page is specified, give the index page
+ page = "index"
+
+ try:
+ page = str(page)
+ except UnicodeEncodeError:
+ jsonAbort(403, 'INVALID_PAGE_NAME')
@kemitche

kemitche Aug 30, 2012

Contributor

403 seems like the wrong error here. Flat 400 perhaps.

Unrelated, but did you consider returning 409 Edit Conflict for wiki edit conflicts?

@andre-d

andre-d Aug 30, 2012

Contributor

re: 409..I did

Contributor

chromakode commented Aug 30, 2012

I made some style tweaks and reorganized the functions into a namespace here: andre-d#70

Contributor

chromakode commented Aug 30, 2012

Could you add a body class called wiki-page for wiki pages in Reddit.page_classes or a subclass thereof?

Contributor

chromakode commented Aug 30, 2012

Looking at the DOM structure of current wiki pages, there's a .content div wrapping an (unused?) share form and another .content div. Inside that .content div, there is finally a #content div with class .wiki. Inside that is a .wikipage div, which contains two empty <p> elements and finally the .md.wiki page content.

Could you please try to simplify this DOM nesting? One top-level .content div and as few wrappers around the .md.wiki content would be preferable.

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikieditpage.html
+## the specific language governing rights and limitations under the License.
+##
+## The Original Code is reddit.
+##
+## The Original Developer is the Initial Developer. The Initial Developer of
+## the Original Code is reddit Inc.
+##
+## All portions of the code written by reddit are Copyright (c) 2006-2012
+## reddit Inc. All Rights Reserved.
+###############################################################################
+
+<%!
+ from r2.lib.filters import keep_space
+%>
+
+<div style="display: none;" id="conflict">
@chromakode

chromakode Aug 30, 2012

Contributor

Can you possibly use classes or less generic id names for these page-specific content sections?

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikieditpage.html
+###############################################################################
+
+<%!
+ from r2.lib.filters import keep_space
+%>
+
+<div style="display: none;" id="conflict">
+ <h2 class="error">there was a conflict editing</h2>
+ <h1>${_("your edit")}</h1>
+ <em>${_("this edit is for you to resolve the conflict, any text in the box below will not save.")}</em><br/>
+ <textarea id="youredit"></textarea>
+ <span id="yourdiff"></span>
+ <h1>${_("current edit")}</h1>
+</div>
+
+<div style="display: none;" id="special">
@chromakode

chromakode Aug 30, 2012

Contributor

Ditto here.

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/lib/pages/wiki.py
+class WikiSettings(WikiBase):
+ def __init__(self, settings, mayedit, **context):
+ content = WikiPageSettings(settings, mayedit, **context)
+ context['wikiaction'] = ('settings', _("settings"))
+ WikiBase.__init__(self, content, **context)
+
+class WikiRevisions(WikiBase):
+ def __init__(self, revisions, **context):
+ content = WikiPageRevisions(revisions)
+ context['wikiaction'] = ('revisions/%s' % c.page, _("revisions"))
+ WikiBase.__init__(self, content, **context)
+
+class WikiRecent(WikiBase):
+ def __init__(self, revisions, **context):
+ content = WikiPageRevisions(revisions)
+ context['wikiaction'] = ('revisions', _("Viewing recent revisions for subreddit"))
@chromakode

chromakode Aug 30, 2012

Contributor

Instead of just "subreddit", could you include the /r/name of the subreddit here and for the listing below?

@chromakode chromakode commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikirevision.html
+ </td>
+
+ <td>
+ ${thing.author_name()}
+ </td>
+
+ <td style="font-style: italic;">
+ ${thing._get('reason')}
+ </td>
+
+ %if c.page and c.is_mod:
+ <td>
+ <a href="#" class="revision_hide" data-revision="${thing._id}" data-page="${thing.page}">hide</a>
+ </td>
+ <td class="wiki_revert" style="white-space: nowrap;">
+ ${ynbutton(_("revert here"), _("done"), "../r/%s/wiki/api/revert/%s/%s" % (thing.sr, thing._id, thing.page), block='true', callback="ReloadPage")}
@chromakode

chromakode Aug 30, 2012

Contributor

This block='true' causes the template to traceback in my install.

<type 'exceptions.TypeError'>: render_ynbutton() got an unexpected keyword argument 'block' 
Contributor

chromakode commented Aug 30, 2012

After playing around with the UI a bit, I think it would make sense to drop the "wiki | " / "editing | " / etc part from the page heading. Since this state is already represented by the buttons to the right of the title, I think the extra word distracts from the page title.

Contributor

chromakode commented Aug 30, 2012

Instead of placing the "wiki home" button in the footer, it might make sense to put it as the first part of the heading path.

Contributor

chromakode commented Aug 30, 2012

Regarding the "view recent revisions" and "list of pages" links at the bottom, I think they're likely to be missed and taken out of context at the bottom of the page. In particular, "view recent revisions" is easily interpreted as applying to the current page, rather than the entire wiki. These links belong somewhere that has the context of the global wiki, though unfortunately there aren't many good candidates I can think of atm.

What if you created a "WIKI TOOLS" sidebar section and moved them there, along with the "ban wiki contributors" and "add wiki contributors" links, and a "wiki settings" link?

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+from r2.lib.pages import PaneStack
+from r2.lib.utils import timesince
+
+import json
+
+page_descriptions = {'config/stylesheet':_("This page is the subreddit stylesheet, changes here apply to the subreddit css"),
+ 'config/sidebar':_("The contents of this page appear on the subreddit sidebar")}
+
+wiki_modactions = {'config/sidebar': "Updated subreddit sidebar"}
+
+WIKI_EDIT_RATELIMIT_SECONDS = g.wiki_edit_ratelimit_seconds
+
+class WikiController(RedditController):
+ allow_stylesheets = True
+
+ @validate(pv = VWikiPageAndVersion(('page', 'v', 'v2'), restricted=False))
@kemitche

kemitche Aug 30, 2012

Contributor

no spaces around kwargs

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+wiki_modactions = {'config/sidebar': "Updated subreddit sidebar"}
+
+WIKI_EDIT_RATELIMIT_SECONDS = g.wiki_edit_ratelimit_seconds
+
+class WikiController(RedditController):
+ allow_stylesheets = True
+
+ @validate(pv = VWikiPageAndVersion(('page', 'v', 'v2'), restricted=False))
+ def GET_wiki_page(self, pv):
+ page, version, version2 = pv
+ message = None
+
+ if not page:
+ return self.GET_wiki_create(page=c.page, view=True)
+
+ kw = {}
@kemitche

kemitche Aug 30, 2012

Contributor

Can the args stored here just be passed in normally? It's generally better to avoid **kw magic.

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+ if edit_by:
+ kw['edit_by'] = edit_by
+ if edit_date:
+ kw['edit_date'] = edit_date
+
+ diffcontent = None
+ if not version:
+ content = page.content
+ if c.is_mod and page.name in page_descriptions:
+ message = page_descriptions[page.name]
+ else:
+ message = _("viewing revision from %s") % timesince(version.date)
+ if version2:
+ timestamp1 = _("%s ago") % timesince(version.date)
+ timestamp2 = _("%s ago") % timesince(version2.date)
+ message = _("comparing revisions from %s and %s") % (timestamp1, timestamp2)
@kemitche

kemitche Aug 30, 2012

Contributor

i18n lesson time 😄 Don't "build up" translatable strings like this. Just create one message:

message = _("comparing revisions from %(date_1)s and %(date_2)s") % {'date_1': timesince(version.date), 'date_2': timesince(version2.date)}

You'll also notice that the format args are now named. This is generally required so that the translator can reorder the inputs (in this case, it really wouldn't matter, but the toolchain will still bark at us).

@andre-d

andre-d Sep 4, 2012

Contributor

timestamp1 and 2 are passed into the diff3 function as titles for the diff output. It was done this way for a reason.

@kemitche

kemitche Sep 4, 2012

Contributor

Then you should compose all 3 strings individually. You can't use translated strings like lego blocks; they don't translate well in languages that need to reorder the words to be grammatically correct.

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+ timestamp1 = _("%s ago") % timesince(version.date)
+ timestamp2 = _("%s ago") % timesince(version2.date)
+ message = _("comparing revisions from %s and %s") % (timestamp1, timestamp2)
+ diffcontent = make_htmldiff(version.content, version2.content, timestamp1, timestamp2)
+ content = version2.content
+ else:
+ message = _("viewing revision from %s ago") % timesince(version.date)
+ content = version.content
+
+ return WikiPageView(content, alert=message, v=version, diff=diffcontent, **kw).render()
+
+ @paginated_listing(max_page_size=100, backend='cassandra')
+ @validate(page = VWikiPage(('page'), restricted=False))
+ def GET_wiki_revisions(self, num, after, reverse, count, page):
+ if not page:
+ return self.GET_wiki_create(page=c.page, view=True)
@kemitche

kemitche Aug 30, 2012

Contributor

This isn't a redirect?

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+ builder = WikiRevisionBuilder(revisions, num=num, reverse=reverse, count=count, after=after, skip=not c.is_mod, wrap=default_thing_wrapper())
+ listing = WikiRevisionListing(builder).listing()
+ return WikiRevisions(listing).render()
+
+ @validate(may_create = VWikiPageCreate('page'))
+ def GET_wiki_create(self, may_create, page, view=False):
+ api = c.extension == 'json'
+ if c.error or api:
+ if api:
+ if c.error:
+ self.handle_error(403, **c.error)
+ else:
+ self.handle_error(404, 'PAGE_NOT_FOUND', may_create=may_create)
+ error = ''
+ if c.error['reason'] == 'PAGE_NAME_LENGTH':
+ error = _("this wiki cannot handle page names of that magnitude! Please select a page name shorter than %d characters") % c.error['max_length']
@kemitche

kemitche Aug 30, 2012

Contributor

lower case the 'Please' please 😄

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+ return WikiRevisions(listing).render()
+
+ @validate(may_create = VWikiPageCreate('page'))
+ def GET_wiki_create(self, may_create, page, view=False):
+ api = c.extension == 'json'
+ if c.error or api:
+ if api:
+ if c.error:
+ self.handle_error(403, **c.error)
+ else:
+ self.handle_error(404, 'PAGE_NOT_FOUND', may_create=may_create)
+ error = ''
+ if c.error['reason'] == 'PAGE_NAME_LENGTH':
+ error = _("this wiki cannot handle page names of that magnitude! Please select a page name shorter than %d characters") % c.error['max_length']
+ elif c.error['reason'] == 'PAGE_CREATED_ELSEWHERE':
+ error = _("this page is a special page, please go into the subreddit settings and save the field once to create this special page")
@kemitche

kemitche Aug 30, 2012

Contributor

Why not just do that for them?

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/controllers/wiki.py
+ return self.redirect(url)
+ return self.GET_wikiPage(page=page)
+
+ @validate(page = VWikiPageRevise('page', restricted=True))
+ def GET_wiki_revise(self, page, message=None, **kw):
+ page = page[0]
+ previous = kw.get('previous', page._get('revision'))
+ content = kw.get('content', page.content)
+ if not message and page.name in page_descriptions:
+ message = page_descriptions[page.name]
+ return WikiEdit(content, previous, alert=message).render()
+
+ @paginated_listing(max_page_size=100, backend='cassandra')
+ def GET_wiki_recent(self, num, after, reverse, count):
+ revisions = WikiRevision.get_recent(c.wiki_id)
+ builder = WikiRecentRevisionBuilder(revisions, num=num, reverse=reverse, count=count, after=after, skip=not c.is_mod, wrap=default_thing_wrapper())
@kemitche

kemitche Aug 30, 2012

Contributor

line is definitely > 80 chars

@chromakode chromakode and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/controllers/validator/wiki.py
+from os.path import normpath
+from r2.lib.db import tdb_cassandra
+from r2.models.wiki import WikiPage, WikiRevision
+from pylons.controllers.util import abort, redirect_to
+import datetime
+import json
+
+MAX_PAGE_NAME_LENGTH = g.wiki_max_page_name_length
+
+MAX_SEPERATORS = g.wiki_max_page_seperators
+
+def jsonAbort(code, reason=None, **data):
+ data['code'] = code
+ data['reason'] = reason if reason else 'UNKNOWN_ERROR'
+ if c.extension == 'json':
+ request.environ['usable_error_content'] = json.dumps(data)
@chromakode

chromakode Aug 30, 2012

Contributor

The recent OAuth merge included some changes to abort() to add additional info: https://github.com/reddit/reddit/blob/master/r2/r2/lib/base.py#L66

Instead of creating Yet Another Abort With Error Message (I'm pretty sure we have two now), is it possible to use/adapt some of that code?

@andre-d

andre-d Sep 4, 2012

Contributor

todo

@andre-d

andre-d Sep 6, 2012

Contributor

done

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ except NotFound:
+ return '[deleted]'
+
+class WikiRevision(tdb_cassandra.UuidThing, Printable):
+ """ Contains content (markdown), author of the edit, page the edit belongs to, and datetime of the edit """
+
+ _use_db = True
+ _connection_pool = 'main'
+
+ _str_props = ('pageid', 'content', 'author', 'reason')
+ _bool_props = ('hidden')
+
+ cache_ignore = set(['subreddit'] + list(_str_props)).union(Printable.cache_ignore)
+
+ def author_name(self):
+ return get_author_name(self._get('author'))
@kemitche

kemitche Aug 30, 2012

Contributor

The _get function in tdb_cassandra seems to be unused elsewhere, and doesn't seem to add anything that getattr doesn't supply; could you change all the _get calls to use getattr instead?

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+
+ @classmethod
+ def is_special(cls, page):
+ return page in special_pages
+
+ @property
+ def special(self):
+ return WikiPage.is_special(self.name)
+
+ def add_to_listing(self):
+ WikiPagesBySR.add_object(self)
+ self.listed_ = True
+
+ def _on_create(self):
+ self.add_to_listing()
+ self._committed = True # Prevent infinite loop
@kemitche

kemitche Aug 30, 2012

Contributor

Since _on_create is called during _commit, why is the call to _commit() necessary here?

@andre-d

andre-d Aug 30, 2012

Contributor

This can mainly be removed now I think, iirc it was because I had a problem where add_to_listing was failing and I put this in to make it redo it on a reload later in the event that it failed.

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ def special(self):
+ return WikiPage.is_special(self.name)
+
+ def add_to_listing(self):
+ WikiPagesBySR.add_object(self)
+ self.listed_ = True
+
+ def _on_create(self):
+ self.add_to_listing()
+ self._committed = True # Prevent infinite loop
+ self._commit()
+
+ def _on_commit(self):
+ if not self._get('listed_'):
+ self.add_to_listing()
+ self._commit()
@kemitche

kemitche Aug 30, 2012

Contributor

I'm not a fan of the fact that _commit() is being called while in the middle of a call to _commit(). Seems very prone to subtle bugs and the infinite looping you appear to have encountered. Seems like the reason for the commit call is due to the tracking of listed_; perhaps that could be revamped.

I'd also recommend (and this may even help solve the issue) just overriding _commit() here, instead of adding a hook.

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ if not self._get('listed_'):
+ self.add_to_listing()
+ self._commit()
+
+ def remove_editor(self, user):
+ WikiPageEditors._remove(self._id, [user])
+
+ def add_editor(self, user):
+ WikiPageEditors._set_values(self._id, {user: ''})
+
+ @classmethod
+ def get_pages(cls, sr, after=None):
+ NUM_AT_A_TIME = 1000
+ pages = WikiPagesBySR.query([sr], after=after, count=NUM_AT_A_TIME)
+ pages = [p for p in pages]
+ if len(pages) == NUM_AT_A_TIME:
@kemitche

kemitche Aug 30, 2012

Contributor

This might be an outdated worry or silly "just in case", but I believe it's better to write these checks as if len(pages) >= NUM_AT_A_TIME:

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ def _on_commit(self):
+ if not self._get('listed_'):
+ self.add_to_listing()
+ self._commit()
+
+ def remove_editor(self, user):
+ WikiPageEditors._remove(self._id, [user])
+
+ def add_editor(self, user):
+ WikiPageEditors._set_values(self._id, {user: ''})
+
+ @classmethod
+ def get_pages(cls, sr, after=None):
+ NUM_AT_A_TIME = 1000
+ pages = WikiPagesBySR.query([sr], after=after, count=NUM_AT_A_TIME)
+ pages = [p for p in pages]
@kemitche

kemitche Aug 30, 2012

Contributor

Could just be pages = list(pages).

Also, I think you should be using tdb_cassandra.max_column_count, though there's been talk of refactoring some of that.

@spladug

spladug Aug 30, 2012

Owner

This is pretty different from max_column_count, we probably shouldn't conflate the two. Agreed on the pages = list(...) thing.

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ self.add_to_listing()
+ self._commit()
+
+ def remove_editor(self, user):
+ WikiPageEditors._remove(self._id, [user])
+
+ def add_editor(self, user):
+ WikiPageEditors._set_values(self._id, {user: ''})
+
+ @classmethod
+ def get_pages(cls, sr, after=None):
+ NUM_AT_A_TIME = 1000
+ pages = WikiPagesBySR.query([sr], after=after, count=NUM_AT_A_TIME)
+ pages = [p for p in pages]
+ if len(pages) == NUM_AT_A_TIME:
+ return pages + self.get_pages(sr, after=pages[-1])
@kemitche

kemitche Aug 30, 2012

Contributor

NameError: There's no such thing as self in a classmethod.

@andre-d

andre-d Aug 30, 2012

Contributor

err, whops

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ return WikiPageEditors._byID(self._id, properties=properties)._values() or []
+ except tdb_cassandra.NotFoundException:
+ return []
+
+ def has_editor(self, editor):
+ return bool(self.get_editors(properties=editor))
+
+ def revise(self, content, previous = None, author=None, force=False, reason=None):
+ if self.content == content:
+ return
+ max_length = special_length_restrictions_bytes.get(self.name, MAX_PAGE_LENGTH_BYTES)
+ if len(content) > max_length:
+ raise ContentLengthError(max_length)
+ try:
+ revision = self.revision
+ except:
@kemitche

kemitche Aug 30, 2012

Contributor

Be explicit with your exception catching.

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/models/wiki.py
+ except ConflictException as e:
+ e.new_id = revision
+ raise e
+
+ wr = WikiRevision.create(self._id, content, author, reason)
+ self.content = content
+ self.last_edit_by = author
+ self.last_edit_date = wr.date
+ self.revision = wr._id
+ self._commit()
+ return wr
+
+ def change_permlevel(self, permlevel, force=False):
+ if permlevel == self.permlevel:
+ return
+ if not force and int(permlevel) not in range(3):
@kemitche

kemitche Aug 30, 2012

Contributor

No magic numbers in the code please; reference a "static" variable containing the allowed permissions levels.

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/public/static/css/reddit.css
@@ -5686,7 +5828,6 @@ tr.gold-accent + tr > td {
margin: .75em 0;
}
-/** one-time password stuff **/
@kemitche

kemitche Aug 30, 2012

Contributor

What happened here?

@andre-d

andre-d Aug 30, 2012

Contributor

rebase accident I guess

@kemitche kemitche and 1 other commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikipagediscussions.html
+##
+## The Original Developer is the Initial Developer. The Initial Developer of
+## the Original Code is reddit Inc.
+##
+## All portions of the code written by reddit are Copyright (c) 2006-2012
+## reddit Inc. All Rights Reserved.
+###############################################################################
+
+<%!
+ from r2.lib.template_helpers import get_domain
+%>
+
+${thing.listing}
+
+<div class="morelink discussionlink">
+<a href="/submit?url=http://${get_domain()}/wiki/${c.page}">${_("submit a discussion")}
@kemitche

kemitche Aug 30, 2012

Contributor

Does this URL include the revision of the wiki page?

@andre-d

andre-d Aug 30, 2012

Contributor

Should it?

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikipagelisting.html
+## WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License for
+## the specific language governing rights and limitations under the License.
+##
+## The Original Code is reddit.
+##
+## The Original Developer is the Initial Developer. The Initial Developer of
+## the Original Code is reddit Inc.
+##
+## All portions of the code written by reddit are Copyright (c) 2006-2012
+## reddit Inc. All Rights Reserved.
+###############################################################################
+
+<% from r2.models.wiki import WikiPage %>
+
+<%def name="listing(pages)">
+ %for k, v in pages.iteritems():
@kemitche

kemitche Aug 30, 2012

Contributor

Could you use more descriptive variable names here, please.

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikipagerevisions.html
+## Software distributed under the License is distributed on an "AS IS" basis,
+## WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License for
+## the specific language governing rights and limitations under the License.
+##
+## The Original Code is reddit.
+##
+## The Original Developer is the Initial Developer. The Initial Developer of
+## the Original Code is reddit Inc.
+##
+## All portions of the code written by reddit are Copyright (c) 2006-2012
+## reddit Inc. All Rights Reserved.
+###############################################################################
+
+${thing.revisions}
+%if c.page and thing.revisions.things:
+ <button onclick="r.wiki.goCompare('${c.page}', '${c.wiki_base_url}')">compare selected</button>
@kemitche

kemitche Aug 30, 2012

Contributor

Change to ${_("compare selected")}

@kemitche kemitche commented on an outdated diff Aug 30, 2012

r2/r2/templates/wikipagesettings.html
+<%namespace name="utils" file="utils.html"/>
+
+<div class="fancy-settings">
+ %if thing.show_settings:
+ <form id="pagesettings" method="post">
+ <%utils:line_field title=" ${_('who can edit this page?')}">
+ <input type="radio" name="permlevel" id="permlevel0" value="0"
+ %if thing.permlevel == 0:
+ checked
+ %endif
+ /><label for="permlevel0">${_('anyone may edit')}</label><br/>
+ <input type="radio" name="permlevel" id="permlevel1" value="1"
+ %if thing.permlevel == 1:
+ checked
+ %endif
+ /><label for="permlevel1">${_('only mods or those allowed may edit')}</label><br/>
@kemitche

kemitche Aug 30, 2012

Contributor

change 'those allowed' to 'approved wiki editors'

Contributor

chromakode commented Sep 6, 2012

Two small requests:

  • Please move the add/ban wiki contributors buttons to the wiki sidebar box
  • Please change the title to "WIKI TOOLS"

bsimpson63 and others added some commits Apr 16, 2012

@bsimpson63 @andre-d bsimpson63 DenormalizedView that stores entire Thing in column. ea7c4c5
@bsimpson63 @andre-d bsimpson63 Add _on_commit hook for tdb_cassandra.ThingBase. ddeb3f0
@bsimpson63 @andre-d bsimpson63 DenormalizedView encodes dates to base64 before dumping to json. 6b9cb2e
@bsimpson63 @andre-d bsimpson63 handle different types of dates from json 85bca4e
@andre-d andre-d Errors: Add the ability to error with json 3cb64a8
@andre-d andre-d Diff3: Added diff3 wrapper cdb6411
@andre-d andre-d Errors: Add more error handler codes 08106b1
@andre-d andre-d Subreddit: Insert fake sr modactions into the default sr 763f256
@andre-d andre-d tdb_cassandra: Fix unicode error in deserialization 8b62cc0
@andre-d andre-d tdb_cassandra: Add the ability to remove items from views 831b7ea
@andre-d andre-d Links: Add the ability to get links to urls rather than just articles 06614ea
@andre-d andre-d Wiki: Base wiki code bc573a0
@andre-d andre-d Wiki: Added wiki ini example items ff8ac91
@andre-d andre-d Wiki: Base sylesheet b4bdea8
@andre-d andre-d Subreddit: Use wiki for stylesheet/description/sidebar ce7319e
@andre-d andre-d Wiki: Add revision listing builders cf10db7
@andre-d andre-d Wiki: Add various json endpoints 6131479
@andre-d andre-d Wiki: Add interface for banning and allowing users 4fe8579
@andre-d andre-d Subreddit: Add a wiki tab to top bar 7c5a44b
@andre-d andre-d Cleanup 08079f8
@chromakode @andre-d chromakode JS style tweaks. fbcd909
@chromakode @andre-d chromakode Refactor JS into a namespaced module. 25a6a0e
@chromakode @andre-d chromakode Use single quoted strings in JS. 5745ba7
@chromakode @andre-d chromakode Whitespace. 88d8276
@andre-d andre-d More cleanup 75da9ab
@andre-d andre-d Allow img in markdown filter 97a0dd5
@andre-d andre-d Fixed images removing most of a rendered page 1eeb88d
@andre-d andre-d More cleanup d025d3e
@andre-d andre-d Merge down 97d6383
@andre-d andre-d Further cleanup 7d64d11
@andre-d andre-d Added ynbutton callback to happen after the jquery request on success 65edca0
@andre-d andre-d Further cleanup 8eb83c4
@andre-d andre-d Restructure permission checks to be more generic 2e85068
@andre-d andre-d Corrected the very broken image extraction a784363
@andre-d andre-d Rearrange page not found 25b52bc
@chromakode @andre-d chromakode Wiki actions style tweaks. 82421b9
@andre-d andre-d user overwrote c.user, use username instead ec96a2f
@andre-d andre-d Whops, do not extract all images, just invalid ones 37648a1
@andre-d andre-d Rebase down d02139e
@andre-d andre-d Fix remove page editor b07599f
@andre-d andre-d unified permissions checking again 8158724
@andre-d andre-d Whops, default accounts to have global wiki access d15c663
@andre-d andre-d Rearranged create page such that the errors were more helpfull again a6bb4c0
@andre-d andre-d Rebase down c87144a
@andre-d andre-d Correct broken has_editor 810cc71
@andre-d andre-d Fix ratelimit and json abort from api fc8c48e
@andre-d andre-d Gut out old, now unused wiki css 028f3b4
@andre-d andre-d Add an emergency wiki kill switch ebc1c07
@andre-d andre-d Fixed non-visible pages appearing on listing dd33cde
@andre-d andre-d Restructure jsonabort to work with base abort f5310e5
@andre-d andre-d Revert reason in english f759067
@andre-d andre-d Correct string construction d680621
@andre-d andre-d Change wiki sidebar content name 1abdf3c
@andre-d andre-d Moved ban/add wiki contributors to wiki tools menu cf0e962

@chromakode chromakode commented on an outdated diff Sep 6, 2012

r2/r2/controllers/validator/wiki.py
+ redirect_to(url)
+
+ page = normalize_page(page)
+
+ c.page = page
+ if (not c.is_wiki_mod) and self.modonly:
+ jsonabort(403, 'MOD_REQUIRED')
+
+ wp = self.ValidPage(page)
+
+ # TODO: MAKE NOT REQUIRED
+ c.page_obj = wp
+
+ return wp
+
+ def ValidPage(self, page):
@chromakode

chromakode Sep 6, 2012

Contributor

Why are these method names capitalized?

@chromakode chromakode commented on an outdated diff Sep 6, 2012

r2/r2/lib/base.py
@@ -63,7 +64,15 @@ def is_local_address(ip):
# TODO: support the /20 and /24 private networks? make this configurable?
return ip.startswith('10.')
-def abort(code_or_exception=None, detail="", headers=None, comment=None):
+def jsonabort(code, reason=None, **data):
+ body = None
+ if c.render_style == 'api':
+ data['code'] = code
+ data['reason'] = reason if reason else 'UNKNOWN_ERROR'
+ body = json.dumps(data)
+ abort(code, body=body)
@chromakode

chromakode Sep 6, 2012

Contributor

This function still feels like a big hack to me. I don't think you should create another abort method to pass the data you need. I'd recommend creating an exception class (similar to ForbiddenError) that encapsulates the info you need to pass, and formatting it in the error controller.

@chromakode chromakode and 1 other commented on an outdated diff Sep 6, 2012

r2/r2/lib/base.py
@@ -63,7 +64,15 @@ def is_local_address(ip):
# TODO: support the /20 and /24 private networks? make this configurable?
return ip.startswith('10.')
-def abort(code_or_exception=None, detail="", headers=None, comment=None):
+def jsonabort(code, reason=None, **data):
+ body = None
+ if c.render_style == 'api':
+ data['code'] = code
@chromakode

chromakode Sep 6, 2012

Contributor

Why do you need to send down the HTTP status code in the JSON?

@andre-d

andre-d Sep 6, 2012

Contributor

Good question, can be removed

@chromakode chromakode commented on an outdated diff Sep 6, 2012

r2/r2/controllers/wiki.py
+ c.wiki_id = g.default_sr if frontpage else c.site.name
+ c.page = None
+ c.show_wiki_actions = True
+ self.editconflict = False
+ c.is_wiki_mod = (c.user_is_admin or c.site.is_moderator(c.user)) if c.user_is_loggedin else False
+ c.wikidisabled = False
+
+ mode = c.site.wikimode
+ if not mode or mode == 'disabled':
+ if not c.is_wiki_mod:
+ self.handle_error(403, 'WIKI_DISABLED')
+ else:
+ c.wikidisabled = True
+
+class WikiApiController(WikiController):
+ @validate(VRatelimit(rate_user = True, rate_ip = True, prefix = "rate_wiki_"),
@chromakode

chromakode Sep 6, 2012

Contributor

Could you please remove the spaces between = here? :)

@chromakode chromakode and 2 others commented on an outdated diff Sep 10, 2012

r2/r2/lib/db/tdb_cassandra.py
@@ -1435,6 +1444,85 @@ def _set_values(cls, row_key, col_values,
# can we be smarter here?
thing_cache.delete(cls._cache_key_id(row_key))
+
+ @classmethod
+ @will_write
+ def _remove(cls, key, columns):
+ cls._cf.remove(key, columns)
+ thing_cache.delete(cls._cache_key_id(key))
+
+class DenormalizedView(View):
+ """Store the entire underlying object inside the View column."""
+
+ # Do we need to check for _dirty?
@chromakode

chromakode Sep 10, 2012

Contributor

Is this comment still relevant? Can it be removed?

@andre-d

andre-d Sep 10, 2012

Contributor

Ask kemitche bsimpson63

@bsimpson63

bsimpson63 Sep 10, 2012

Owner

DenormalizedViews are created from Thing objects and duplicate all the attributes so you don't have to go look up the original object. My thinking was that we may want to check that the object is _commited and not _dirty before allowing someone to create a DenormalizedView from it so it will be consistent.

@andre-d

andre-d Sep 10, 2012

Contributor

Ahhh, I think we should be fine without it. Seems pretty obvious...

@chromakode chromakode commented on an outdated diff Sep 10, 2012

r2/r2/controllers/wiki.py
+ c.wiki_id = g.default_sr if frontpage else c.site.name
+ c.page = None
+ c.show_wiki_actions = True
+ self.editconflict = False
+ c.is_wiki_mod = (c.user_is_admin or c.site.is_moderator(c.user)) if c.user_is_loggedin else False
+ c.wikidisabled = False
+
+ mode = c.site.wikimode
+ if not mode or mode == 'disabled':
+ if not c.is_wiki_mod:
+ self.handle_error(403, 'WIKI_DISABLED')
+ else:
+ c.wikidisabled = True
+
+class WikiApiController(WikiController):
+ @validate(pageandprevious = VWikiPageRevise(('page', 'previous'), restricted=True),
@chromakode

chromakode Sep 10, 2012

Contributor

Whitespace around the '='

@chromakode chromakode commented on an outdated diff Sep 10, 2012

r2/r2/controllers/wiki.py
+ skip=not c.is_wiki_mod)
+ listing = WikiRevisionListing(builder).listing()
+ return WikiRecent(listing).render()
+
+ def GET_wiki_listing(self):
+ def check_hidden(page):
+ g.log.debug("Got here %s" % str(this_may_view(page)))
+ return this_may_view(page)
+ pages = WikiPage.get_listing(c.wiki_id, filter_check=check_hidden)
+ return WikiListing(pages).render()
+
+ def GET_wiki_redirect(self, page):
+ return redirect_to(str("%s/%s" % (c.wiki_base_url, page)), _code=301)
+
+ @base_listing
+ @validate(page = VWikiPage('page', restricted=True))
@chromakode

chromakode Sep 10, 2012

Contributor

Whitespace around the '='

@chromakode chromakode commented on an outdated diff Sep 10, 2012

r2/r2/controllers/wiki.py
+ else:
+ message = _("viewing revision from %s ago") % timesince(version.date)
+ content = version.content
+
+ return WikiPageView(content, alert=message, v=version, diff=diffcontent,
+ edit_by=edit_by, edit_date=edit_date).render()
+
+ @paginated_listing(max_page_size=100, backend='cassandra')
+ @validate(page=VWikiPage(('page'), restricted=False))
+ def GET_wiki_revisions(self, num, after, reverse, count, page):
+ revisions = page.get_revisions()
+ builder = WikiRevisionBuilder(revisions, num=num, reverse=reverse, count=count, after=after, skip=not c.is_wiki_mod, wrap=default_thing_wrapper())
+ listing = WikiRevisionListing(builder).listing()
+ return WikiRevisions(listing).render()
+
+ @validate(may_create = VWikiPageCreate('page'))
@chromakode

chromakode Sep 10, 2012

Contributor

Whitespace around the '='

@logan logan and 1 other commented on an outdated diff Sep 10, 2012

r2/r2/controllers/validator/wiki.py
+
+from pylons.controllers.util import redirect_to
+from pylons import c, g, request
+
+from r2.models.wiki import WikiPage, WikiRevision
+from r2.controllers.validator import Validator
+from r2.lib.db import tdb_cassandra
+from r2.lib.base import abort
+from r2.controllers.errors import WikiError
+
+MAX_PAGE_NAME_LENGTH = g.wiki_max_page_name_length
+
+MAX_SEPARATORS = g.wiki_max_page_separators
+
+def apiabort(code, reason, **data):
+ abort(WikiError(code, reason, **data))
@logan

logan Sep 10, 2012

I like your goal of eliminating tedious, risky error checking in controller methods. However, we need to enforce some separation of concerns. I'd like to see an implementation where validators don't raise exceptions (so these validators can be used like any other existing ones). Someday we can have validators raising exceptions, but the way the code is laid out now doesn't support that in a manageable fashion yet.

I think the way that would make everyone happy would be to introduce a @wiki_validate, and enhance Validator.set_error, ErrorSet.add, and Error to include an optional HTTP error code. We don't have an inheritence scheme in place yet for @validate-like decorators, so you basically need to duplicate (or wrap) @validate -- which is easy -- and then scan c.errors and abort accordingly. I don't think that'll be too hard, but you'll also have to change all your apiabort calls to be explicit returns. Does this sound reasonable?

@andre-d

andre-d Sep 10, 2012

Contributor

yeah, no problem

andre-d closed this Sep 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment