Rough TicketIndex implementation

Rough TicketIndex implementation

[xiaq, 2013-07-07 13:04:37]

plz review this! :)

[Thomas.J.Waldmann, 2013-07-11 16:23:49]

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:203: description = L_('An index for all tickets under
it')
under what?

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:223: )
so we need to create a separate item and store it into storage just to get a
list of other items?

does this "index item" store any other useful info?

otherwise it somehow rather sounds like a view than an item.

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/index_w...
File MoinMoin/templates/index_widgets.html (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/index_w...
MoinMoin/templates/index_widgets.html:20: {{
storage.get_item(itemid=itemid).name }}
so you fetch the item from storage just to determine its name(s)?

this rather looks like a task for an index access, itemids and names are all
stored into index.

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/ticket/...
File MoinMoin/templates/ticket/index.html (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/ticket/...
MoinMoin/templates/ticket/index.html:41: <tr><td colspan="100"> {{ _("(No Ticket
Yet)") }} </td></tr>
i think it should be plural for 0: "tickets" (and check casing)

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.py
File MoinMoin/util/itemfields.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.p...
MoinMoin/util/itemfields.py:15: pass
rather use a docstring than "pass"

also, I am wondering: didn't we already have similar code for misc. metadata
types (from your work last year)? maybe in som existing other module?

[xiaq, 2013-07-12 01:31:21]

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:203: description = L_('An index for all tickets under
it')
On 2013/07/11 16:23:49, Thomas.J.Waldmann wrote:
> under what?

Under the TicketIndex item. 'An index item for all tickets under itself' would
be likely better...

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:223: )
On 2013/07/11 16:23:49, Thomas.J.Waldmann wrote:
> so we need to create a separate item and store it into storage just to get a
> list of other items?
> 
> does this "index item" store any other useful info?
> 
> otherwise it somehow rather sounds like a view than an item.

Its content is shown before the index. I've followed the pattern of
BlogEntry/Blog here, though I believe for maximum flexibility we should
integrate this into +index *and* make index usable as a macro, so that instead
of having a TicketIndex, we have a normal wiki page that reads

"""

**introduction to the ticket tracker**

<<Index(.,itemtype=ticket)>>

**perhaps some words below the index**

"""

Also, it is reasonable to store a set of tickets under a single item, like what
we have now (https://moinmo.in/MoinMoinBugs).

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/index_w...
File MoinMoin/templates/index_widgets.html (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/index_w...
MoinMoin/templates/index_widgets.html:20: {{
storage.get_item(itemid=itemid).name }}
On 2013/07/11 16:23:49, Thomas.J.Waldmann wrote:
> so you fetch the item from storage just to determine its name(s)?
> 
> this rather looks like a task for an index access, itemids and names are all
> stored into index.

i'll look into that.

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/ticket/...
File MoinMoin/templates/ticket/index.html (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/templates/ticket/...
MoinMoin/templates/ticket/index.html:41: <tr><td colspan="100"> {{ _("(No Ticket
Yet)") }} </td></tr>
On 2013/07/11 16:23:49, Thomas.J.Waldmann wrote:
> i think it should be plural for 0: "tickets" (and check casing)

ok

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.py
File MoinMoin/util/itemfields.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.p...
MoinMoin/util/itemfields.py:15: pass
On 2013/07/11 16:23:49, Thomas.J.Waldmann wrote:
> rather use a docstring than "pass"
> 
> also, I am wondering: didn't we already have similar code for misc. metadata
> types (from your work last year)? maybe in som existing other module?

The widgets in forms.py maybe?

They are related, but different. We actually need 2 widget systems, one for
editing (forms.py and forms.html) another for displaying (in the case of tickets
- either on ticket index or on ticket item page). The latter is much simpler,
since simply calling str() would suffice for many fields. But there are some
which requires a bit more work, e.g. metadata fields that are itemids need to be
shown as item names linking to the item.

Also I just realized that the same widget used on the index can be used on the
item page too so IndexFieldSpec is not an appropriate name.

It would be nice to make these 2 systems cohesive. Better, even integrate the
Whoosh schema into this system (after we have pluggable Whoosh schema). But
that's yet to be done, and doing them separate first is easier. :)

Moving this file to MoinMoin/index_widgets.py would be better, in analog with
forms.py/forms.html.

[xiaq, 2013-07-12 01:50:05]

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.py
File MoinMoin/util/itemfields.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.p...
MoinMoin/util/itemfields.py:15: pass
On 2013/07/12 01:31:22, xiaq wrote:
> On 2013/07/11 16:23:49, Thomas.J.Waldmann wrote:
> > rather use a docstring than "pass"
> > 
> > also, I am wondering: didn't we already have similar code for misc. metadata
> > types (from your work last year)? maybe in som existing other module?
> 
> The widgets in forms.py maybe?
> 
> They are related, but different. We actually need 2 widget systems, one for
> editing (forms.py and forms.html) another for displaying (in the case of
tickets
> - either on ticket index or on ticket item page). The latter is much simpler,
> since simply calling str() would suffice for many fields. But there are some
> which requires a bit more work, e.g. metadata fields that are itemids need to
be
> shown as item names linking to the item.
> 
> Also I just realized that the same widget used on the index can be used on the
> item page too so IndexFieldSpec is not an appropriate name.
> 
> It would be nice to make these 2 systems cohesive. Better, even integrate the
> Whoosh schema into this system (after we have pluggable Whoosh schema). But
> that's yet to be done, and doing them separate first is easier. :)
> 
> Moving this file to MoinMoin/index_widgets.py would be better, in analog with
> forms.py/forms.html.

Ah, I forgot that the same widgets can be used for displaying the metadata
either on index or on item page. So display_widgets.py/display_widgets.html

[Thomas.J.Waldmann, 2013-07-13 14:40:58]

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:223: )
in some cases this would make sense, if one really provides additional
information.

but in general I rather dislike that method - we had that in moin 1.x and ended
up having lots of "system pages" that just called a macro, polluted the
namespace (by showing up on TitleIndex) and should have been rather views than
pages.

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.py
File MoinMoin/util/itemfields.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/util/itemfields.p...
MoinMoin/util/itemfields.py:15: pass
maybe all of that stuff could go into widgets.* later?

[xiaq, 2013-07-14 15:41:04]

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/10889044/diff/10001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:223: )
On 2013/07/13 14:40:58, Thomas.J.Waldmann wrote:
> in some cases this would make sense, if one really provides additional
> information.
> 
> but in general I rather dislike that method - we had that in moin 1.x and
ended
> up having lots of "system pages" that just called a macro, polluted the
> namespace (by showing up on TitleIndex) and should have been rather views than
> pages.

i see several possible approaches here:

1) a +tickets view.
2) a TicketIndex itemtype (what is done now)
3) an <<Index>> macro

1) and 2) has the advantage of requiring no boilerplate to set up a ticket
index. The only difference is view vs. item. Yet, I think an item is useful even
if it doesn't contain any description; it is used only as a directory to place
all tickets under.

With 3) there is need for some boilerplate: you at least have to write something
like <<Index(.,itemtype=ticket)>>. But with that also comes flexibility - you
may set predefined ordering, categorizing, etc. like
<<Index(.,itemtype=ticket,sort=assigned_to)>>.

Also, there is something between 2) and 3): 2.5) a more generic Index itemtype
that has some metadata properties that can be configured to behave like
TicketIndex or +index. This almost offers the flexibility of 3), but the page
layout is a bit restricted - with 3) you can place text above and below
<<Index>>, wrap it in a table etc. - but that's not necessarily a bad thing.

Personally I'm still favoring 3). The problem of boilerplate can be partly
solved by using template. I know the mess of "system pages" in moin1.x, but I
find this a valid use case for such macros.

[Thomas.J.Waldmann, 2013-07-19 21:13:13]

https://codereview.appspot.com/10889044/diff/42001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):

https://codereview.appspot.com/10889044/diff/42001/MoinMoin/items/ticket.py#n...
MoinMoin/items/ticket.py:30: from MoinMoin.display_field import DisplayField
maybe don't put it into the toplevel package