-
-
Notifications
You must be signed in to change notification settings - Fork 322
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
[18.0][MIG] base_report_to_printer: Migration to 18.0 #371
base: 18.0
Are you sure you want to change the base?
Conversation
Have you a clue why CUPS is not running in the tests? (I compared the |
|
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.
non blocking comment
pre-approving: just the tests to fix
("client", "Send to Client"), | ||
("user_default", "Use user's defaults"), | ||
] | ||
_available_action_types = AVAILABLE_ACTION_TYPES |
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 think I liked the previous code better (here, and in all other similar cases), as it allowed for easier extension through overrides.
I wonder, what motivated this change?
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.
Hi @ivantodorovich, thanks for your in-depth review
As Odoo native decided to bypass the validation when the selection is a callable in here, it breaks some logic of the module, e.g:
report-print-send/base_report_to_printer/tests/test_res_users.py
Lines 15 to 19 in 58cba79
def test_available_action_types_excludes_user_default(self): | |
"""It should not contain `user_default` in avail actions""" | |
self.user_vals["printing_action"] = "user_default" | |
with self.assertRaises(ValueError): | |
self.new_record() |
But my approach supports extension as well, no?
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.
Could it work with a property? So we keep a method to ease the override
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.
Hi @sebalix, using 'property' is a good approach 👍 , thank you
I updated the code
a5aae19
to
3b27729
Compare
|
ad38882
to
aec32f6
Compare
Migrate ir_report.py to new API Migrate printing.py to new API Migrate res_users.py to new API Migrate report_xml_action.py to new API Migrate wizard/update_printers.py to new API Better view for wizard Recursion when calling a method with old-style api signature from browse Remove the Lock because it is useless on multiprocess Replace it by a database lock so the different processes are all aware of the lock and the last update timestamp. browse is called often enough to call the update routine (even too much) Implements the print on the new 'report' model Restore the print capability on deprecated reports Update copyrights Improve form view, add search view for printers Update translations, add a string to URI so it is uppercased missing api decorator We need the report in print_document and print options (needed in printer_tray) Move the 'skip_update' right in the browse, it prevents a loop See odoo/odoo#3644 Also, it helps to have the value set/read in context close to each other. Avoid to hits the database too many times to check if the list of printers needs to be refreshed. Keep the last update datetime in cache and invalidate this datetime if is is older than POLL_INTERVAL. Thus, one process won't hit the DB more than 1 time every POLL_INTERVAL (10 seconds currently) to check if it needs to update the list. Refresh the list of printers every 15 seconds instead of 10 Extract a method so it will be easier to override in printer_tray Error on installation of the module Invalidate the cache when the table is created so the table_exists() method returns a fresh value after creation of the table Use a cron instead of threads to update printers status The implementation with threads was blocking the loading of the server in multiprocess. Using a cron will lower the frequency of the updates but at least it is simple and reliable. Fixes OCA#14 Do not write the printer status if it has not changed Avoid unnecessary UPDATE every minute Clean the XML file (remove eval, reindent) Give access to models to all users for reading So they are able to print
[Usability] Auto-add Administrator user to the Print group Make XML code more readable base_report_to_printer: add support for remote CUPS server (not just localhost) More logging and better error handling Add CUPS_HOST in more debug logs
Instead, a notification is displayed to the user. When report.get_pdf() is called on a report that must be printer, it will print the report *and* returns the pdf, thus code that calls directly report.get_pdf() will print the pdf on the printer as expected. Fixes OCA#16
In order to get visibility on https://www.odoo.com/apps the OCA board has decided to add the OCA as author of all the addons maintained as part of the association.
By calling `super.get_pdf` in print_document we can encounter trouble with MRO resolution that prevent custom report parser (e.g. override of `get_pdf`) to be called. The fix consist of not calling `super` and prevent multiple call to 'printer.print_document'
* context was lost while getting report * now it will be passed using with_context * could be used for print_options (example: pass copies amount for productlabals)
cups is an external dependency, if it is not installed Odoo will not start. OCA guidelines specify guidelines for External dependencies, code is from there.
This PR has the |
I forwarded several commits up to 17.0, could you update your migration base? |
If the CUPS server isn't available the user won't be able to do anything to print the report they need. At last we can give them the chance to have a fallback behavior downloading the document. TT47134
- Add access rules to the wizard - Set a fallback name for the printers and respect the user custom ones TT45159
58cba79
to
5ab9f3e
Compare
Better handling of exceptions feedback. A notification will show up with the issued printer and report and a button for the user to download the report as a fallback to the failure. TT51628
5ab9f3e
to
02e1d3b
Compare
02e1d3b
to
4fd9a30
Compare
92d9e70
to
4bfd4d8
Compare
4bfd4d8
to
128c02e
Compare
Hi @chienandalu, it's done |
// Just so the translation engine detects them as it doesn't do it inside | ||
// template strings | ||
const terms = { | ||
the_report: env._t("The 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.
the_report: env._t("The report"), | |
the_report: _t("The report"), |
// template strings | ||
const terms = { | ||
the_report: env._t("The report"), | ||
couldnt_be_printed: env._t( |
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.
couldnt_be_printed: env._t( | |
couldnt_be_printed: _t( |
couldnt_be_printed: env._t( | ||
"couldn't be printed. Click on the button below to download it" | ||
), | ||
issue_on: env._t("Issue on"), |
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.
issue_on: env._t("Issue on"), | |
issue_on: _t("Issue on"), |
messageIsHtml: true, | ||
buttons: [ | ||
{ | ||
name: env._t("Print"), |
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.
name: env._t("Print"), | |
name: _t("Print"), |
Note
Changes in 18.0
ir.property
was removed in odoo/odoo@de302c2. Hence, replace default value forproperty_printing_action_id
with invoked function.doall
&numbercall
fields were removed from ir_cron in odoo/odoo@3a9949aAVAILABLE_ACTION_TYPES
with the precomputed value instead of the function which returns the precomputed value.This changes