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

Merged
merged 128 commits into from
Jan 20, 2025

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.
@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 8 times, most recently 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

@trisdoan trisdoan force-pushed the 18.0-mig-base_report_to_printer branch from 128c02e to d0276e8 Compare December 5, 2024 10:55
@trisdoan trisdoan requested a review from chienandalu December 5, 2024 10:55
@sebalix
Copy link
Contributor

sebalix commented Dec 20, 2024

Hello @chienandalu , are you OK with the latest changes?

@sebalix
Copy link
Contributor

sebalix commented Jan 20, 2025

/ocabot migration base_report_to_printer

@OCA-git-bot OCA-git-bot added this to the 18.0 milestone Jan 20, 2025
@sebalix
Copy link
Contributor

sebalix commented Jan 20, 2025

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 18.0-ocabot-merge-pr-371-by-sebalix-bump-nobump, awaiting test results.

@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 936c934. Thanks a lot for contributing to OCA. ❤️

@OCA-git-bot OCA-git-bot merged commit c36d716 into OCA:18.0 Jan 20, 2025
6 of 7 checks passed
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.