Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Move c.wiki_page into pre()

  • Loading branch information...
commit 27ccb35cfe2b78f0ee2e6d5ea6d70127c2d852fc 1 parent 1eaa38f
@andre-d authored
View
99 r2/r2/controllers/validator/wiki.py
@@ -171,10 +171,33 @@ def normalize_page(page):
return page
class AbortWikiError(Exception):
- pass
+ def __init__(self, reason=None, error=None):
+ self.reason = reason
+ self.error = error
page_match_regex = re.compile(r'^[\w_/]+\Z')
+
+def validate_page_name(page):
+ if not page:
+ # If no page is specified, give the index page
+ page = "index"
+
+ try:
+ page = str(page)
+ except UnicodeEncodeError:
+ raise AbortWikiError('INVALID_PAGE_NAME', 400)

Why not raise an InvalidWikiPageName here rather than a generic abort?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ if ' ' in page:
+ page = page.replace(' ', '_')

Worth moving this into normalize_page and calling normalize_page before validating the page name w/ the regex?

@andre-d Owner
andre-d added a note

I don't see any reason not to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ if not page_match_regex.match(page):
+ raise AbortWikiError('INVALID_PAGE_NAME', code=400)
+
+ page = normalize_page(page)
+
+ return page
+
class VWikiPage(Validator):
def __init__(self, param, required=True, restricted=True, modonly=False, **kw):
self.restricted = restricted
@@ -183,54 +206,30 @@ def __init__(self, param, required=True, restricted=True, modonly=False, **kw):
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:
- return self.set_error('INVALID_PAGE_NAME', code=400)
-
- if ' ' in page:
- new_name = page.replace(' ', '_')
- url = '%s/%s' % (c.wiki_base_url, new_name)
- redirect_to(url)

Where did this redirect logic go?

@andre-d Owner
andre-d added a note

As mentioned in irc, accidentally regressed that, coming back in a commit tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
-
- if not page_match_regex.match(page):
- return self.set_error('INVALID_PAGE_NAME', code=400)
-
- page = normalize_page(page)
-
- c.wiki_page = page
if (not c.is_wiki_mod) and self.modonly:
return self.set_error('MOD_REQUIRED', code=403)
-
- try:
- wp = self.validpage(page)
- except AbortWikiError:
- return
-
- c.wiki_may_revise = this_may_revise(wp)
-
- return wp
-
- def validpage(self, page):
- try:
- wp = WikiPage.get(c.site, page)
- if self.restricted and wp.restricted:
- if not wp.special:
- self.set_error('RESTRICTED_PAGE', code=403)
- raise AbortWikiError
- if not this_may_view(wp):
- self.set_error('MAY_NOT_VIEW', code=403)
- raise AbortWikiError
- return wp
- except tdb_cassandra.NotFound:
- if self.required:
- self.set_error('PAGE_NOT_FOUND', code=404)
- raise AbortWikiError
- return None
+
+ if not isinstance(page, WikiPage):
+ try:
+ page = validate_page_name(page)
+ except AbortWikiError as e:
+ return self.set_error(e.reason, e.code)
+
+ try:
+ page = WikiPage.get(c.site, page)
+ except tdb_cassandra.NotFound:
+ if self.required:
+ return self.set_error('PAGE_NOT_FOUND', code=404)
+ return None
+
+ if self.restricted and page.restricted:
+ if not page.special:
+ return self.set_error('RESTRICTED_PAGE', code=403)
+
+ if not this_may_view(page):
+ return self.set_error('MAY_NOT_VIEW', code=403)
+
+ return page
def validversion(self, version, pageid=None):
if not version:
@@ -282,7 +281,7 @@ def run(self, page, previous=None):
return
if not wp:
return self.set_error('INVALID_PAGE', code=404)
- if not c.wiki_may_revise:
+ if not this_may_revise(wp):
return self.set_error('MAY_NOT_REVISE', code=403)
if previous:
try:
@@ -303,8 +302,6 @@ def __init__(self, param, **kw):
VWikiPage.__init__(self, param, required=False, **kw)
def run(self, page):
- if not page:

Ah, here's that page thing again. Can you please rebase this out of both commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- return
wp = VWikiPage.run(self, page)
if c.errors:
return
@@ -319,7 +316,7 @@ def run(self, page):
c.error = {'reason': 'PAGE_NAME_MAX_SEPARATORS', 'MAX_SEPARATORS': MAX_SEPARATORS}
elif len(page) > MAX_PAGE_NAME_LENGTH:
c.error = {'reason': 'PAGE_NAME_LENGTH', 'max_length': MAX_PAGE_NAME_LENGTH}
- return c.wiki_may_revise
+ return this_may_revise(wp)
def param_docs(self):
return {
View
29 r2/r2/controllers/wiki.py
@@ -38,7 +38,9 @@
from r2.controllers.validator.wiki import (VWikiPage, VWikiPageAndVersion,
VWikiPageRevise, VWikiPageCreate,
- this_may_view, wiki_validate)
+ this_may_view, this_may_revise,
+ wiki_validate, AbortWikiError,
+ validate_page_name)
from r2.controllers.api_docs import api_doc, api_section
@@ -70,6 +72,7 @@
'config/sidebar':_("The contents of this page appear on the subreddit sidebar")}
class WikiController(RedditController):
+ wiki_setup_page_cvars = True
allow_stylesheets = True
@wiki_validate(pv=VWikiPageAndVersion(('page', 'v', 'v2'),
@@ -188,7 +191,7 @@ def GET_wiki_settings(self, page):
return WikiSettings(settings, mayedit, show_settings=not page.special).render()
@wiki_validate(page=VWikiPage('page', restricted=True, modonly=True),\
- permlevel=VInt('permlevel'))

This whitespace change should probably go in a different commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ permlevel=VInt('permlevel'))
def POST_wiki_settings(self, page, permlevel):
oldpermlevel = page.permlevel
try:
@@ -213,7 +216,20 @@ def pre(self):
c.wiki_base_url = '%s/wiki' % base
c.wiki_api_url = '%s/api/wiki' % base
c.wiki_id = g.default_sr if frontpage else c.site.name
- c.wiki_page = None
+
+ if self.wiki_setup_page_cvars:
+ try:
+ page = request.environ["pylons.routes_dict"].get('page')
+ c.wiki_page = validate_page_name(page)
+ try:
+ wp = WikiPage.get(c.site, page)
+ c.wiki_may_revise = this_may_revise(wp)

Can you please move the code that won't raise a tdb_cassandra.NotFound to an else clause of the try?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ request.environ["pylons.routes_dict"]['page'] = wp

Can you design this in a way that doesn't require overwriting this global? I was under the impression request.environ["pylons.routes_dict"] contained values verbatim from the URL. Changing it to an instantiated object is surprising.

@andre-d Owner
andre-d added a note

Not sure what the best design would be, it has to be something specific enough so that if it exists, other calls to VWikiPage do not unexpectedly use it when it exists. (The whole point of moving this logic out was so that VWikiPage could become generic).

Sorry, I don't quite follow. What do you mean by VWikiPage becoming generic?

@andre-d Owner
andre-d added a note

Not setting c.page for an example, being able to be used twice, etc

@andre-d Owner
andre-d added a note

Will restructure such that c.wiki_page is not required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ except tdb_cassandra.NotFound:
+ pass
+ except AbortWikiError as e:

What raises an AbortWikiError? Can you please increase the specificity of the try?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.handle_error(e.code, e.reason)
+
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
@@ -227,6 +243,8 @@ def pre(self):
c.wikidisabled = True
class WikiApiController(WikiController):
+ wiki_setup_page_cvars = False

What's the reasoning behind disabling the cvars here?

@andre-d Owner
andre-d added a note

APIs do not use the url route to input the data, would involve checking for page in in GET, POST, and the url route, then placing it back into the matching place to be pushed into the validators...was just much more simple to only use it for that. It is made a lot cleaner in 8823993 though

@andre-d Owner
andre-d added a note

Oh yes, and it had to be redesigned as some pages like "list of pages" were ending up with a c.page for "index" and such (Actually a bug in this commit, fixed in the commit mentioned above). So there had to be code for some pages not to do the c.page stuff anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
@wiki_validate(VModhash(),
pageandprevious=VWikiPageRevise(('page', 'previous'), restricted=True),
content=VMarkdown(('content')))
@@ -292,8 +310,9 @@ def GET_wiki_create(self, may_create):
elif c.error:
self.handle_error(403, **c.error)
else:
- WikiPage.create(c.site, c.wiki_page)
- url = join_urls(c.wiki_base_url, '/edit/', c.wiki_page)
+ page = request.get['page']
+ WikiPage.create(c.site, page)
+ url = join_urls(c.wiki_base_url, '/edit/', page)
return self.redirect(url)
@wiki_validate(VModhash(),
View
6 r2/r2/lib/jsontemplates.py
@@ -567,16 +567,16 @@ def render(self, thing, *a, **kw):
class WikiViewJsonTemplate(ThingJsonTemplate):
def render(self, thing, *a, **kw):
- edit_date = time.mktime(thing.edit_date.timetuple())
+ edit_date = time.mktime(thing.edit_date.timetuple()) if thing.edit_date else None

What does this have to do with this commit?

@andre-d Owner
andre-d added a note

Whops, likely an accidental --amend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return ObjectTemplate(dict(content_md=thing.page_content_md,
content_html=wikimarkdown(thing.page_content_md),
revision_by=thing.edit_by,
revision_date=edit_date,
- may_revise=thing.may_revise))
+ may_revise=c.wiki_may_revise))

Does this belong in a different commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
class WikiRevisionJsonTemplate(ThingJsonTemplate):
def render(self, thing, *a, **kw):
- timestamp = time.mktime(thing.date.timetuple())
+ timestamp = time.mktime(thing.date.timetuple()) if thing.date else None

Ditto?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return ObjectTemplate(dict(author=thing._get('author'),
id=str(thing._id),
timestamp=timestamp,
@chromakode

Ah, here's that page thing again. Can you please rebase this out of both commits?

@chromakode

What does this have to do with this commit?

@chromakode

Why not raise an InvalidWikiPageName here rather than a generic abort?

@chromakode

This whitespace change should probably go in a different commit.

@chromakode

Can you design this in a way that doesn't require overwriting this global? I was under the impression request.environ["pylons.routes_dict"] contained values verbatim from the URL. Changing it to an instantiated object is surprising.

@chromakode

What's the reasoning behind disabling the cvars here?

@chromakode

Can you please move the code that won't raise a tdb_cassandra.NotFound to an else clause of the try?

@chromakode

What raises an AbortWikiError? Can you please increase the specificity of the try?

@chromakode

Does this belong in a different commit?

@chromakode

Worth moving this into normalize_page and calling normalize_page before validating the page name w/ the regex?

@chromakode

Where did this redirect logic go?

@andre-d

Whops, likely an accidental --amend

@andre-d

APIs do not use the url route to input the data, would involve checking for page in in GET, POST, and the url route, then placing it back into the matching place to be pushed into the validators...was just much more simple to only use it for that. It is made a lot cleaner in 8823993 though

@andre-d

Oh yes, and it had to be redesigned as some pages like "list of pages" were ending up with a c.page for "index" and such (Actually a bug in this commit, fixed in the commit mentioned above). So there had to be code for some pages not to do the c.page stuff anyway.

@andre-d

I don't see any reason not to

@andre-d

As mentioned in irc, accidentally regressed that, coming back in a commit tonight.

@andre-d

Not sure what the best design would be, it has to be something specific enough so that if it exists, other calls to VWikiPage do not unexpectedly use it when it exists. (The whole point of moving this logic out was so that VWikiPage could become generic).

@chromakode

Sorry, I don't quite follow. What do you mean by VWikiPage becoming generic?

@andre-d

Not setting c.page for an example, being able to be used twice, etc

@andre-d

Will restructure such that c.wiki_page is not required.

Please sign in to comment.
Something went wrong with that request. Please try again.