Code review - Issue 246020043: file uploadhttps://codereview.appspot.com/2015-08-12T07:10:15+00:00rietveld
Message from unknown
2015-06-11T13:49:37+00:00vipulurn:md5:4d376503b61e445f25fa18be0a17d37c
Message from vipul.sharma20@gmail.com
2015-06-11T14:15:13+00:00vipulurn:md5:03ae7b83118e35cb3bddbf3773016789
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode164
MoinMoin/items/ticket.py:164: def file_upload(self, data_file):
creating item of file uploaded and adding 'refer_to' in meta to refer to current ticket item
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode168
MoinMoin/items/ticket.py:168: try:
when I create and modify item again, the meta updates the 'refer_to' value otherwise not don't know why
Message from Thomas.J.Waldmann@gmail.com
2015-06-11T15:44:45+00:00Thomas.J.Waldmannurn:md5:3d3f8857e80933a15e83e9cb2562b76d
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode166
MoinMoin/items/ticket.py:166: contenttype = data_file.content_type
is that always present or could it be empty / None?
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode169
MoinMoin/items/ticket.py:169: item = Item.create(item_name)
are you creating a named, toplevel item here? that's ugly if you look at the namespace.
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode170
MoinMoin/items/ticket.py:170: update_meta = self.fqname.value
not sure why you assign to update_meta.
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode172
MoinMoin/items/ticket.py:172: item = Item.create(item_name)
you already did that some lines above
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode173
MoinMoin/items/ticket.py:173: item.modify(item.meta, data, refer_to=update_meta)
same here
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode182
MoinMoin/items/ticket.py:182: for rev in revs:
this is very expensive.
and it's not really what you are searching for. use a better query.
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/items/ticket.py#newcode185
MoinMoin/items/ticket.py:185: if rev.meta.get('refer_to') == self.fqname.value:
i told you about 5 times now not to use the name if you can avoid it.
use the itemid.
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html#newcode46
MoinMoin/templates/ticket/base.html:46: {% set mimetype = "application/x.moin.download" %}
why is it that mimetype?
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html#newcode47
MoinMoin/templates/ticket/base.html:47: <a href="{{ url_for('.show_item', item_name=e.fullname) }}"
don't use the name
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html#newcode51
MoinMoin/templates/ticket/base.html:51: <a href="{{ url_for('.download_item', item_name=e.fullname, mimetype=mimetype) }}">
don't use the name
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html#newcode52
MoinMoin/templates/ticket/base.html:52: {{ e.relname|truncate(maxchars, true, '..') }}
i think we already have a better "shorten" function somewhere.
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/modify.html
File MoinMoin/templates/ticket/modify.html (right):
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/modify.html#newcode40
MoinMoin/templates/ticket/modify.html:40: {% for e in files %}
for file in files
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/modify.html#newcode41
MoinMoin/templates/ticket/modify.html:41: {{ render_files(e) }}
if render_files' task is to render 1 file, then change its name to render_file.
Message from unknown
2015-06-11T20:52:41+00:00vipulurn:md5:4ce710ed4ce1ae88af809fea1790d0c9
Message from vipul.sharma20@gmail.com
2015-06-11T21:10:07+00:00vipulurn:md5:2577ec2ef8c132f187c1cfd7138ab2ba
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html#newcode47
MoinMoin/templates/ticket/base.html:47: <a href="{{ url_for('.show_item', item_name=e.fullname) }}"
On 2015/06/11 15:44:44, Thomas.J.Waldmann wrote:
> don't use the name
for rendering files, I've used IndexEntry instances of file items which stores fqname as 'fullname'. It's actually itemid only and not the name. it is of the form CompositeName(namespace=u'', field=u'itemid', value=u'3366e50dc6bf490abd97ade32eb5b634')
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket/base.html#newcode51
MoinMoin/templates/ticket/base.html:51: <a href="{{ url_for('.download_item', item_name=e.fullname, mimetype=mimetype) }}">
On 2015/06/11 15:44:44, Thomas.J.Waldmann wrote:
> don't use the name
same as said above
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py#newcode177
MoinMoin/items/ticket.py:177: query = And([Term(WIKINAME, app.cfg.interwikiname), Or([Prefix(NAME_EXACT, prefix)])])
getting files with prefix as itemid of ticket.
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py#newcode180
MoinMoin/items/ticket.py:180: for rev in revs:
revs contains items with name starting with "itemid/" not as expensive as previous one.
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py#newcode184
MoinMoin/items/ticket.py:184: file_fqname = CompositeName(rev.meta[NAMESPACE], ITEMID, rev.meta[ITEMID])
file_fqname is stored as 'fullname' in IndexEntry's instance hence the 'file.fullname' in templates is not name but itemid
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/templates/ticket/base.html#newcode47
MoinMoin/templates/ticket/base.html:47: class="{{ file.meta['contenttype']|contenttype_to_class }} moin-itemtype-{{ file.meta['itemtype'] }} moin-item">
for rendering files, I've used IndexEntry instances of file items which stores fqname as 'fullname'. It's actually itemid only and not the name. it is of the form CompositeName(namespace=u'', field=u'itemid', value=u'3366e50dc6bf490abd97ade32eb5b634')
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/templates/ticket/base.html#newcode50
MoinMoin/templates/ticket/base.html:50: {{ file.relname|truncate(maxchars, true, '..') }}
this is to limit the length of the filename displayed on the page. I think we have 'shorten' function for itemid shortening only.
Message from Thomas.J.Waldmann@gmail.com
2015-06-14T17:16:08+00:00Thomas.J.Waldmannurn:md5:54aeda93378fdc6f89110d3845e32fbe
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py#newcode165
MoinMoin/items/ticket.py:165: item_name = self.fqname.value + '/' + data_file.filename
can we get rid of the NAME?
it is ok to keep the filename somewhere (add a new key constant for that) so you can reconstruct it on download, but I'ld prefer if you avoid spoiling the namespace with that, so please use an unnamed item if possible (NAME == []).
btw, is data_file.filename unicode or str?
https://codereview.appspot.com/246020043/diff/40001/MoinMoin/items/ticket.py#newcode177
MoinMoin/items/ticket.py:177: query = And([Term(WIKINAME, app.cfg.interwikiname), Or([Prefix(NAME_EXACT, prefix)])])
why are you acting on the NAME(_EXACT) field at all?
i advised you to put the main item's itemid into refers_to field of all comments or related file uploads, so you can query it via whoosh - why don't you just do that? a precise match is way more efficient than a prefix match.
why do you have Or() there?
Message from unknown
2015-06-19T15:28:27+00:00vipulurn:md5:b11de640c4f0510b4446d635eeba2dd3
Message from vipul.sharma20@gmail.com
2015-06-19T15:44:00+00:00vipulurn:md5:841c4908b74bdb61f006f2a0571bd34f
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py#newcode165
MoinMoin/items/ticket.py:165: item_name = self.fqname.value + '/' + data_file.filename
I don't think we can get rid of this name. We'll have to use a name in Item.create(item_name).
Here, data_file.filename is unicode
Message from Thomas.J.Waldmann@gmail.com
2015-06-23T16:02:21+00:00Thomas.J.Waldmannurn:md5:7536ea273cfda28b6b29947b447d4bda
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py#newcode165
MoinMoin/items/ticket.py:165: item_name = self.fqname.value + '/' + data_file.filename
On 2015/06/19 15:44:00, vipul wrote:
> I don't think we can get rid of this name. We'll have to use a name in
> Item.create(item_name).
>
> Here, data_file.filename is unicode
IIRC at another place (blog or item creation), the name is first given, but later removed, so that would be a workaround.
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py#newcode170
MoinMoin/items/ticket.py:170: item.modify({}, data, contenttype_guessed=contenttype, refer_to=self.fqname.value)
what's in self.fqname.value?
make sure you use a itemid for refer_to.
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py#newcode176
MoinMoin/items/ticket.py:176: prefix = self.fqname.value + '/'
this looks rather like fqname.value is a name.
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/items/ticket.py#newcode177
MoinMoin/items/ticket.py:177: query = And([Term(WIKINAME, app.cfg.interwikiname), Term(REFER_TO, self.fqname.value)])
refer_to must be itemid. is it that?
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/storage/middleware/indexing.py
File MoinMoin/storage/middleware/indexing.py (right):
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/storage/middleware/indexing.py#newcode332
MoinMoin/storage/middleware/indexing.py:332: REFER_TO: ID(stored=True),
grammar question: isn't it rather REFERS_TO?
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/templates/ticket/base.html#newcode46
MoinMoin/templates/ticket/base.html:46: <a href="{{ url_for('.show_item', item_name=file.fullname) }}"
fullname sounds like a name.
you should use the itemid of the file.
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/templates/ticket/base.html#newcode49
MoinMoin/templates/ticket/base.html:49: <a href="{{ url_for('.download_item', item_name=file.fullname) }}" title="{{ file.relname }}">
same here.
https://codereview.appspot.com/246020043/diff/160001/MoinMoin/templates/ticket/base.html#newcode50
MoinMoin/templates/ticket/base.html:50: {{ file.relname|truncate(maxchars, true, '..') }}
didn't you find the shorten function I mentioned already in a past review?
Message from vipul.sharma20@gmail.com
2015-06-27T07:49:35+00:00vipulurn:md5:c33e960e9a4b9fc052bbf21c5a6b3471
add TODO and shorten function
Message from vipul.sharma20@gmail.com
2015-06-27T08:21:40+00:00vipulurn:md5:e61a0ca33b10298546d3b6d12d29fc5f
add TODO and shorten function
Message from vipul.sharma20@gmail.com
2015-06-27T08:25:06+00:00vipulurn:md5:c61aefc8bafd97ee2bdae13dfdc50fd7
add TODO and shorten function
Message from unknown
2015-06-27T08:26:10+00:00vipulurn:md5:fa68d2ef87a61dffaffed009f4d37a43
Message from vipul.sharma20@gmail.com
2015-06-27T08:26:12+00:00vipulurn:md5:b6927dbdcdaab4ef2bc2f7d634ee510d
add TODO and shorten function
Message from vipul.sharma20@gmail.com
2015-06-27T08:44:54+00:00vipulurn:md5:a639e7e569383c11444b4ebc825f6339
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/templates/ticket/base.html
File MoinMoin/templates/ticket/base.html (right):
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/templates/ticket/base.html#newcode44
MoinMoin/templates/ticket/base.html:44: <a href="{{ url_for('.show_item', item_name=file.fullname) }}"
for rendering files, I've used IndexEntry instances of file items which stores fqname as 'fullname'. It's actually itemid only and not the name. it is of the form CompositeName(namespace=u'', field=u'itemid', value=u'3366e50dc6bf490abd97ade32eb5b634')
Message from sksaurabhkathpalia@gmail.com
2015-07-01T05:56:48+00:00sksaurabhkathpaliaurn:md5:700e62bd6ed57034b444a0e463b1db74
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py#newcode178
MoinMoin/items/ticket.py:178: prefix = self.fqname.value + '/'
is self.fqname.value itemid or name?
The same questions is asked in the last version of this CR also. You should atleast reply to the previous question before you update the CR
Message from vipul.sharma20@gmail.com
2015-07-01T07:17:20+00:00vipulurn:md5:84b161b3905821fb89f4f23926470992
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py#newcode178
MoinMoin/items/ticket.py:178: prefix = self.fqname.value + '/'
On 2015/07/01 05:56:48, sksaurabhkathpalia wrote:
> is self.fqname.value itemid or name?
> The same questions is asked in the last version of this CR also. You should
> atleast reply to the previous question before you update the CR
its a name initially, in the ticket create view as there is no itemid generated. Once, the ticket is created self.fqname.value is an itemid
Message from sksaurabhkathpalia@gmail.com
2015-07-01T07:19:02+00:00sksaurabhkathpaliaurn:md5:e722a1f36f542a25ecf5a8051be305bf
On 2015/07/01 07:17:20, vipul wrote:
> https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py
> File MoinMoin/items/ticket.py (right):
>
> https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py#newcode178
> MoinMoin/items/ticket.py:178: prefix = self.fqname.value + '/'
> On 2015/07/01 05:56:48, sksaurabhkathpalia wrote:
> > is self.fqname.value itemid or name?
> > The same questions is asked in the last version of this CR also. You should
> > atleast reply to the previous question before you update the CR
>
> its a name initially, in the ticket create view as there is no itemid generated.
> Once, the ticket is created self.fqname.value is an itemid
This code doesn't work see logs of IRC, I have explained it there
Message from Thomas.J.Waldmann@gmail.com
2015-07-01T08:54:23+00:00Thomas.J.Waldmannurn:md5:4abd83d393582a084a09c96673465203
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/constants/keys.py#newcode60
MoinMoin/constants/keys.py:60: REFERS_TO = u"refers_to"
hmm, should that be in the list of immutable keys? see below.
Message from unknown
2015-07-01T09:01:52+00:00vipulurn:md5:2cc571a776117271cc8f9b5773286563
Message from vipul.sharma20@gmail.com
2015-07-01T09:09:27+00:00vipulurn:md5:9c54bf4e6b14cb12030e4751f6073b21
please review the new CR. I've added a function which will check if both name and id have been created, then use itemid and change previously created items (which used ticket's name) to use itemid of the ticket.
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/constants/keys.py#newcode60
MoinMoin/constants/keys.py:60: REFERS_TO = u"refers_to"
On 2015/07/01 08:54:23, Thomas.J.Waldmann wrote:
> hmm, should that be in the list of immutable keys? see below.
Acknowledged.
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py#newcode178
MoinMoin/items/ticket.py:178: prefix = self.fqname.value + '/'
On 2015/07/01 05:56:48, sksaurabhkathpalia wrote:
> is self.fqname.value itemid or name?
> The same questions is asked in the last version of this CR also. You should
> atleast reply to the previous question before you update the CR
ok, the problem is if I use self.fqname.value in create view it will have name as prefix in the file item_name but from the next time i.e. during updation of ticket item, it will now have itemid and the files with name having ticket's initial name as prefix will not be displayed. I am creating a new CR to fix this
Message from sksaurabhkathpalia@gmail.com
2015-07-01T11:20:04+00:00sksaurabhkathpaliaurn:md5:79581e180e8f61f092f01a3b98a94f60
On 2015/07/01 09:09:27, vipul wrote:
> please review the new CR. I've added a function which will check if both name
> and id have been created, then use itemid and change previously created items
> (which used ticket's name) to use itemid of the ticket.
>
> https://codereview.appspot.com/246020043/diff/260001/MoinMoin/constants/keys.py
> File MoinMoin/constants/keys.py (right):
>
> https://codereview.appspot.com/246020043/diff/260001/MoinMoin/constants/keys.py#newcode60
> MoinMoin/constants/keys.py:60: REFERS_TO = u"refers_to"
> On 2015/07/01 08:54:23, Thomas.J.Waldmann wrote:
> > hmm, should that be in the list of immutable keys? see below.
>
> Acknowledged.
>
> https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py
> File MoinMoin/items/ticket.py (right):
>
> https://codereview.appspot.com/246020043/diff/260001/MoinMoin/items/ticket.py#newcode178
> MoinMoin/items/ticket.py:178: prefix = self.fqname.value + '/'
> On 2015/07/01 05:56:48, sksaurabhkathpalia wrote:
> > is self.fqname.value itemid or name?
> > The same questions is asked in the last version of this CR also. You should
> > atleast reply to the previous question before you update the CR
>
> ok, the problem is if I use self.fqname.value in create view it will have name
> as prefix in the file item_name but from the next time i.e. during updation of
> ticket item, it will now have itemid and the files with name having ticket's
> initial name as prefix will not be displayed. I am creating a new CR to fix this
Yeah now its working
Message from sksaurabhkathpalia@gmail.com
2015-07-01T11:28:07+00:00sksaurabhkathpaliaurn:md5:d39e3aeb82a9dbf0d00cfe15c85cac08
https://codereview.appspot.com/246020043/diff/280001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/280001/MoinMoin/items/ticket.py#newcode174
MoinMoin/items/ticket.py:174: print name
remove this print name
Message from unknown
2015-07-01T11:43:25+00:00vipulurn:md5:abeb9a86099a3bc444a061219ccdba2b
Message from sksaurabhkathpalia@gmail.com
2015-07-01T15:55:02+00:00sksaurabhkathpaliaurn:md5:a60754450cd5b94fb1dc244652315453
https://codereview.appspot.com/246020043/diff/340001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/340001/MoinMoin/items/ticket.py#newcode171
MoinMoin/items/ticket.py:171: for rev in revs:
In this you are creating new items that have name with prefix ITEMID of ticket but you are not deleting the alread existing items which were created should be deleted.
Also I think the better method would be to rename the item made for files
Message from unknown
2015-07-02T11:45:23+00:00vipulurn:md5:cacb2e9cc16e2cbce2cf355c3cd0d761
Message from vipul.sharma20@gmail.com
2015-07-02T12:39:17+00:00vipulurn:md5:c862b9ddf6dde85158e468a0d0e68377
https://codereview.appspot.com/246020043/diff/340001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/340001/MoinMoin/items/ticket.py#newcode171
MoinMoin/items/ticket.py:171: for rev in revs:
On 2015/07/01 15:55:01, sksaurabhkathpalia wrote:
> In this you are creating new items that have name with prefix ITEMID of ticket
> but you are not deleting the alread existing items which were created should be
> deleted.
> Also I think the better method would be to rename the item made for files
I've fixed this in the new patch
Message from sksaurabhkathpalia@gmail.com
2015-07-02T12:45:50+00:00sksaurabhkathpaliaurn:md5:96768c0c924c6f2e347109f7fb9cebeb
On 2015/07/02 12:39:17, vipul wrote:
> https://codereview.appspot.com/246020043/diff/340001/MoinMoin/items/ticket.py
> File MoinMoin/items/ticket.py (right):
>
> https://codereview.appspot.com/246020043/diff/340001/MoinMoin/items/ticket.py#newcode171
> MoinMoin/items/ticket.py:171: for rev in revs:
> On 2015/07/01 15:55:01, sksaurabhkathpalia wrote:
> > In this you are creating new items that have name with prefix ITEMID of ticket
> > but you are not deleting the alread existing items which were created should
> be
> > deleted.
> > Also I think the better method would be to rename the item made for files
>
> I've fixed this in the new patch
This looks Ok to me. But once ask Thomas if the item should be renamed or deleted
Message from Thomas.J.Waldmann@gmail.com
2015-07-14T16:17:20+00:00Thomas.J.Waldmannurn:md5:4d6a685ca2a177deab3f844fe4aaeb16
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode168
MoinMoin/items/ticket.py:168: query = And([Term(WIKINAME, app.cfg.interwikiname), Term(REFERS_TO, self.meta[NAME])])
.meta[NAME] is a list. does the query Term support getting a list here?
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode178
MoinMoin/items/ticket.py:178: item.modify({}, rev.meta[CONTENT], refers_to=self.meta[ITEMID])
so, what's the goal of this?
how is having the ITEMID inside the name helpful?
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode184
MoinMoin/items/ticket.py:184: pass
that "else: pass" is rather superfluous.
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode193
MoinMoin/items/ticket.py:193: refers_to = self.meta[ITEMID]
same question as above.
Message from unknown
2015-07-17T12:13:20+00:00vipulurn:md5:12bc83bf2f195063460ac9b185f122f3
Message from vipul.sharma20@gmail.com
2015-07-17T12:17:29+00:00vipulurn:md5:62a1654453134352bfccbff79e8b5246
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py
File MoinMoin/items/ticket.py (right):
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode168
MoinMoin/items/ticket.py:168: query = And([Term(WIKINAME, app.cfg.interwikiname), Term(REFERS_TO, self.meta[NAME])])
On 2015/07/14 16:17:20, Thomas.J.Waldmann wrote:
> .meta[NAME] is a list. does the query Term support getting a list here?
Yes
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode178
MoinMoin/items/ticket.py:178: item.modify({}, rev.meta[CONTENT], refers_to=self.meta[ITEMID])
On 2015/07/14 16:17:20, Thomas.J.Waldmann wrote:
> so, what's the goal of this?
>
> how is having the ITEMID inside the name helpful?
discussed in irc meeting (https://moinmo.in/MoinMoinChat/Logs/moin-dev/2015-07-14)
line 130
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode184
MoinMoin/items/ticket.py:184: pass
On 2015/07/14 16:17:20, Thomas.J.Waldmann wrote:
> that "else: pass" is rather superfluous.
will correct this
https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py#newcode193
MoinMoin/items/ticket.py:193: refers_to = self.meta[ITEMID]
On 2015/07/14 16:17:20, Thomas.J.Waldmann wrote:
> same question as above.
discussed in irc meeting (https://moinmo.in/MoinMoinChat/Logs/moin-dev/2015-07-14)
line 130
Message from Thomas.J.Waldmann@gmail.com
2015-08-11T15:34:30+00:00Thomas.J.Waldmannurn:md5:90e134ead451cbff331b1eeb81e784ce
ok, fix the one thing you said in last reply, give it a final test and if it works, commit.
Message from vipul.sharma20@gmail.com
2015-08-12T07:10:15+00:00vipulurn:md5:1ac682e172aa4774ab3d4faba8699544
On 2015/08/11 15:34:30, Thomas.J.Waldmann wrote:
> ok, fix the one thing you said in last reply, give it a final test and if it
> works, commit.
I've fixed that in my last CR already. I will give it a final test and commit