Let Item subclasses handle +show

Let Item subclasses handle +show

[xiaq, 2012-07-29 09:17:32]

Patch based off my repo (which is one changeset ahead of main repo, see comment
in modify_select_itemtype.html). Please review! :)

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (left):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:319: status = 404
Status code 404 is generated handled in NonExistent, which is more OO now.

(Of course more OO doesn't always mean better, but the meaning of NonExistent is
to depend on polymorphism instead of testing for the existence of items
everywhere.)

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
File MoinMoin/templates/modify_select_itemtype.html (left):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
MoinMoin/templates/modify_select_itemtype.html:1: {% extends
theme("layout.html") %}
I've renamed the template in a previous changeset not yet in main repo (pull
request already sent), so this patch won't directly apply to the main repo...

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
File MoinMoin/templates/modify_select_itemtype.html (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
MoinMoin/templates/modify_select_itemtype.html:3: <h1>{{ _("Item not found,
create it now?") }}</h1>
Since this template is now also shown when visiting a nonexistent item, i've
reworded the title.

[Reimar Bauer, 2012-07-30 06:36:43]

some questions, comments

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:591:
data_rendered=Markup(self.content._render_data()),
hmm, as default I would rather say it is unsafe content - needs esacpe. There
are at least some items possible which require escaping.

why did you choose Markup here?

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:670: # XXX 403 on +show can be confusing
What is confusing, tell

[xiaq, 2012-07-30 13:43:01]

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:591:
data_rendered=Markup(self.content._render_data()),
On 2012/07/30 06:36:43, Reimar Bauer wrote:
> hmm, as default I would rather say it is unsafe content - needs esacpe. There
> are at least some items possible which require escaping.
> 
> why did you choose Markup here?

was moved from show_item.

if there is a problem, i'll fix it in a followup changeset...

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:670: # XXX 403 on +show can be confusing
On 2012/07/30 06:36:43, Reimar Bauer wrote:
> What is confusing, tell

you visit http://localhost:8080/some_non_existent_item and if you don't have
right to create you get 403 directly.

[ThomasJWaldmann, 2012-08-01 17:05:23]

hmm, i am wondering a bit why we did not have the code like that before.

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:306:
@frontend.route('/+show/+<rev>/<itemname:item_name>', methods=['GET'])
first has GET and POST, second only GET?

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:314: abort(403)
btw: that 403 is because of read access being denied

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:591:
data_rendered=Markup(self.content._render_data()),
he didn't choose, it already was there like that.

and it is needed because our converters return html, which you definitely WANT
to get rendered, not shown verbatim as html source.

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:670: # XXX 403 on +show can be confusing
hmm, for a nonexisting item, you should always get 404.

403 should be returned if you really TRY to create it (modify) and there is a
ACL violation, but not before trying.

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
File MoinMoin/templates/modify_select_itemtype.html (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
MoinMoin/templates/modify_select_itemtype.html:5: {{ _("Item \"%(name)s\" does
not exist (yet), but you can try creating it now. Please select the type of the
item you want to create.", name=item_name) }}
please use single quotes inside to make lifes easier. :)

[xiaq, 2012-08-09 05:11:13]

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:306:
@frontend.route('/+show/+<rev>/<itemname:item_name>', methods=['GET'])
On 2012/08/01 17:05:23, ThomasJWaldmann wrote:
> first has GET and POST, second only GET?

First form is used to allow modifying behavior. Like POSTing to some ticket's
url /lorem instead of /+modify/lorem.

OTOH, modifying a historical version is not allowed.

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
File MoinMoin/templates/modify_select_itemtype.html (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
MoinMoin/templates/modify_select_itemtype.html:5: {{ _("Item \"%(name)s\" does
not exist (yet), but you can try creating it now. Please select the type of the
item you want to create.", name=item_name) }}
On 2012/08/01 17:05:23, ThomasJWaldmann wrote:
> please use single quotes inside to make lifes easier. :)

perhaps use single quotes outside. double quotes are more common on the ui.

[ThomasJWaldmann, 2012-08-09 08:20:56]

looks ok

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:306:
@frontend.route('/+show/+<rev>/<itemname:item_name>', methods=['GET'])
not yet at least. :)

ok, i think you should put a comment into the source, because at a quick glance,
it looks like a bug.

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
File MoinMoin/templates/modify_select_itemtype.html (right):

http://codereview.appspot.com/6457048/diff/2001/MoinMoin/templates/modify_sel...
MoinMoin/templates/modify_select_itemtype.html:5: {{ _("Item \"%(name)s\" does
not exist (yet), but you can try creating it now. Please select the type of the
item you want to create.", name=item_name) }}
in English, you need ' all the time.

"it's"

so the other way is usually better.

[xiaq, 2012-08-12 07:30:06]

This is the version i'm including in an upcoming pull request.