openerp-community-reviewer team mailing list archive

[Graeme Gellatly <gdgellatly@xxxxxxxxx>]

Hi,

Thanks for this, major annoyance of mine for some time.

I make some comments here.

1. I am not agreed this is a bug, perhaps if main openerp-trunk implemented this it would be worth backporting, it is changing functionality.
2. I am not agreed with its placement when it could be made an extension module.

has_key() is deprecated use 'in' instead.
I am not sure about using read for getting name.
I think calling name_get() would be better and cover far more models
Also read returns the results in id ascending order, however depending on the order the ids are passed this may not make sense.  But actually there is no gaurentee of contiguous and sequential ids or objects being passed so the whole multiple naming technique has issues.

Maybe there is a better filenaming convention for multiple files, maybe a date, model description, and number of documents inside.

For the single filename I think it would be far better to use something like the ir_attachment attchment save as field in report_xml definitions.  This allows each company to define the name they want rather than guessing at what they might want but maybe that is wanting too much.

As a bug what happens if 'name' contains '/' for example.  I know most browsers will ordinarily handle this but it seems relying on an implicit handling of bad filenames by an external program is unnecessarily risky, at least for the common cases.

It might be better just to perform name_get on the first and last active_ids (or just the first in a list of 1)

file_name = '-'.join(list_of_names) - it will be one or two only but that gets rid of a for loop, an enumerate, an if statement, a read on all the objects between 1 and n

I also feel the whole diff could be broken out into a new function - it is only doing one thing which is getting a filename.

 

-- 
https://code.launchpad.net/~yann-papouin/ocb-web/6.1-bug-1261322-useful-report-filename/+merge/200007
Your team OpenERP Community Backports Team is requested to review the proposed merge of lp:~yann-papouin/ocb-web/6.1-bug-1261322-useful-report-filename into lp:ocb-web/6.1.