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

[18.0][MIG] base_report_to_printer: Migration to 18.0 #371

Open
wants to merge 128 commits into
base: 18.0
Choose a base branch
from

Conversation

trisdoan
Copy link

@trisdoan trisdoan commented Oct 14, 2024

Note

Changes in 18.0

  • ir.property was removed in odoo/odoo@de302c2. Hence, replace default value for property_printing_action_id with invoked function.
  • doall & numbercall fields were removed from ir_cron in odoo/odoo@3a9949a
  • callable is not validated in selection any longer due to performance cost in odoo/odoo@eaef3d2. So, initialise AVAILABLE_ACTION_TYPES with the precomputed value instead of the function which returns the precomputed value.

This changes

  • Take this chance to replace percent format with f-string

@sebalix
Copy link
Contributor

sebalix commented Oct 23, 2024

Have you a clue why CUPS is not running in the tests? (I compared the test.yml GH workflow file with the 17.0, and didn't saw real difference...)

@nilshamerlinck
Copy link

  • these tests don't need a real cups server to run, as calls are mocked
  • what is new is that 18.0 repos are now configured with OCA_ENABLE_CHECKLOG_ODOO=1, so the warnings are blocking
  • but these WARNING odoo odoo.addons.base_report_to_printer.models.printing_server: Failed to connect to the CUPS server on localhost:631. Check that the CUPS server is running and that you can reach it from the Odoo server. are actually expected, as concerned tests are testing some failure cases
  • so @trisdoan could you please use the .assertLogs context manager to trap these logs? see here for an example

Copy link

@ivantodorovich ivantodorovich left a 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

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?

Copy link
Author

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:

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?

Copy link
Contributor

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

Copy link
Author

@trisdoan trisdoan Nov 19, 2024

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

@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch from a5aae19 to 3b27729 Compare October 28, 2024 08:58
@trisdoan trisdoan marked this pull request as draft October 28, 2024 08:59
@trisdoan
Copy link
Author

trisdoan commented Oct 28, 2024

DRAFT: update test

@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch 5 times, most recently from ad38882 to aec32f6 Compare October 28, 2024 09:31
guewen and others added 16 commits October 28, 2024 16:34
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.
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@chienandalu
Copy link
Member

I forwarded several commits up to 17.0, could you update your migration base?

alexis-via and others added 3 commits November 19, 2024 09:20
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
@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch from 58cba79 to 5ab9f3e Compare November 19, 2024 02:54
sergio-teruel and others added 3 commits November 19, 2024 09:57
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
@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch from 5ab9f3e to 02e1d3b Compare November 19, 2024 02:57
@trisdoan trisdoan marked this pull request as draft November 19, 2024 02:58
@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch from 02e1d3b to 4fd9a30 Compare November 19, 2024 03:41
@trisdoan trisdoan marked this pull request as ready for review November 19, 2024 03:41
@trisdoan trisdoan marked this pull request as draft November 19, 2024 03:52
@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch 7 times, most recently from 92d9e70 to 4bfd4d8 Compare November 19, 2024 04:54
@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch from 4bfd4d8 to 128c02e Compare November 19, 2024 04:57
@trisdoan trisdoan marked this pull request as ready for review November 19, 2024 04:59
@trisdoan
Copy link
Author

I forwarded several commits up to 17.0, could you update your migration base?

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"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
issue_on: env._t("Issue on"),
issue_on: _t("Issue on"),

messageIsHtml: true,
buttons: [
{
name: env._t("Print"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: env._t("Print"),
name: _t("Print"),

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

Successfully merging this pull request may close these issues.