Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIG] web_export_view: Migration to 11.0 #798

Merged
merged 21 commits into from
Aug 15, 2018

Conversation

JBF91
Copy link
Contributor

@JBF91 JBF91 commented Nov 14, 2017

A proposal for the Migration of web_export_view to 11.0 . Some notes about the changes and the problems I've found:

Odoo 11.0 changes some attrs in the rendering of HTML. We do not longer have auxiliar attributes like "data-id" or "data-field" very useful in the older "web_export_view" wich uses "data-field" attrs to get the column keys. I've changed it for a column index (fast and ugly workaround, but do the trick).

CONS: Couldn't get the columns Strings in the XLS header, it exports the technical name of the field instead the string.

PD: As @pedrobaeza recommend me I was triying to migrate it with the new 8.0 feature, wich exports all the dataset if all the rows are checked instead of only the first page (just like the odoo export base behaviour) but couldn't find the right way to get all item id's since we don't longer have "data-id".

Changes resume:

  • Update objects attributes to new JS structure
  • Update function names (redraw -> _redraw)
  • Update JQuery selectors to get the list in the CSS classes.

@JBF91 JBF91 mentioned this pull request Nov 14, 2017
68 tasks
@yajo yajo added this to the 11.0 milestone Nov 14, 2017
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but no runbot yet to test. Functionally OK too.

Thanks for the good explanation on the migration job, BTW.

Remember to squash all translation commits in one please.

@simahawk
Copy link
Contributor

quick tests on runbot by exporting some contacts: works fine ;)

@simahawk
Copy link
Contributor

@JBF91 as for the labels, what about these 2 options:

  1. quick n dirty: get the closest TH and grab them by position
  2. call fields_get passing exported fields and collect all fields_info['string']

@JBF91
Copy link
Contributor Author

JBF91 commented Nov 14, 2017

I think I've squash properly the translation commits (thanks to @yajo )

@simahawk I've get the title row (quick and dirty way) but dont like the abuse of jQuery selectors. Tried and failed the other way, maybe tomorrow I give it another try.

@simahawk
Copy link
Contributor

simahawk commented Nov 15, 2017

@JBF91 I see... In any case the whole JS here is kind of a trick ;)

Maybe you can collect fields into self.fieldnames then at the end you can wrap the call to the controller into something like this:

this._rpc({
  model: view.modelName,
  method: 'fields_get',
  args: [self.fieldnames],
}).done(function (export_fields) {
  session.get_file({
    url: '/web/export/xls_view',
    data: {data: JSON.stringify({
        model: view.modelName,
        headers: _.map(export_fields, function(field){ return field['string']; }),
        rows: export_rows
    })},
    complete: $.unblockUI
  });
})

kind of... :)

Anyway, you have a "plan B" in case the JS fails: if you still send the header as field names you can call fields_get on controller side and inject the 1st line before calling xls creation.

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code ok

Copy link
Contributor

@benwillig benwillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review and test 👍

@pedrobaeza
Copy link
Member

Is this exporting the whole set or not yet? It's important to keep that behavior, as Odoo export works that way and now people expect this also to work that way.

@hbrunn
Copy link
Member

hbrunn commented Mar 1, 2018

I hacked the all records export into 8 in #785, and then had to fix a lot of stuff in https://github.com/OCA/web/pull/816/files in case that helps (probably won't given the js changes)

@okuryan
Copy link

okuryan commented Mar 9, 2018

What is progress on this migration?

@JBF91
Copy link
Contributor Author

JBF91 commented Mar 9, 2018

@okuryan Well this was working fine (two months ago) afaik, just a thing that @pedrobaeza suggested, to add an extra feature that exports all records (everypage) if all the records in the main page are selected.

(Don't know if that's a good idea, to give every user who has access to a list of records exports it so easily, you already have base export functionality for that.)

@fr33co
Copy link

fr33co commented Mar 23, 2018

I fix the column problem with this (11.0 Enterprise):

export_columns_names.push(view.$el.find('.o_list_view > thead > tr > .o_column_sortable:eq('+column_index+')')[0].textContent);

Change th[title] with .o_column_sortable:eq

@CasVissers-360ERP
Copy link

CasVissers-360ERP commented Apr 18, 2018

Module works when exporting as admin but results in a traceback when exporting as a user:

`Error:
TypeError: view.$el.find(...)[0] is undefined

https://customer.com/web/content/32110-3cbb18d/web.assets_backend.js:3395
Traceback:
on_sidebar_export_treeview_xls/<@https://customer.com/web/content/32110-3cbb18d/web.assets_backend.js:3395:264
each@https://customer.com/web/content/32058-c463c92/web.assets_common.js:625:758
on_sidebar_export_treeview_xls@https://customer.com/web/content/32110-3cbb18d/web.assets_backend.js:3395:75
dispatch@https://customer.com/web/content/32058-c463c92/web.assets_common.js:892:378
add/elemData.handle@https://customer.com/web/content/32058-c463c92/web.assets_common.js:865:151`

EDIT:
The fix by @fr33co fixes this issue.

@CasVissers-360ERP
Copy link

CasVissers-360ERP commented Apr 25, 2018

While exporting the list view generated by the OCA tax balance module:

Fout:
TypeError: view.$el.find(...)[0] is undefined

https://customer.com/web/content/33266-43b409b/web.assets_backend.js:3395
Traceback:
on_sidebar_export_treeview_xls/<@https://customer.com/web/content/33266-43b409b/web.assets_backend.js:3395:264
each@https://customer.com/web/content/32058-c463c92/web.assets_common.js:625:758
on_sidebar_export_treeview_xls@https://customer.com/web/content/33266-43b409b/web.assets_backend.js:3395:75
dispatch@https://customer.com/web/content/32058-c463c92/web.assets_common.js:892:378
add/elemData.handle@https://customer.com/web/content/32058-c463c92/web.assets_common.js:865:151

@yajo
Copy link
Member

yajo commented Apr 25, 2018

Please send useful tracebacks: https://github.com/odoo/odoo/wiki/Contributing#tracebacks

if (this.tag == 'field' && (this.attrs.widget === undefined || this.attrs.widget != 'handle')) {
// non-fields like `_group` or buttons
export_columns_keys.push(column_index);
export_columns_names.push(view.$el.find('.o_list_view > thead > tr> th[title]:eq('+column_index+')')[0].textContent);
Copy link
Contributor

@benwillig benwillig May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out this selector only works in debug mode.
I fixed it here acsone@bf97999
I'm not able to push on your repo @JBF91 but you can cherry pick my commit.
This commit also fixes errors when a column is not sortable, this is related to last @CasVissers reported error.

@JBF91
Copy link
Contributor Author

JBF91 commented May 18, 2018

Few tests passed, everything OK with @benwillig commit cherry picked.

@benwillig
Copy link
Contributor

I pushed an other commit here acsone@88f0e42. When exporting, numbers formatted with a float time widget are changed from '01:00' to '0100', this commit fixes this behavior.

headers: export_columns_names,
rows: export_rows
})},
complete: $.unblockUI
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add error handler ?
error: c.rpc_error.bind(c),
'c' will be crash manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any fast way of testing the error binding?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May b you can raise a warning or traceback from /web/export/xls_view route and it will be handled via crashmanager => error: c.rpc_error.bind(c)

Copy link

@antoniocanovas antoniocanovas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review Ok.

@JBF91
Copy link
Contributor Author

JBF91 commented Jul 9, 2018

As @MeetKD recommended, added CrashManager. I think this is ready to merge.

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you accidentally introduced a lint error.

@@ -7,6 +7,7 @@
import json
import odoo.http as http
from odoo.http import request
from odoo import exceptions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 not needed

@angelmoya
Copy link
Member

Tested local and works ok.

I don't if someone else have this little issue, but I need to restart Odoo because list view get a javascript error

Copy link

@CasVissers-360ERP CasVissers-360ERP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Leonardo Pistone and others added 20 commits August 15, 2018 16:29
* Compatibility with Werkzeug 0.9.3
* Export of first one2many field works again
* Parsing of float values
… with the original index used in the native excel export
Monetary fields were being exported empty because the parsing failed.

In the way of correctly exporting them as numbers, this chunk of code's performance has been improved.
* Retrieve all columns even if they are not sortable. Prevent to get select record column
* Prevent to process float time numbers as real numbers during the export
@pedrobaeza
Copy link
Member

As there's no better option for now than exporting what is shown in the view, let's continue with the merge.

@pedrobaeza pedrobaeza merged commit 9139f87 into OCA:11.0 Aug 15, 2018
@richard-willdooit
Copy link

We have been using it for a short while now in a production environment with no noticeable issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet