-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
[11.0][MIG] base_report_to_printer + printer tray: Merged together #110
Conversation
…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
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 |
<field eval="'()'" name="args"/> | ||
<field name="state">code</field> | ||
<field name="code"> | ||
model.action_update_jobs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks done
d2ab7b9
to
628e152
Compare
@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. |
JS is not my best skill. @yajo can you help with this? |
@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? |
@lasley sure https://github.com/odoo/odoo/blob/11.0/addons/web/static/src/js/report/qwebactionmanager.js#L110 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope it helps 😊
base_report_to_printer/README.rst
Outdated
|
||
sudo apt-get install cups | ||
sudo apt-get install libcups2-dev | ||
sudo apt-get install python-dev OR sudo apt-get install python3-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be only python3-dev
. Python 2 is not supported in odoo v11.
base_report_to_printer/README.rst
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easy_install, really? Drop that please 😆
base_report_to_printer/README.rst
Outdated
|
||
|
||
Known issues / Roadmap | ||
====================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this empty section.
__name__ = 'Create a printing.server record from previous configuration' | ||
|
||
|
||
def migrate(cr, v): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this file, it's a migration for v9
context: action_val.context || {} | ||
}).then(function () { | ||
self.do_notify(_t('Report'), | ||
_t('Document sent to the printer ') + print_action.printer_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't concatenate strings, inerpolate with %s
and _.str.sprintf()
, for better translations.
_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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use %s
also.
var self = this; | ||
var _super = this._super; | ||
|
||
if ('report_type' in action_val && action_val.report_type === 'qweb-pdf') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
628e152
to
506debb
Compare
kwargs: {data: action_val.data || {}}, | ||
context: action_val.context || {} | ||
}).then(function () { | ||
self.do_notify(_t('Report'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lasley Essentially I just want to replicate this block in python - so the top right kind of notification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
9ba72a3
to
668aa61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello
I just tried locally this module successfully 👍
412fa16
to
708c52f
Compare
@@ -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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you add below to expect
dict: 'id': behaviour.id
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it, but I don't think it will work, behaviour is just a method, not an instance of a report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yajo I fixed it so its much better
708c52f
to
740ab72
Compare
740ab72
to
9e02139
Compare
@yvaucher please give your final blessing |
Thanks @gdgellatly great work let's merge it |
Relies on #109
Straight port with addition of _order attribute on printer tray
Tested locally