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.

Sign up for GitHub

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

Jump to bottom

[12.0][ADD] report_async #355

Merged
merged 1 commit into from May 15, 2020
Merged

[12.0][ADD] report_async #355

merged 1 commit into from May 15, 2020

Conversation

kittiu
Copy link
Member

@kittiu kittiu commented Dec 10, 2019

The new menu "Report Center" is the central place to host all your reports in one place.
From here, there are 2 ways to launch the report,

  1. Run Now - run report immediately as per normal.
  2. Run Background - put the report execution to queue job.

report_async

@kittiu kittiu mentioned this pull request Dec 10, 2019
Closed
1 task
@kittiu
Copy link
Member Author

kittiu commented Dec 11, 2019

@Saran440 can you review this?

@kittiu kittiu mentioned this pull request Dec 27, 2019
Open
@kittiu kittiu mentioned this pull request Jan 9, 2020
Closed
6 tasks
@kittiu kittiu force-pushed the 12.0-add-report_async branch 2 times, most recently from a27af7f to 52d014c Compare January 13, 2020 11:38
@kittiu
Copy link
Member Author

kittiu commented Jan 15, 2020

This module is ready for review / merge

Note: --test-enable can result in fallback to type html (it is in Odoo core). So, when you test by hand, make sure you don't use --test-enable on your environment.

@sbidoul
Copy link
Member

sbidoul commented Feb 7, 2020

Very interesting.

Regarding the mechanism to make the file available to the user, I have a preference for the one in base_export_async which sends an email with a link to the report that only the user can download. It also has a mechanism to remove these files after a while.

@kittiu
Copy link
Member Author

kittiu commented Feb 7, 2020 •
edited

@sbidoul we have both, do you think it is ok? In fact, base_export_async was my example for this.
https://github.com/ecosoft-odoo/reporting-engine/blob/12.0-add-report_async/report_async/models/report_async.py#L158

rousseldenis
rousseldenis suggested changes Feb 18, 2020
View reviewed changes
Copy link
Sponsor Contributor

@rousseldenis rousseldenis left a comment •
edited

Choose a reason for hiding this comment

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

Code review. Apart of that, this is great !

report_async/__manifest__.py Outdated
'version': '12.0.1.0.0',
'author': 'Ecosoft, Odoo Community Association (OCA)',
'license': 'AGPL-3',
'website': 'https://github.com/OCA/queue',
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggested change
'website': 'https://github.com/OCA/queue',
'website': 'https://github.com/OCA/reporting-engine',

report_async/models/report_async.py

@api.multi
def _compute_job(self):
for rec in self:
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggested change
for rec in self:
for rec in self.sudo():

At each call of sudo(), a new environment is created and can lead to performance issues.

Copy link
Member Author

@kittiu kittiu Feb 21, 2020 •
edited

Choose a reason for hiding this comment

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

Or should I remove all the .sudo() from _compute_job()
and use compute_sudo=True in all fields that call _compute_job instead?
WDYT?

report_async/models/report_async.py Outdated
@api.multi
def _compute_file(self):
for rec in self:
rec.file_ids = self.env['ir.attachment'].search(
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Move the search before the loop with ('res_id', 'in', self.ids) and then filter.

It can avoid many sql queries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea. Fixed.

report_async/models/report_async.py Outdated
'recipient_ids': [(6, 0, [user.partner_id.id])],
'subject': _("Export {} {}").format(
description, fields.Date.to_string(fields.Date.today())),
'body_html': _("""
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Mmmmh, maybe a mail.template ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done using a template.

image

Copy link

Choose a reason for hiding this comment

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

Add a widget in the navigation drawer to centrally notify the report print results.
How about this suggestion？

Copy link

Choose a reason for hiding this comment

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

This is great anyway

@rousseldenis
Copy link
Sponsor Contributor

@OCA/core-maintainers

@kittiu kittiu force-pushed the 12.0-add-report_async branch 2 times, most recently from 493060d to afdfc2b Compare February 21, 2020 10:10
jarroyomorales
jarroyomorales approved these changes Apr 30, 2020
View reviewed changes
Copy link

@jarroyomorales jarroyomorales left a comment

Choose a reason for hiding this comment

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

Tested locally and LGTM!
Just two questions. Is the dashboards app the best place for the report center? And should we create a security group to better control who accesses this?
Thank you for this awesome module!

etobella
etobella reviewed Apr 30, 2020
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Thanks for the effort!!! I think this is really a good implementation!!!
About the widget on the bar, that could be a great idea, but we can do this on a new PR, this way we can proceed. On the other hand, I agree about the need of a group.

@kittiu
Copy link
Member Author

kittiu commented Apr 30, 2020

About groups, the first idea is allow everyone to create his/her own report async. And have someone (group_no_one) to see them all.
I still not super sure what kind of security structure to best works in real lift, except anyone can help me decide. Otherwise I suggest we merge this first (given it is good enough). Then, people can use it and propose what best fit.

WDYT?

@etobella
Copy link
Member

Good for me.

@jarroyomorales
Copy link

@kittiu Okay, I agree with you, we can merge this and discuss it later.

@JordiBForgeFlow
Copy link
Sponsor Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-355-by-JordiBForgeFlow-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 48c8696 into OCA:12.0 May 15, 2020
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dong-z dong-z dong-z left review comments

@rousseldenis rousseldenis rousseldenis requested changes

@moylop260 moylop260 moylop260 approved these changes

@JordiBForgeFlow JordiBForgeFlow JordiBForgeFlow approved these changes

@etobella etobella etobella approved these changes

@jarroyomorales jarroyomorales jarroyomorales approved these changes

Assignees
No one assigned
Labels
merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@kittiu @sbidoul @rousseldenis @etobella @jarroyomorales @JordiBForgeFlow @OCA-git-bot @moylop260 @dong-z