support if key is not unique + displaying links

Displaying Links
1) Changed the item trail to show the value/id (filter modified as per the requirement)
2) Search results show value normally and on-mouse-hovering show the fullname.
3) Added code for the case when field is not a unique identifier, render the results using a link_list.

[ana.balica, 2013-07-03 21:34:55]

https://codereview.appspot.com/10915044/diff/1/MoinMoin/templates/common.js
File MoinMoin/templates/common.js (right):

https://codereview.appspot.com/10915044/diff/1/MoinMoin/templates/common.js#n...
MoinMoin/templates/common.js:232: $(document).ready(function(){
Regarding the js style of this document - first the function is declared, then
it's called. So maybe do:
`$(document).ready(FunctionCall);`

https://codereview.appspot.com/10915044/diff/1/MoinMoin/templates/common.js#n...
MoinMoin/templates/common.js:237: $(this).html(fullname);
Since you are querying `this` 3 times, it's better to store it to a local var.
Should be something like this:

var $this = $(this);
fullname = $this.attr("fqname");

[ashu1461, 2013-07-04 00:20:56]

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py#...
MoinMoin/items/__init__.py:155: item = flaskg.storage.get_item(**fqname.query)
made a method 'query' of CompositeName that returns the related whoosh query.

[Thomas.J.Waldmann, 2013-07-13 15:43:25]

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py#...
MoinMoin/items/__init__.py:158: fqname = item.fqname
and what if not?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py#...
MoinMoin/items/__init__.py:255: The Field is not a UNIQUE.
that docstring is not that helpful. be more precise / tell what this means.

The constant involved seems to be UFIELDS (not UNIQUE, if that also should
denote a constant).

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py#...
MoinMoin/items/__init__.py:293: raise FieldNotUniqueError
maybe FieldNotUniqueError("field {0} is not in UFIELDS".format(fqname.field)) ?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.js
File MoinMoin/templates/common.js (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.j...
MoinMoin/templates/common.js:237: fullname = $this.attr("fqname");
is that using an invalid html attr "fqname"?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.j...
MoinMoin/templates/common.js:239: $this.html(fullname);
is this a XSS by fullname?

e.g. fullname = <b>isthatbold?</b>

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.j...
MoinMoin/templates/common.js:245: 
alternatively: just put it into title attr?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/link_lis...
File MoinMoin/templates/link_list_no_item_panel.html (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/link_lis...
MoinMoin/templates/link_list_no_item_panel.html:10: <li><a class="moin-fqname"
href="{{ url_for('frontend.show_item', item_name=item_name) }}"
fqname={{item_name}}>{{ item_name.value }}</a></li>
not valid html

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:93: item_name = fqname.value
location breadcrumbs only make sense if the location is determined by a name,
like MainPage/SubPage/SubSubPage - where do you check fqname is using a name
(and not something else)?

also, after we made sure it really is a name (not itemid, ...), we could rather
operate on the whole fqname string representation, not just the .value.

e.g.

somenamespace/MainItem/SubItem could link to all these 3 parts.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:98: breadcrumbs.append((fq_segment, fq_current,
bool(self.storage.get_item(**fq_current.query))))
why not has_item()?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:118: exists =
bool(self.storage.get_item(**fqname.query))
why not has_item()?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:348: long names are used (WYGIWYD).
docstring needs update (and also the function name), it is rather about item
identifiers now than names.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:356: fqname = split_fqname(fqname)
you didn't document that this can be called with unicode. is this temporary
only? add a TODO?

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:366: name = u'{0}...{1}'.format(name[:half + left],
name[-half:])
shorten_id code contained in that function
vs
shorten-no-id code contained directly here, inline.

this is a bit inconsistent.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/util/interwiki.py
File MoinMoin/util/interwiki.py (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:148: return {NAMESPACE: self.namespace, u'field':
self.field, u'item_name': self.value}
please add to your todo list that 'item_name' should be changed to
'(item_)identifier' - globally and in a separate changeset.

alternatively:
item_key / item_value
or any other less confusing naming

should be also in-sync with the tuple element names.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:160: returns a dict that can be used as a whoosh
query
... to lookup index documents matching this CompositeName.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/util/interwiki.py#...
MoinMoin/util/interwiki.py:163: return {NAMESPACE: self.namespace, field:
self.value}
2 empty lines, see pep8

[ana.balica, 2013-07-14 10:56:55]

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/link_lis...
File MoinMoin/templates/link_list_no_item_panel.html (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/link_lis...
MoinMoin/templates/link_list_no_item_panel.html:10: <li><a class="moin-fqname"
href="{{ url_for('frontend.show_item', item_name=item_name) }}"
fqname={{item_name}}>{{ item_name.value }}</a></li>
In order to make it a valid HTML5 document use custom attributes prefixed with
`data-`.
http://www.w3.org/html/wg/drafts/html/master/single-page.html#embedding-custo...

Also to make it easier for development check the rendered templates with
http://validator.w3.org/ :)

[ashu1461, 2013-07-17 02:37:58]

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/items/__init__.py#...
MoinMoin/items/__init__.py:158: fqname = item.fqname
On 2013/07/13 15:43:25, Thomas.J.Waldmann wrote:
> and what if not?

now item.fqname can't be None, will change in a separate changeset.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.js
File MoinMoin/templates/common.js (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.j...
MoinMoin/templates/common.js:239: $this.html(fullname);
On 2013/07/13 15:43:25, Thomas.J.Waldmann wrote:
> is this a XSS by fullname?
> 
> e.g. fullname = <b>isthatbold?</b>

no

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/common.j...
MoinMoin/templates/common.js:245: 
On 2013/07/13 15:43:25, Thomas.J.Waldmann wrote:
> alternatively: just put it into title attr?

We can require toggling in other places like in displaying results.

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/link_lis...
File MoinMoin/templates/link_list_no_item_panel.html (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/templates/link_lis...
MoinMoin/templates/link_list_no_item_panel.html:10: <li><a class="moin-fqname"
href="{{ url_for('frontend.show_item', item_name=item_name) }}"
fqname={{item_name}}>{{ item_name.value }}</a></li>
thanks :)

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:93: item_name = fqname.value
okay fine.
Cases to consider:- 
1) field is NAME_EXACT:
   return format [namespace/][itemname/][../]..
2) field is not NAME_EXACT:
   return format value(linking to ns/@field/value)

https://codereview.appspot.com/10915044/diff/7001/MoinMoin/themes/__init__.py...
MoinMoin/themes/__init__.py:98: breadcrumbs.append((fq_segment, fq_current,
bool(self.storage.get_item(**fq_current.query))))
On 2013/07/13 15:43:25, Thomas.J.Waldmann wrote:
> why not has_item()?

has_item takes only item_name as argument.

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/common.js
File MoinMoin/templates/common.js (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/common....
MoinMoin/templates/common.js:243: $(document).ready(togglefqname);
Dont know why 
doing $this = $(this)
and then $this.attr('..')
does not work sometimes ..

[Thomas.J.Waldmann, 2013-07-17 11:16:33]

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/apps/frontend/vie...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:364: item_name=item_name)
do these 2 make sense if it's not just a unique item name?

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:259: The Field is not a UFILED(unique Field).
typo

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/common.js
File MoinMoin/templates/common.js (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/common....
MoinMoin/templates/common.js:243: $(document).ready(togglefqname);
On 2013/07/17 02:37:59, ashu1461 wrote:
> Dont know why 
> doing $this = $(this)
> and then $this.attr('..')
> does not work sometimes ..

i have no idea what you are talking about or how this related to the code above.
but as I already said: ask someone else about javascript stuff.

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/link_li...
File MoinMoin/templates/link_list_no_item_panel.html (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/link_li...
MoinMoin/templates/link_list_no_item_panel.html:10: <li><a class="moin-fqname"
href="{{ url_for('frontend.show_item', item_name=fq_name) }}"
data-fqname={{fq_name}}>{{ fq_name.value }}</a></li>
"{{fq_name}}"

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:98: breadcrumbs.append((fq_ns, fq_ns, False))
it looks a bit like we are dealing differently with namespaces depending on
whether it is the default '' namespace or not.

don't think this should be the case.

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:353: Shorten fqnames
it sounds like you are dealing with more than one here.

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:381: :param name: item_name, unicode
the right side is a description, so no need to insert an underscore there.

[ashu1461, 2013-07-17 17:42:23]

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:259: The Field is not a UFILED(unique Field).
On 2013/07/17 11:16:33, Thomas.J.Waldmann wrote:
> typo

y i no see them ???

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/common.js
File MoinMoin/templates/common.js (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/templates/common....
MoinMoin/templates/common.js:243: $(document).ready(togglefqname);
not a issue .. sorry

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:98: breadcrumbs.append((fq_ns, fq_ns, False))
The main purpose of doing this is that we don't want to insert empty entities in
breadcrumbs (no sense of it actually).
It is not about dealing with the default namespace differently, it is just
because it is empty, we have a similar check for names too see line 100.

[Thomas.J.Waldmann, 2013-07-17 22:46:18]

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/10915044/diff/24002/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:98: breadcrumbs.append((fq_ns, fq_ns, False))
but if you have a non-default namespace, you can click on it and something
happens (goes to main page of this namespace?).

why shouldn't same be possible with default ns?

[Thomas.J.Waldmann, 2013-07-19 20:48:42]

https://codereview.appspot.com/10915044/diff/54001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/10915044/diff/54001/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:97: fq_segment = CompositeName(u'', NAME_EXACT,
namespace or '~')
doesn't that really USE ~ ? i meant just displaying ~ but use ""