Unifying global_index and item index page

[sinha, 2011-07-29 21:22:48]

Most of things are working, got 1-2 small issue.

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

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:472:
contenttype=contenttype_to_class(item.contenttype), # always get contenttype
here as application/x-nonexistent.
here i am getting contenttype as application/x-nonexistent for a item, but when
i reloads the page it comes as the actual. 
So what i am supposed to do, is there any different way to retrieve contenttype
of a item other than just doing item.contenttype ?

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html#ne...
MoinMoin/templates/index.html:65: {% else %}
Here i have to use one more if condition just to see if a itemname exist and i
will generate the url according to that.

As when item_name='', the url thus generated as "+index/" but there is only
"/+index" at the frontend route, what should i do ? should i use this if
condition and generate url based on that or i just simply add "/+index/" at the
frontend.route such that it will serve the global_index on seeing that.

[Reimar Bauer, 2011-07-30 08:15:52]

answers and new comments.

remove the missed index2 in a different changeset

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

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:450: 
Please search for index2, you missed to remove it at least on one place

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

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:472:
contenttype=contenttype_to_class(item.contenttype), # always get contenttype
here as application/x-nonexistent.
On 2011/07/29 21:22:48, sinha wrote:
> here i am getting contenttype as application/x-nonexistent for a item, but
when
> i reloads the page it comes as the actual. 
> So what i am supposed to do, is there any different way to retrieve
contenttype
> of a item other than just doing item.contenttype ?

At uploading request the new file is not existing as item and therefore you get
item.contenttype as application/x-nonexistent. The request afterwards then knows
the item because it was stored by moin.

Search in items for
# this is likely a guess by the browser, based on the filename

This is how we currently get the contenttype by the browser

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:507: split_char = u'/'
why have you created this var?

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html#ne...
MoinMoin/templates/index.html:5: {% if item_name: %}
if it makes this code better readable, you can also define macros

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html#ne...
MoinMoin/templates/index.html:65: {% else %}
On 2011/07/29 21:22:48, sinha wrote:
> Here i have to use one more if condition just to see if a itemname exist and i
> will generate the url according to that.
> 
> As when item_name='', the url thus generated as "+index/" but there is only
> "/+index" at the frontend route, what should i do ? should i use this if
> condition and generate url based on that or i just simply add "/+index/" at
the
> frontend.route such that it will serve the global_index on seeing that.

I would prefer to have it as /+index/ because the latter is the root. The first
one depends on the wsgi definition. It could be also e.g. /wiki/+index/

Please verify the unit tests too

[ThomasWaldmann, 2011-07-30 11:11:19]

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

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:481: item = Item.create(item_name) # when
item_name='', it gives toplevel index
why did you remove exception handling?

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:486: if request.method == "POST":
elif...

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:489: if form[gname].value is True]
do you really need "is True" here or is it sufficient if it is trueish? if the
latter, you could just leave away " is True".

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:490: if not selected_groups:
hmm, if user did not select any groups, you give the default?

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:493: startswith =
request.values.get("startswith", None)
does have .get() None as default anyway?

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:496: ct_groups = [(gname, ", ".join(["%s" %
ctlabel for ctname, ctlabel in contenttypes]))
what's the point of using "%s" % ctlabel? why not just ctlabel?

[sinha, 2011-07-30 22:34:10]

Reply to the questions and a new patch set added for review.

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

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:481: item = Item.create(item_name) # when
item_name='', it gives toplevel index
On 2011/07/30 11:11:24, ThomasWaldmann wrote:
> why did you remove exception handling?

sry, i missed that actually i had made changes to global_index and thus forgot
to add that.

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:490: if not selected_groups:
On 2011/07/30 11:11:24, ThomasWaldmann wrote:
> hmm, if user did not select any groups, you give the default?

yes, all will be selected.

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:493: startswith =
request.values.get("startswith", None)
On 2011/07/30 11:11:24, ThomasWaldmann wrote:
> does have .get() None as default anyway?

yes, corrected.

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:496: ct_groups = [(gname, ", ".join(["%s" %
ctlabel for ctname, ctlabel in contenttypes]))
On 2011/07/30 11:11:24, ThomasWaldmann wrote:
> what's the point of using "%s" % ctlabel? why not just ctlabel?

it was my fault, corrected.

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:507: split_char = u'/'
On 2011/07/30 08:15:52, Reimar Bauer wrote:
> why have you created this var?

you mean why assigning a variable to that expression or what is purpose of that
variable.

If it is about purpose, then for displaying the path at top i am sending the
names of item's parents in a list form.

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html#ne...
MoinMoin/templates/index.html:65: {% else %}
On 2011/07/30 08:15:52, Reimar Bauer wrote:
> On 2011/07/29 21:22:48, sinha wrote:
> > Here i have to use one more if condition just to see if a itemname exist and
i
> > will generate the url according to that.
> > 
> > As when item_name='', the url thus generated as "+index/" but there is only
> > "/+index" at the frontend route, what should i do ? should i use this if
> > condition and generate url based on that or i just simply add "/+index/" at
> the
> > frontend.route such that it will serve the global_index on seeing that.
> 
> I would prefer to have it as /+index/ because the latter is the root. The
first
> one depends on the wsgi definition. It could be also e.g. /wiki/+index/
> 
> Please verify the unit tests too

i have changed it to "/+index/" at frontend route, and now if someone enters url
as "/+index" only, it gets redirected to "/+index/", so is this normal or
something needs to be done here.

[Reimar Bauer, 2011-07-31 07:54:10]

some small changes needed

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

http://codereview.appspot.com/4830045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:507: split_char = u'/'
On 2011/07/30 22:34:10, sinha wrote:
> On 2011/07/30 08:15:52, Reimar Bauer wrote:
> > why have you created this var?
> 
> you mean why assigning a variable to that expression

yes and I guess it is for cosmetics. The split char won't change.

 or what is purpose of that
> variable.
> 
> If it is about purpose, then for displaying the path at top i am sending the
> names of item's parents in a list form.

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/4830045/diff/1/MoinMoin/templates/index.html#ne...
MoinMoin/templates/index.html:65: {% else %}
On 2011/07/30 22:34:10, sinha wrote:
> On 2011/07/30 08:15:52, Reimar Bauer wrote:
> > On 2011/07/29 21:22:48, sinha wrote:
> > > Here i have to use one more if condition just to see if a itemname exist
and
> i
> > > will generate the url according to that.
> > > 
> > > As when item_name='', the url thus generated as "+index/" but there is
only
> > > "/+index" at the frontend route, what should i do ? should i use this if
> > > condition and generate url based on that or i just simply add "/+index/"
at
> > the
> > > frontend.route such that it will serve the global_index on seeing that.
> > 
> > I would prefer to have it as /+index/ because the latter is the root. The
> first
> > one depends on the wsgi definition. It could be also e.g. /wiki/+index/
> > 
> > Please verify the unit tests too
> 
> i have changed it to "/+index/" at frontend route, and now if someone enters
url
> as "/+index" only, it gets redirected to "/+index/", so is this normal or
> something needs to be done here.

I guess this happens by the framework. You should discuss this on #pocoo

http://codereview.appspot.com/4830045/diff/5002/MoinMoin/templates/index.html
File MoinMoin/templates/index.html (right):

http://codereview.appspot.com/4830045/diff/5002/MoinMoin/templates/index.html...
MoinMoin/templates/index.html:7: <div>Filter by content type </div>
i18n

http://codereview.appspot.com/4830045/diff/5002/MoinMoin/templates/index.html...
MoinMoin/templates/index.html:45: <div id="moin-letters">
id="moin-initials" ?