Issue 246020043: file upload
|
Created:
9 years, 6 months ago by vipul Modified:
9 years, 4 months ago Reviewers:
thomas.j.waldmann Visibility:
Public. |
Descriptionfile upload
implemented file upload feature by creating a named top level item. Every item uploaded is referred to the ticket's itemid using a new meta field "refers_to". This field is indexed in whoosh.
Patch Set 1 : file upload #
Total comments: 17
Patch Set 2 : file upload: improved query #
Total comments: 7
Patch Set 3 : file upload #
Total comments: 9
Patch Set 4 : add TODO and shorten function #
Total comments: 6
Patch Set 5 : fixed issue of ticket-name prefix in file's item_name #
Total comments: 1
Patch Set 6 : now files uploaded in create view does not disappear in update view #
Total comments: 2
Patch Set 7 : delete duplicate-old-file item, using name of ticket. #
Total comments: 8
Patch Set 8 : removed "else: pass" #
MessagesTotal messages: 25
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#... 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#... 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
Sign in to reply to this message.
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#... 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#... 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#... 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#... 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#... 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#... 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#... 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... File MoinMoin/templates/ticket/base.html (right): https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket... 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... 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... 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... 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... File MoinMoin/templates/ticket/modify.html (right): https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket... MoinMoin/templates/ticket/modify.html:40: {% for e in files %} for file in files https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket... 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.
Sign in to reply to this message.
https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket... File MoinMoin/templates/ticket/base.html (right): https://codereview.appspot.com/246020043/diff/20001/MoinMoin/templates/ticket... 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... 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#... 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#... 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#... 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... File MoinMoin/templates/ticket/base.html (right): https://codereview.appspot.com/246020043/diff/40001/MoinMoin/templates/ticket... 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... 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.
Sign in to reply to this message.
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#... 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#... 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?
Sign in to reply to this message.
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... 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
Sign in to reply to this message.
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... 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... 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... 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... 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/middlew... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/246020043/diff/160001/MoinMoin/storage/middlew... 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/ticke... File MoinMoin/templates/ticket/base.html (right): https://codereview.appspot.com/246020043/diff/160001/MoinMoin/templates/ticke... 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/ticke... 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/ticke... 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?
Sign in to reply to this message.
add TODO and shorten function
Sign in to reply to this message.
add TODO and shorten function
Sign in to reply to this message.
add TODO and shorten function
Sign in to reply to this message.
add TODO and shorten function
Sign in to reply to this message.
https://codereview.appspot.com/246020043/diff/260001/MoinMoin/templates/ticke... File MoinMoin/templates/ticket/base.html (right): https://codereview.appspot.com/246020043/diff/260001/MoinMoin/templates/ticke... 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')
Sign in to reply to this message.
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... 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
Sign in to reply to this message.
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... 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
Sign in to reply to this message.
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... > 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
Sign in to reply to this message.
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.... MoinMoin/constants/keys.py:60: REFERS_TO = u"refers_to" hmm, should that be in the list of immutable keys? see below.
Sign in to reply to this message.
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.... 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... 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
Sign in to reply to this message.
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.... > 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... > 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
Sign in to reply to this message.
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... MoinMoin/items/ticket.py:174: print name remove this print name
Sign in to reply to this message.
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... 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
Sign in to reply to this message.
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... 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
Sign in to reply to this message.
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... > 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
Sign in to reply to this message.
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... 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... 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... MoinMoin/items/ticket.py:184: pass that "else: pass" is rather superfluous. https://codereview.appspot.com/246020043/diff/360001/MoinMoin/items/ticket.py... MoinMoin/items/ticket.py:193: refers_to = self.meta[ITEMID] same question as above.
Sign in to reply to this message.
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... 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... 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... 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... 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
Sign in to reply to this message.
ok, fix the one thing you said in last reply, give it a final test and if it works, commit.
Sign in to reply to this message.
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
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
