Merge lp:~openerp-dev/openobject-client-web/export into lp:openobject-client-web/trunk

[Kunal Chavda]

[Xavier (Open ERP) (xmo-deactivatedaccount)]

0. Please perform reformattings (even automated by your IDE) in different revisions than you perform actual work, mixing them makes reviews and reading history harder than it should (you are not limited in the number of revisions you use, so don't worry about that)

1. If we have only one value (namely CSV), there is no need to display the control/section at all since there is no choice and csv is the default output format. Thus instead of a sequence of export format you could just as well use a boolean `enable_excel_export` (or something like that) flag and only display the control if it's `True` (as far as I know, we're not expected to add new export formats anytime soon). That flag would simply be set if we can import xlwt, unset otherwise

2. This means you can also move the xlwt import to the toplevel of the module (instead of within functions) wrapped in an except

3. Please use precise as exception catch as you can: the error we're looking for here is specifically ImportError, we don't want to "eat" exceptions due to e.g. a broken xlwt installation

review: Needs Fixing

[Navrang Oza (noz-tiny)]

> 0. Please perform reformattings (even automated by your IDE) in different
> revisions than you perform actual work, mixing them makes reviews and reading
> history harder than it should (you are not limited in the number of revisions
> you use, so don't worry about that)
>
> 1. If we have only one value (namely CSV), there is no need to display the
> control/section at all since there is no choice and csv is the default output
> format. Thus instead of a sequence of export format you could just as well use
> a boolean `enable_excel_export` (or something like that) flag and only display
> the control if it's `True` (as far as I know, we're not expected to add new
> export formats anytime soon). That flag would simply be set if we can import
> xlwt, unset otherwise

Conclusion what I understood: If xlwt is not installed then only CSV option will be there and as boolean option "Excel" option will be available. When user will click for Excel then error message will raise "Please Install xlwt Library ......". RIGHT ??

> 2. This means you can also move the xlwt import to the toplevel of the module
> (instead of within functions) wrapped in an except

If we import xlwt to the top level, then how will we check that it is installed or not ?
How can we raise error ?
We have to keep in try/catch somewhere.

> 3. Please use precise as exception catch as you can: the error we're looking
> for here is specifically ImportError, we don't want to "eat" exceptions due to
> e.g. a broken xlwt installation

[Xavier (Open ERP) (xmo-deactivatedaccount)]

> Conclusion what I understood: If xlwt is not installed then only CSV option will be there and as boolean option "Excel" option will be available. When user will click for Excel then error message will raise "Please Install xlwt Library ......". RIGHT ??

I don't think so, I'd rather have no option at all if only CSV export is available, an option for which you can't make any choice is useless cognitive burden for the user.

I will ask Aline about it though, just to be sure.

> If we import xlwt to the top level, then how will we check that it is installed or not ?

try:
    import xlwt
    xls_export_available = True
except ImportError:
    xls_export_available = False

> How can we raise error ?
We don't raise error, that's the point.

> We have to keep in try/catch somewhere.
Sure, but that can be at the toplevel.

[Navrang Oza (noz-tiny)]

> > Conclusion what I understood: If xlwt is not installed then only CSV option
> will be there and as boolean option "Excel" option will be available. When
> user will click for Excel then error message will raise "Please Install xlwt
> Library ......". RIGHT ??
>
> I don't think so, I'd rather have no option at all if only CSV export is
> available, an option for which you can't make any choice is useless cognitive
> burden for the user.

That is also true. Actually in GTK there is not Excel option available.

>
> I will ask Aline about it though, just to be sure.
>
> > If we import xlwt to the top level, then how will we check that it is
> installed or not ?
>
> try:
> import xlwt
> xls_export_available = True
> except ImportError:
> xls_export_available = False
>
> > How can we raise error ?
> We don't raise error, that's the point.
>
> > We have to keep in try/catch somewhere.
> Sure, but that can be at the toplevel.

[lp:~openerp-dev/openobject-client-web/export]

3931. By Kunal Chavda on 2010-11-24

[IMP] Improved code.

[Xavier (Open ERP) (xmo-deactivatedaccount)]

There is no need for the following code:

    % else:
        <td>
            <input type="radio" id="export_as" name="export_as" value="csv" checked/>
        </td>
        <td>
            ${_("Export as CSV")}
        </td>

If Excel export is not available, csv is default export format, so clicking export will generate a CSV export.

Also, as I asked noz (i think, though I may have forgotten), please merge the two options fieldsets, aline finds there is no need for two different ones for the same things.

review: Needs Fixing

[lp:~openerp-dev/openobject-client-web/export]

3932. By Kunal Chavda on 2010-11-25

[IMP] Improved Code.

[Kunal Chavda (kunal-chavda)]

Hello Sir,

As per your suggestion I have fixed changes.

Thanks.

[Navrang Oza (noz-tiny)]

I have checked, and its working fine.
You can merge it.

review: Approve

=== modified file 'addons/openerp/controllers/impex.py'
--- addons/openerp/controllers/impex.py	2010-11-09 09:24:23 +0000
+++ addons/openerp/controllers/impex.py	2010-11-25 06:20:12 +0000
@@ -39,6 +39,13 @@
 from openobject.tools import expose, redirect, ast
 
 
+try:
+    import xlwt
+    xls_export_available = True
+except:
+    xls_export_available = False
+
+
 def datas_read(ids, model, fields, context=None):
     ctx = dict((context or {}), **rpc.session.context)
     return rpc.RPCProxy(model).export_data(ids, fields, ctx)
@@ -71,7 +78,7 @@
         raise common.message(_("Operation failed\nI/O error")+"(%s)" % (errno,))
 
 def _fields_get_all(model, views, context=None):
-    
+
     context = context or {}
 
     def parse(root, fields):
@@ -117,7 +124,7 @@
     @expose(template="/openerp/controllers/templates/exp.mako")
     def exp(self, **kw):
         params, data = TinyDict.split(kw)
-        
+
         ctx = dict((params.context or {}), **rpc.session.context)
 
         views = {}
@@ -143,11 +150,12 @@
         new_list = listgrid.List(name='_terp_list', model='ir.exports', view=view, ids=None,
                                  domain=[('resource', '=', params.model)],
                                  context=ctx, selectable=1, editable=False, pageable=False, impex=True)
-
+      
+		
 
         return dict(new_list=new_list, model=params.model, ids=params.ids, ctx=ctx,
                     search_domain=params.search_domain, source=params.source,
-                    tree=tree)
+                    tree=tree, xls_export_available=xls_export_available)
 
     @expose()
     def save_exp(self, **kw):
@@ -178,23 +186,23 @@
     @expose('json')
     def get_fields(self, model, prefix='', name='', field_parent=None, **kw):
 
-        is_importing = kw.get('is_importing', False)        
-        
+        is_importing = kw.get('is_importing', False)
+
         try:
             ctx = ast.literal_eval(kw['context'])
         except:
             ctx = {}
-            
+
         ctx.update(**rpc.session.context)
 
         try:
             views = ast.literal_eval(kw['views'])
         except:
             views = {}
-            
+
 
         fields = _fields_get_all(model, views, ctx)
-        
+
         fields.update({'id': {'string': 'ID'}, 'db_id': {'string': 'Database ID'}})
 
         fields_order = fields.keys()
@@ -257,7 +265,7 @@
     def get_namelist(self, **kw):
 
         params, data = TinyDict.split(kw)
-        
+
         ctx = dict((params.context or {}), **rpc.session.context)
 
         id = params.id
@@ -343,11 +351,8 @@
             params.fields2 = fields
 
         if export_as == 'xls':
-            try:
-                import xlwt
-            except ImportError:
-                  raise common.warning(_('Please Install xlwt Library.\nTo create spreadsheet files compatible with MS Excel.'), _('Import Error.'))
 
+            import xlwt
             ezxf = xlwt.easyxf
 
             fp = StringIO.StringIO()
@@ -380,9 +385,9 @@
     @expose(template="/openerp/controllers/templates/imp.mako")
     def imp(self, **kw):
         params, data = TinyDict.split(kw)
-        
+
         ctx = dict((params.context or {}), **rpc.session.context)
-        
+
         views = {}
         if params.view_mode and params.view_ids:
             for i, view in enumerate(params.view_mode):
@@ -408,11 +413,11 @@
 
         _fields = {}
         _fields_invert = {}
-        
+
         fields = dict(rpc.RPCProxy(params.model).fields_get(False, rpc.session.context),
                       id={'type': 'char', 'string': 'ID'},
                       db_id={'type': 'char', 'string': 'Database ID'})
-        
+
         def model_populate(fields, prefix_node='', prefix=None, prefix_value='', level=2):
             def str_comp(x,y):
                 if x<y: return 1
@@ -426,7 +431,7 @@
                             and (not fields[field].get('readonly')\
                             or not dict(fields[field].get('states', {}).get(
                             'draft', [('readonly', True)])).get('readonly',True)):
-                    
+
                     st_name = prefix_value+fields[field]['string'] or field
                     _fields[prefix_node+field] = st_name
                     _fields_invert[st_name] = prefix_node+field
@@ -474,7 +479,7 @@
         data = list(csv.reader(input, quotechar=str(csvdel), delimiter=str(csvsep)))[int(csvskip):]
         datas = []
         ctx = dict(rpc.session.context)
-        
+
         if not isinstance(fields, list):
             fields = [fields]
 

=== modified file 'addons/openerp/controllers/templates/exp.mako'
--- addons/openerp/controllers/templates/exp.mako	2010-11-09 09:04:31 +0000
+++ addons/openerp/controllers/templates/exp.mako	2010-11-25 06:20:12 +0000
@@ -7,9 +7,9 @@
 
     <script type="text/javascript">
         function add_fields(){
-        
+
             var tree = treeGrids['${tree.name}'];
-            
+
             var fields = tree.selection;
             var select = openobject.dom.get('fields');
 
@@ -28,7 +28,7 @@
                 select.options.add(new Option(text, id));
             });
         }
-		
+
         function open_savelist(id) {
             var elem = openobject.dom.get(id);
             elem.style.display = elem.style.display == 'none' ? '' : 'none';
@@ -56,7 +56,7 @@
                 });
             }
         }
-        
+
         function do_select(id, src) {
             openobject.dom.get('fields').innerHTML = '';
             var model = openobject.dom.get('_terp_model').value;
@@ -87,7 +87,7 @@
             var id = boxes[0].value;
             form.action = openobject.http.getURL('/openerp/impex/delete_listname', {'_terp_id' : id});
   			form.submit();
-  			
+
         }
 
         function reload(name_list) {
@@ -148,7 +148,7 @@
                     </tr>
                 </table>
             </td>
-        </tr>        
+        </tr>
         % if new_list.ids:
         <tr>
             <td class="side_spacing">
@@ -211,13 +211,13 @@
             </td>
         </tr>
         <tr>
-            <td class="side_spacing">            
+            <td class="side_spacing">
                 <div id="savelist" style="display: none">
                     <fieldset>
                         <legend>${_("Save List")}</legend>
                         <table>
-                            <tr>                           
-                                <td class="label">${_("Name of This Export:")}</td>                            
+                            <tr>
+                                <td class="label">${_("Name of This Export:")}</td>
                                 <td>
                                     <input type="text" id="savelist_name" name="savelist_name"/>
                                 </td>
@@ -226,41 +226,36 @@
                                 </td>
                             </tr>
                         </table>
-                    </fieldset>         
-                </div>   
+                    </fieldset>
+                </div>
             </td>
-        </tr>        
+        </tr>
         <tr>
-            <td class="side_spacing">
-                <fieldset>
-                    <legend>${_("Options")}</legend>
+        	<td class="side_spacing">
+        		<fieldset title="Restricts the number of exportable fields to ensure you will be able to import your data back in OpenERP">
+                    <legend>${_("Select Options to Export")}</legend>
                     <table>
-                        <tr>
-                            <td>
-                                <select id="export_as" name="export_as">
+                    	<tr>
+                             % if xls_export_available == True:
+                            <td style="padding-right: 8px;">
+                                <select style="height:18px" id="export_as" name="export_as">
                                     <option value="csv">${_("Export as CSV")}</option>
                                     <option value="xls">${_("Export as Excel")}</option>
                                 </select>
                             </td>
+                            % else:
+                            <td>
+                            	<input type="hidden" id="export_as" name="export_as" value="csv"/>
+                            </td>
+                            % endif
                             <td>
                                 <input type="checkbox" class="checkbox" name="add_names" checked="checked"/>
                             </td>
-                            <td>${_("Add field names")}</td>
-                        </tr>
-                    </table>
-                </fieldset>
-            </td>
-        </tr>
-        <tr>
-        	<td class="side_spacing">
-        		<fieldset title="Restricts the number of exportable fields to ensure you will be able to import your data back in OpenERP">
-                    <legend>${_("Select an Option to Export")}</legend>
-                    <table>
-                        <tr>
-                            <td>
+                            <td style="padding-left:3px">${_("Add field names")}</td>
+                            <td style="padding-left:8px">
                                 <input type="checkbox" class="checkbox" name="import_compat" id="import_compat"/>
                             </td>
-                            <td><label for="import_compat"
+                            <td style="padding-left:3px"><label for="import_compat"
                                     >${_("Import Compatible")}</label></td>
                         </tr>
                     </table>