[11.0][MIG] base_report_to_printer + printer tray: Merged together #110

gdgellatly · 2017-10-04T16:31:21Z

Relies on #109

Straight port with addition of _order attribute on printer tray
Tested locally

…t: because if an exception occurs, a postgresql transaction will be leaked. The except clause should properly rollback the cr

…thing else than None which is forbidden

replaced by isinstance(printer, basestring)

… activated when their migration is done.

…nd orm.TransientModel instead of osv aliases

…nstead of a report id makes more sens

…f printer browse record

… in behavior for printer

gdgellatly · 2017-12-06T09:23:41Z

Just hold on reviewing this. I think the new way they have implemented the Report Controller means that this javascript code intercepts the wrong route so we end up with both an old style report action and a new style controller resulting in the report printing both on printer and on screen

Garamotte · 2017-12-06T09:24:52Z

base_report_to_printer/data/printing_data.xml

-        <field eval="'()'" name="args"/>
+        <field name="state">code</field>
+        <field name="code">
+model.action_update_jobs()


Thanks, I discovered the change of crons recently, and didn't adapt the base_report_to_printer PR.
You can put this on a single line, like I did here: OCA/stock-logistics-barcode@b4cc193#diff-2cad5a619c492c769aa7bf1f7cb7e9e7R15

Thanks done

gdgellatly · 2017-12-06T22:12:34Z

@pedrobaeza @lasley @sylvain-garancher I need one last bit of help to get this done (or we call it a feature and put in known issues).

In qwebactionmanager.js of web there is a function called trigger_download which calls the report_download route in web.ReportController. It is this function that ultimately prints the report and generates the client response. So what is happening as best I can tell is that our javascript is called and correctly doesn't call super. But there is something else in the web client that does regardless.

I'm kind of out of ideas on how to approach and fix this. I've tried a few things within the existing paradigm, such as overiding the report_download function to return nothing (endless loop), changing the action_val report type (no effect, not passed).

Also note there is a further complication, that is that nearly every module in reporting-engine repo overrides this same function and does call super.

I kind of get the feeling that the approach needs a complete rethink or else I'm just missing something obvious.

pedrobaeza · 2017-12-12T19:03:42Z

JS is not my best skill. @yajo can you help with this?

lasley · 2017-12-12T19:06:29Z

@gdgellatly - I'm having trouble finding the method you're referring to in this code. Can you please comment inline on the method in question?

gdgellatly · 2017-12-12T19:17:38Z

@lasley sure

https://github.com/odoo/odoo/blob/11.0/addons/web/static/src/js/report/qwebactionmanager.js#L110
for the trigger download code

https://github.com/odoo/odoo/blob/11.0/addons/web/controllers/main.py#L1637 for the ultimate route.

EDIT: These are what are called, no matter what the current js does.

yajo

I hope it helps 😊

yajo · 2017-12-13T08:57:01Z

base_report_to_printer/README.rst

+
+   sudo apt-get install cups
+   sudo apt-get install libcups2-dev
+   sudo apt-get install python-dev OR sudo apt-get install python3-dev


This should be only python3-dev. Python 2 is not supported in odoo v11.

yajo · 2017-12-13T08:57:17Z

base_report_to_printer/README.rst

+   sudo apt-get install cups
+   sudo apt-get install libcups2-dev
+   sudo apt-get install python-dev OR sudo apt-get install python3-dev
+   sudo easy_install pycups OR sudo pip install pycups


easy_install, really? Drop that please 😆

yajo · 2017-12-13T08:57:50Z

base_report_to_printer/README.rst

+
+
+Known issues / Roadmap
+======================


Delete this empty section.

yajo · 2017-12-13T09:00:11Z

base_report_to_printer/migrations/9.0.2.0.0/post-10-create_server_record.py

+__name__ = 'Create a printing.server record from previous configuration'
+
+
+def migrate(cr, v):


Delete this file, it's a migration for v9

yajo · 2017-12-13T09:03:00Z

base_report_to_printer/static/src/js/qweb_action_manager.js

+                            context: action_val.context || {}
+                        }).then(function () {
+                            self.do_notify(_t('Report'),
+                                _t('Document sent to the printer ') + print_action.printer_name);


Don't concatenate strings, inerpolate with %s and _.str.sprintf(), for better translations.

yajo · 2017-12-13T09:03:13Z

base_report_to_printer/static/src/js/qweb_action_manager.js

+                                _t('Document sent to the printer ') + print_action.printer_name);
+                        }).fail(function () {
+                            self.do_notify(_t('Report'),
+                                _t('Error when sending the document to the printer ') + print_action.printer_name);


Use %s also.

yajo · 2017-12-13T09:12:36Z

base_report_to_printer/static/src/js/qweb_action_manager.js

+            var self = this;
+            var _super = this._super;
+
+            if ('report_type' in action_val && action_val.report_type === 'qweb-pdf') {


If I'm not wrong, you should be adding a new action.report_type called i.e. qweb-pdf-server, or make the action have notion of being printed in the server somehow.

Upstream code uses that key to know if you need an HTML report or a PDF report, so you should just do in this method:

Check if action.report_type === "qweb-pdf-server".

If it's true, do my things.

If it's false, return this._super.apply(this, arguments) and forget.

thanks @yajo it will take me a while to get my head around. I don't think a new report type is correct, because every qweb-pdf report is subject to a specific or generic printing behaviour, even if that behaviour is just to send to client.

The current js works, and prints as expected, its just that it also downloads to the client, so super is being called regardless of this function. Is there some way we can, say around line 33, once we know it is to be printed, change action_val.report_type so it falls through?

Total shot in the dark, but I wonder if Action.ir_actions_report is the culprit. This is the first I've looked at this js/chrome folder though - I have no idea wtf it's for

@lasley @yajo OK I made massive progress. I found a bug in the js code where action_val was checked instead of action. As a result, none of this javascript was ever called anyway, yet it still printed the report.

I just commented out the entire file and still got my printed report (plus the client download).

I then just manually edited the report/download http in web/controllers/main to return this (warning ugly ahead)

@http.route(['/report/download'], type='http', auth="user") def report_download(self, data, token): """This function is used by 'qwebactionmanager.js' in order to trigger the download of a pdf/controller report. :param data: a javascript array JSON.stringified containg report internal url ([0]) and type [1] :returns: Response with a filetoken cookie and an attachment header """ requestcontent = json.loads(data) url, type = requestcontent[0], requestcontent[1] try: if type == 'qweb-pdf': reportname = url.split('/report/pdf/')[1].split('?')[0] # <-----------------snip--------------> error = { 'code': 200, 'message': "Report Sent to Printer", 'data': {} } return request.make_response(html_escape(json.dumps(error)))

and I got this (EDIT: And the printed report)

so what I think the best strategy is

Remove the javascript

Override report_download with this algorithm

Call super

If it is to be sent to printer -> Return notification report has printed else return super response.

So (and sorry for using you guys as support here) how do I return a notification rather than an error in a python web controller?

@gdgellatly - A notification as in one of the javascript boxes that pops up in the top right corner from time to time? Or some sort of error that still allows processing?

gdgellatly · 2017-12-13T22:16:08Z

base_report_to_printer/static/src/js/qweb_action_manager.js

+                            kwargs: {data: action_val.data || {}},
+                            context: action_val.context || {}
+                        }).then(function () {
+                            self.do_notify(_t('Report'),


@lasley Essentially I just want to replicate this block in python - so the top right kind of notification

Damn I thought that was the case - I don't know of a way to do this. The crash manager is what handles anything we can send back error-wise from Python, and it does dialogs.

An interesting module idea would be a NotificationError in Python and something in JS that intercepts the internal RPC calls, catches errors, and handles appropriately if a notification error. I don't think that exists though.

gdgellatly · 2017-12-14T00:05:14Z

@lasley @yajo et al, I got it, don't waste any more time helping me.

gdgellatly · 2017-12-14T00:41:24Z

@lasley @pedrobaeza @sylvain-garancher @yajo Ok Ready for review. My apologies, I went down a rabbithole with this javascript, which I don't really get, but finally realized the error was in the change to using rpc.

I left it as a seperate commit so you can see how dumb I am.

ougc27

Hello

I just tried locally this module successfully 👍

yajo · 2017-12-14T09:00:07Z

base_report_to_printer/tests/test_ir_actions_report.py

@@ -73,6 +73,7 @@ def test_print_action_for_report_name_returns_if_report(self):
        """ It should return correct serializable result for behaviour """
        with mock.patch.object(self.Model, '_get_report_from_name') as mk:
            res = self.Model.print_action_for_report_name('test')
+            del res['id']  # HELP!


What if you add below to expect dict: 'id': behaviour.id instead?

I tried it, but I don't think it will work, behaviour is just a method, not an instance of a report.

actually, now that I think about it, probably the way I did the test is best for now. print_action_for_report_name adds id and is the only function that adds id becuase it is only needed for the rpc query. By deleting that key, we are confirming that it is adding that key, and only that key.

@yajo I fixed it so its much better

pedrobaeza · 2017-12-18T11:01:22Z

@yvaucher please give your final blessing

yvaucher · 2017-12-18T12:48:10Z

Thanks @gdgellatly great work let's merge it

eLBati and others added 30 commits October 3, 2017 18:05

[ADD] base_report_to_printer

feae5db

[IMP] base_report_to_printer - module description

e9598f5

[imp] description with images

c9adf1b

[FIX] The close should be done in the finally: clause of a try: excep…

4498bce

…t: because if an exception occurs, a postgresql transaction will be leaked. The except clause should properly rollback the cr

[FIX] We better have to use the "with" syntax

ced1d0c

[FIX] update method is accessible from XML/RPC and should return some…

6930c5c

…thing else than None which is forbidden

[FIX] print_direct return value

b452b33

[fix] get_default return value

64f8a7f

[FIX] context=None

5db700c

[FIX] using logging

576fbe4

[FIX] isinstance(printer, (str,unicode)):

76f9704

replaced by isinstance(printer, basestring)

[FIX] base_report_to_printer description

d57fc8f

[FIX] base_report_to_printer - 'authors'

04eb2b7

[MIGR] Set the 'installable' flags to False on addons. They should be…

08db5a9

… activated when their migration is done.

base_report_to_printer V7 upgrade

49d4e2e

[FIX] base_report_to_printer - load security first

e9e16db

[IMP] base_report_to_printer - retab and clean extra spaces

bbe2142

[IMP] base_report_to_printer - remove class calls and use orm.Model a…

10b5678

…nd orm.TransientModel instead of osv aliases

[IMP] base_report_to_printer - clean imports

c336491

[IMP] base_report_to_printer - remove class call

121865f

[IMP] base_report_to_printer - simplify calls on class

0dc248d

[TYPO]

ab4afa3

[IMP] base_report_to_printer - replace lpr by use of pycups

7d39b4d

[IMP] base_report_to_printer - add report_id on print_direct

c7f1e0b

[IMP] base_report_to_printer - use an action id in an action method i…

60d01e0

…nstead of a report id makes more sens

[FIX] base_report_to_printer - using system_name of printer instead o…

ec028b3

…f printer browse record

[IMP] base_report_to_printer - remove uselsee context set

3878a86

[IMP] base_report_to_printer - return browse record instead of string…

63874cb

… in behavior for printer

[IMP] base_report_to_printer - retab xml files

42efe15

[IMP] base_report_to_printer - show field name in report and user views

083bc82

Garamotte reviewed Dec 6, 2017

View reviewed changes

gdgellatly force-pushed the 11.0-printer-tray branch from d2ab7b9 to 628e152 Compare December 6, 2017 21:46

yajo requested changes Dec 13, 2017

View reviewed changes

[FIX] Runbot error, try new cron code, plus @yajo review cleanup

506debb

gdgellatly force-pushed the 11.0-printer-tray branch from 628e152 to 506debb Compare December 13, 2017 11:07

gdgellatly commented Dec 13, 2017

View reviewed changes

gdgellatly force-pushed the 11.0-printer-tray branch from 9ba72a3 to 668aa61 Compare December 14, 2017 00:44

ougc27 approved these changes Dec 14, 2017

View reviewed changes

gdgellatly force-pushed the 11.0-printer-tray branch 2 times, most recently from 412fa16 to 708c52f Compare December 14, 2017 01:42

yajo requested changes Dec 14, 2017

View reviewed changes

gdgellatly force-pushed the 11.0-printer-tray branch from 708c52f to 740ab72 Compare December 14, 2017 09:07

[FIX] Fix the javascript calls

9e02139

gdgellatly force-pushed the 11.0-printer-tray branch from 740ab72 to 9e02139 Compare December 14, 2017 09:18

pedrobaeza changed the title ~~11.0 printer tray~~ [11.0][MIG] base_report_to_printer + printer tray: Merged together Dec 14, 2017

pedrobaeza mentioned this pull request Dec 14, 2017

Porting to Odoo 11 #118

Closed

lasley added the needs review label Dec 14, 2017

yajo approved these changes Dec 18, 2017

View reviewed changes

yvaucher approved these changes Dec 18, 2017

View reviewed changes

yvaucher merged commit 36a0288 into OCA:11.0 Dec 18, 2017

pedrobaeza mentioned this pull request Jan 1, 2018

[MIG] printer_tray: Migration to 11.0 #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[11.0][MIG] base_report_to_printer + printer tray: Merged together #110

[11.0][MIG] base_report_to_printer + printer tray: Merged together #110

gdgellatly commented Oct 4, 2017

gdgellatly commented Dec 6, 2017

Garamotte Dec 6, 2017

gdgellatly Dec 6, 2017

gdgellatly commented Dec 6, 2017

pedrobaeza commented Dec 12, 2017

lasley commented Dec 12, 2017

gdgellatly commented Dec 12, 2017 •

edited

yajo left a comment

yajo Dec 13, 2017

yajo Dec 13, 2017

yajo Dec 13, 2017

yajo Dec 13, 2017

yajo Dec 13, 2017

yajo Dec 13, 2017

yajo Dec 13, 2017

gdgellatly Dec 13, 2017

lasley Dec 13, 2017 •

edited

gdgellatly Dec 13, 2017 •

edited

lasley Dec 13, 2017

gdgellatly Dec 13, 2017

lasley Dec 13, 2017

lasley Dec 13, 2017

gdgellatly commented Dec 14, 2017

gdgellatly commented Dec 14, 2017

ougc27 left a comment

yajo Dec 14, 2017

gdgellatly Dec 14, 2017

gdgellatly Dec 14, 2017

gdgellatly Dec 14, 2017

pedrobaeza commented Dec 18, 2017

yvaucher commented Dec 18, 2017

		__name__ = 'Create a printing.server record from previous configuration'


		def migrate(cr, v):



		Known issues / Roadmap
		======================

[11.0][MIG] base_report_to_printer + printer tray: Merged together #110

[11.0][MIG] base_report_to_printer + printer tray: Merged together #110

Conversation

gdgellatly commented Oct 4, 2017

gdgellatly commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdgellatly commented Dec 6, 2017

pedrobaeza commented Dec 12, 2017

lasley commented Dec 12, 2017

gdgellatly commented Dec 12, 2017 • edited

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Dec 13, 2017 • edited

Choose a reason for hiding this comment

gdgellatly Dec 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdgellatly commented Dec 14, 2017

gdgellatly commented Dec 14, 2017

ougc27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Dec 18, 2017

yvaucher commented Dec 18, 2017

gdgellatly commented Dec 12, 2017 •

edited

lasley Dec 13, 2017 •

edited

gdgellatly Dec 13, 2017 •

edited