code for /+trash view

Code for /+trash view.
I am breaking the code into two parts:-
1) If an item is without names (AND not intentionally created/kept so, e.g. for threaded comment system, that uses nameless items), we set the TRASH flag to indicate that it is not needed any more.

2) Manual entry into trash view:- If we decide that we don't need an item any more, we even could set the TRASH flag without having all names removed first.

This is the code for the first part.

[Thomas.J.Waldmann, 2013-08-07 14:49:57]

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

https://codereview.appspot.com/12560043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:836: title_name = _('Deleted Items')
put that into the render_template call as you see it in mychanges().

also put a u before 'Deleted Items' to be consistent.

https://codereview.appspot.com/12560043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:844: q = And([Term(WIKINAME,
app.cfg.interwikiname), Term(DELETED, True)])
obviously this is not namespace aware, but you have to think about this for all
sorts of views still.

-> later changeset, put it on your todo or issue tracker.

https://codereview.appspot.com/12560043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:848: results.append(deletedEntry(rev.fqname,
rev.meta[NAME_OLD], rev.meta[REVID], rev.meta[MTIME], rev.meta[COMMENT]))
the user (name) who deleted it might be also interesting.

https://codereview.appspot.com/12560043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:849: return results
hmm, guess we need to think more about this.

the DELETED flag is now the only place that indicates "yes, this item was
deleted".

so, even an item with a non-empty NAME list could be deleted. but we only show
NAME_OLD, not NAME.

otoh, if an item has NAME = ["foo", "bar"] and you hit "delete" on the UI when
looking at "foo", it will remove the "foo" and for the user, it looks like he
deleted "foo". but as there is a name left, we won't set DELETED flag, so the
user can't find it on /+deleted. What does he do if he wants to undelete?

maybe the current implementation should be rather /+garbage (like in "garbage
collection" semantics of python: all unreferenced items).

but still, the "i deleted it and want to undelete" usecase should be addressed
somehow.

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

https://codereview.appspot.com/12560043/diff/1/MoinMoin/items/__init__.py#new...
MoinMoin/items/__init__.py:541: elif not meta.get(ITEMID):
add a comment about why you check ITEMID

https://codereview.appspot.com/12560043/diff/1/MoinMoin/items/__init__.py#new...
MoinMoin/items/__init__.py:547: meta[DELETED] = not meta.get(NAME)
we introduced the DELETED flag to indicate whether it should be considered
trash/garbage, because we wanted to have non-trash nameless items (e.g. for
comment system).

what you do here, is having an equivalence of DELETED == True and NAME == [],
which is completely spoiling it.

https://codereview.appspot.com/12560043/diff/1/MoinMoin/templates/deleted.html
File MoinMoin/templates/deleted.html (right):

https://codereview.appspot.com/12560043/diff/1/MoinMoin/templates/deleted.htm...
MoinMoin/templates/deleted.html:12: {% for result in
results|sort(attribute='oldname', reverse=True) %}
maybe the reverse mtime would be also nice as sort order, so last deleted stuff
comes first.

guess we might want to have both, which is yet another need for a nice table
widget...

-> todo list (look later what xiaq was working on)

https://codereview.appspot.com/12560043/diff/1/MoinMoin/templates/deleted.htm...
MoinMoin/templates/deleted.html:32: <p class ="info searchhitinfobar"><a
href="{{ url_for('frontend.revert_item', item_name=result.fqname.fullname,
rev=result.revid) }}">{{ _('Undelete') }}</a>
did you check whether this practically works?

what's item_name looking like here?

guess it can't be a name as the item has no name any more...

https://codereview.appspot.com/12560043/diff/1/MoinMoin/templates/deleted.htm...
MoinMoin/templates/deleted.html:33: <a href="{{ url_for('frontend.show_item',
item_name=result.fqname) }}" fqname={{result.fqname}}>{{ _('Show') }}</a></p>
same here

[ashu1461, 2013-08-10 08:49:29]

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/templates/garbage...
File MoinMoin/templates/garbage.html (right):

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/templates/garbage...
MoinMoin/templates/garbage.html:28: <td class="moin-wordbreak">{{ result.comment
}}</td>
This works.
result.fqname would be the FQNAME being referred by itemid.
The item_name there is confusing it should actually be something like
item_identifier.
will change globally in a separate changeset later.
Same for the revert below.

[Thomas.J.Waldmann, 2013-08-10 18:16:07]

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

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:833: Returns the deleted items.
Returns garbage items

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:838: title_name=_(u'Deleted Items'),
2x Garbage Items

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:842: def _deleted():
_garbage

sigh, why don't you see such systematic stuff yourself?

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:843: q = And([Term(WIKINAME,
app.cfg.interwikiname), Term(DELETED, True)])
we could even rename DELETED -> GARBAGE.

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:844: deletedEntry =
namedtuple('deletedEntry','fqname oldname revid mtime comment editor')
...

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:848: results.append(deletedEntry(rev.fqname,
meta[NAME_OLD], meta[PARENTID], meta[MTIME], meta[COMMENT],
get_editor_info(meta)))
...

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

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:541: # So if ITEMID is present in the meta, we can
afford not having any name.
unclear: where do these comments belong to?
the placement and indentation does not make that clear.

also: isn't ITEMID always present because it gets added by the storage layer
automatically even if it was not in the meta we gave it to store?

but: if there is no ITEMID in meta given to storage, it means a new item is
created (new uuid generated by storage). so even if there is some name existing,
it would create a different item (and not a new revision of same name item).

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:552: meta[DELETED] = True
it seems like you either coded this wrong or you are redefining what
meta[DELETED] shall mean.

document what it means and how this all shall work.

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/templates/garbage...
File MoinMoin/templates/garbage.html (right):

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/templates/garbage...
MoinMoin/templates/garbage.html:25: <td>{{ result.mtime|datetimeformat }}</td>
decide whether you have spaces around | or not.

follow the main style you see everywhere else.

https://codereview.appspot.com/12560043/diff/17001/MoinMoin/templates/garbage...
MoinMoin/templates/garbage.html:28: <td class="moin-wordbreak">{{ result.comment
}}</td>
> The item_name there is confusing it should actually be something like
> item_identifier.
> will change globally in a separate changeset later.

OK, good idea!

[ashu1461, 2013-08-12 16:49:57]

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

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:550: 
The earlier method to find NAME_OLD does not work if we edit the meta name
through the JSON.
So, what I am using now is 
if there exists a name s.t. it was present in the previous meta but is not
present now, fill NAME_OLD.

[Thomas.J.Waldmann, 2013-08-12 18:57:55]

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

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:541: meta[TRASHED] = True
please explain.

i already asked that for last code. now you just killed the comments, changed
the code, but there is no new comment (in the code) explaining that code.

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:547: deleted_names = [name for name in
currentrev.meta.get(NAME, []) if name not in meta.get(NAME, [])]
please rewrite using set operations.

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/templates/trash.html
File MoinMoin/templates/trash.html (right):

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/templates/trash.h...
MoinMoin/templates/trash.html:21: {% for result in results|
sort(attribute='mtime', reverse=True)%}
put a space before %

[ashu1461, 2013-08-12 19:24:08]

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

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:541: meta[TRASHED] = True
The earlier comments were for a different statement, i killed that statement so
killed the comments.

What I am checking here is:-
meta[NAME] can be None/[] in two cases,
 *either the ITEM was created with default no names as done by the comment
system.
 *it had names earlier and someone deleted them.
So, If the item is of type1 set the TRASHED flag.

[Thomas.J.Waldmann, 2013-08-12 19:29:13]

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

https://codereview.appspot.com/12560043/diff/28001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:541: meta[TRASHED] = True
"...but there is no new comment (in the code) explaining that code."

also, it seems to only get executed if fqname.field != NAME_EXACT.

[Thomas.J.Waldmann, 2013-08-12 20:19:42]

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

https://codereview.appspot.com/12560043/diff/40001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:541: # If the item is created initally with no names
(as by the comment system) set the TRASHED flag.
that doesn't make sense.

why do you set trash status if it was e.g. created by the comment system?

https://codereview.appspot.com/12560043/diff/40001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:549: deleted_names = {name for name in
currentrev.meta.get(NAME, []) if name not in meta.get(NAME, [])}
i rather meant computing the set difference.

[Thomas.J.Waldmann, 2013-08-13 21:33:15]

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

https://codereview.appspot.com/12560043/diff/46001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:551: meta[TRASH] = True
looks better. unit tests?

[Thomas.J.Waldmann, 2013-08-13 21:37:31]

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

https://codereview.appspot.com/12560043/diff/46001/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:551: meta[TRASH] = True
you're inconsistent. at one place TRASH, at another TRASHED.

use TRASH.

[Thomas.J.Waldmann, 2013-08-15 16:22:46]

https://codereview.appspot.com/12560043/diff/51001/MoinMoin/items/_tests/test...
File MoinMoin/items/_tests/test_Item.py (right):

https://codereview.appspot.com/12560043/diff/51001/MoinMoin/items/_tests/test...
MoinMoin/items/_tests/test_Item.py:376: fqname = u'trash_item_test'
isn't it confusing if this is just a simple string here, while it is a tuple
elsewhere?

https://codereview.appspot.com/12560043/diff/51001/MoinMoin/items/_tests/test...
MoinMoin/items/_tests/test_Item.py:380: become_trusted()
do you need that?

https://codereview.appspot.com/12560043/diff/51001/MoinMoin/items/_tests/test...
MoinMoin/items/_tests/test_Item.py:403: 
maybe start with 2 names, then delete one, then delete last one and make sure
TRASH is only set after the 2nd delete.

[ashu1461, 2013-08-15 16:55:40]

https://codereview.appspot.com/12560043/diff/51001/MoinMoin/items/_tests/test...
File MoinMoin/items/_tests/test_Item.py (right):

https://codereview.appspot.com/12560043/diff/51001/MoinMoin/items/_tests/test...
MoinMoin/items/_tests/test_Item.py:376: fqname = u'trash_item_test'
On 2013/08/15 16:22:46, Thomas.J.Waldmann wrote:
> isn't it confusing if this is just a simple string here, while it is a tuple
> elsewhere?

This fqname is finally converted to tuple, in the Item.create.
I can change it to name but, setting name=u'@itemid/' + ..
wont look good.