[IMP] report_async 14.0 by KKamaa · Pull Request #611 · OCA/reporting-engine

KKamaa · 2022-04-13T15:41:59Z

@thomaspaulb @kittiu I hope we can now add the improvements? Tom should we use this MR?

OCA-git-bot · 2022-04-13T15:42:03Z

Hi @kittiu,
some modules you are maintaining are being modified, check this out!

kittiu · 2022-04-13T16:13:02Z

Can you describe what are the improvements? Also add them in the readme.
I saw javascript, I am not very familiar but I can help function test with the improvement.

HviorForgeFlow · 2022-04-14T06:23:55Z

IMO this functionality could be split to an specific module, something like report_async_mail_delivery

thomaspaulb · 2022-04-14T12:42:41Z

@kittiu The functionality is to generate reports async directly from the form view itself, and not have to go into the "Report Center" specifically to do it. So you press Print, you get a popup asking you if you want to run it Async, and then to verify the email address that it should be sent to.

@HviorForgeFlow I don't know if splitting it into a separate module _mail_delivery is really valid in this case, because the original module also does mail delivery. This is just another UI flow to achieve the same.

thomaspaulb · 2022-04-14T12:43:23Z

@KKamaa Maybe you can update the readme and/or post a small GIF

KKamaa · 2022-04-15T11:08:09Z

@kittiu @thomaspaulb I added the sample and additional info to readme.rst as show -> How It Works <-:
When it is merged it will point to the OCA - 14.0.

KKamaa · 2022-04-15T11:30:07Z

@thomaspaulb About the codecov part, it is already covered on the JS tests since we are testing the UI dialog and eventually we fill the to_email which is assigned to print_document_async -> run_report. All is covered by the normal tests created for the original report_async module.

kittiu · 2022-04-21T02:47:41Z

@KKamaa first of all, thanks for creating this. I was wanted this kind of feature before but lack of JS knowledge.

Following are my reviews,

First of all, I think this feature should be separated module, i.e., print_form_async or print_async, as the way of working has no relation with the report_async which is the aimed for long running reports, to be managed in report center. Your feature is aimed for form printing only and sending by email. So, separated module make sense to me.

Comment about this feature itself.

Normally, I think the use printing the document should be the one to receive the email.
So, in the async option, the email should be optional. May be change the label to "Default Email Recipient" and have some help saying that if not specified, the user who print this report will get email by default.
Now sure, we can send email to more than 1 user? This would be helpful if the wizard allow to add more emailes.
In the Wizard, if not default email in the setting, we can default the current user email.
In the Wizard, it make no sense if email is not filled, so it think it should raise error (currently, it just not us async)

thomaspaulb · 2022-04-26T15:03:59Z

@kittiu Why "no relation" ? I thought it is just the same feature but managed from the report center instead of on the record. I also reuse the same function for emailing the report from original module.

Maybe I don't understand what the report center is for? What is the reason to make it separated?

kittiu · 2022-04-26T15:54:00Z

@kittiu Why "no relation" ? I thought it is just the same feature but managed from the report center instead of on the record. I also reuse the same function for emailing the report from original module.

Maybe I don't understand what the report center is for? What is the reason to make it separated?

To me, the user experiences is very much different between

Go to report center -> Register new report action + config it as async -> run the report here and expect both email and stored reports.

VS

Go to existing report in settings -> config the report as async -> go to the working document -> run print and expect only email

The only thing being reused is email template, and the fact that it is async.

May be others who might have known this module can share their opinion too.
cc @rousseldenis @etobella @jarroyomorales

kittiu · 2022-04-26T15:56:09Z

IMO this functionality could be split to an specific module, something like report_async_mail_delivery

I just happen to read this comment. I think I see the same thing. Just the module naming to be decided.

thomaspaulb · 2022-04-26T16:37:44Z

If a new module, then the naming does not fit because the point of the new code is to make it easier for users to print a long running report directly from where they would normally call it. So i would call it "report_async_dialog". Then on the dialog we could also have a checkbox to say that the report should be downloadable as an attachment to the record.

I will study the module in more detail it just seem the same functionality with another type of interface.

NL66278

👍 LGTM

thomaspaulb · 2022-04-28T09:58:44Z

@kittiu If we add the option to save the attachment to the record as an extra feature in the dialog, would that be a good enough compromise to let you accept it to merge it to the main module?

Separating it would be some extra work that we would avoid if possible, but if you insist on it, we can do it

kittiu · 2022-04-29T03:06:39Z

@kittiu If we add the option to save the attachment to the record as an extra feature in the dialog, would that be a good enough compromise to let you accept it to merge it to the main module?

Separating it would be some extra work that we would avoid if possible, but if you insist on it, we can do it

@thomaspaulb ok then, thanks for your effort!

kittiu · 2022-04-29T03:12:07Z

+        default=100,
+        help="Min no of records to use async report functionality; e.g 100+",
+    )
+    async_mail_recipient = fields.Char(


Just idea, should this be the many2many TO: filed like in Send Email wizard?

the idea was not to have restrict to system users it can be used with emails where the user is not existing on the system. Also the char field is feed directly on the send_mail values as email_to. This allows comma separated emails, so you can just add a list of emails but separated with comma. e.g

oca@oca.com, tom@oca.com, jane@@usarmy.mil

elvise · 2022-07-29T17:00:24Z

@KKamaa great module!
What is your plan for this PR ?

P.S. Please rebase for fix the runboat issue

KKamaa · 2022-07-29T18:59:21Z

@KKamaa great module! What is your plan for this PR ?

P.S. Please rebase for fix the runboat issue

I was hoping to get it merged soon only issue is the codecov part but everything is ok

elvise · 2022-07-29T19:33:35Z

@KKamaa great module! What is your plan for this PR ?

P.S. Please rebase for fix the runboat issue

I was hoping to get it merged soon only issue is the codecov part but everything is ok

@KKamaa Can you rebase ?

elvise · 2022-08-15T13:15:34Z

@KKamaa great module! What is your plan for this PR ?

P.S. Please rebase for fix the runboat issue

I was hoping to get it merged soon only issue is the codecov part but everything is ok

@KKamaa Can you rebase ?

@KKamaa just a gentle reminder

thomaspaulb · 2022-08-16T09:08:22Z

Is there even a runboat issue? Runboat looks green.

elvise · 2022-08-16T11:35:46Z

Is there even a runboat issue? Runboat looks green.

@thomaspaulb yup, just try with rebase

thomaspaulb · 2022-08-16T11:47:12Z

@KKamaa please rebase, I cant do that in your repository

KKamaa · 2022-08-17T10:50:34Z

@thomaspaulb @elvise I did a rebase with OCA 14.0

francesco-ooops · 2022-08-24T08:11:13Z

@kittiu If we add the option to save the attachment to the record as an extra feature in the dialog, would that be a good enough compromise to let you accept it to merge it to the main module?

hi @thomaspaulb , is this feature present in the current version?

KKamaa · 2022-08-24T08:28:47Z

@kittiu If we add the option to save the attachment to the record as an extra feature in the dialog, would that be a good enough compromise to let you accept it to merge it to the main module?

hi @thomaspaulb , is this feature present in the current version?

@francesco-ooops I'm currently adding it, will update the functionality today

KKamaa · 2022-08-26T09:14:20Z

@kittiu @thomaspaulb @francesco-ooops did a rebase with OCA 14.0 and added the ability to save attachment on the record through checker. It will also do message_post of the attachment if message post is set via mail thread model.

thomaspaulb · 2022-08-29T12:22:01Z

+                    }
+                )
+                if hasattr(record, 'message_post'):
+                    record.message_post(attachment_ids=[attachment.id])


I'm not 100% sure but I think in some cases this might require a sudo, anyway it wouldn't hurt

@thomaspaulb added the sudo just in case

francesco-ooops · 2022-09-08T07:59:34Z

@KKamaa hi, I was trying to do a functional review on runboat, but I can't seem to find the most basic features on the module (please note I don't know this module at all)

From usage file:

Menu: Dashboard > Report Center

where do I access this menu on runboat? dev mode is activated

francesco-ooops · 2022-09-19T08:15:17Z

where do I access this menu on runboat? dev mode is activated

@KKamaa gentle reminder :)

KKamaa · 2022-09-21T03:43:04Z

where do I access this menu on runboat? dev mode is activated

@KKamaa gentle reminder :)

@francesco-ooops so you can checkout runbot test then try as below, see sample of test

francesco-ooops · 2022-10-03T09:31:42Z

+*Print*, you will get a popup asking you if you want to run it Async, and then to verify the email
+address that it should be sent to. See below sample:
+
+.. image:: https://raw.githubusercontent.com/OCA/reporting-engine/14.0/report_async/static/description/sample.gif


I think this should be just ../static/description/sample.gif

francesco-ooops

functional review ok!

I think it could be improved to add user email in report popup (if I'm generating the report, I'm probably the recipient) without having to type it everytime, but it's not blocking

@thomaspaulb @kittiu could you review? thanks!

francesco-ooops · 2022-10-03T13:10:47Z

I'm still trying to evaluate if behavior is correct, but it's not really clear to me what is the standard behavior of report_async (not being able to test job queue on runboat does not help)

please check this video :

create a report for a model (sale order in this case)
click "run in background"
select multiple records > print > report (is this correct?)
report is downloaded directly and there's no job in job queue ?

@kittiu @thomaspaulb can you help me understand the base flow of the module?

francesco-ooops · 2022-10-10T08:03:40Z

@kittiu @thomaspaulb @KKamaa gentle reminder :)

kittiu · 2022-10-11T07:54:15Z

@kittiu @thomaspaulb @KKamaa gentle reminder :)

Hello, I have seen the video. What you are choosing as the report is the Action to open Sales Order, which mean, when you click the button it will then open the Sales Order list. After that, it is just the normal Sales Order list.

You should select only Report Action.

So, the aim of this module is to print the long running report, not the form. But if you want to print the form in background, @thomaspaulb has add the option to do so, but not through Report Center. And I think you only do in the form view, if I am not mistaken.

Quoted here,

As additional improvement, you can now generate reports async directly from the form view itself, and not have to go into the "Report Center" specifically to do it. This can be done by pressing Print, you will get a popup asking you if you want to run it Async, and then to verify the email address that it should be sent to.

By the way, if you are ok, I think this IMP can be merged by the maintainer?

francesco-ooops · 2022-10-11T08:10:30Z

@kittiu yes good for me, maybe squashing commits @KKamaa ?

@OCA/reporting-engine-maintainers what do you say? good for merge?

francesco-ooops · 2022-10-12T10:47:40Z

@kittiu can you approve too? :)

francesco-ooops · 2022-10-14T08:46:49Z

@thomaspaulb good for you?

francesco-ooops · 2022-10-19T14:23:48Z

@OCA/reporting-engine-maintainers gentle reminder :)

thomaspaulb · 2022-10-21T12:26:40Z

/ocabot merge minor

OCA-git-bot · 2022-10-21T12:26:49Z

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-611-by-thomaspaulb-bump-minor, awaiting test results.

OCA-git-bot · 2022-10-21T12:36:15Z

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

KKamaa mentioned this pull request Apr 13, 2022

WIP: [14.0][IMP] report_async: direct dialog on Print sunflowerit/reporting-engine#3

Open

NL66278 approved these changes Apr 28, 2022

View reviewed changes

kittiu reviewed Apr 29, 2022

View reviewed changes

KKamaa added 2 commits August 17, 2022 13:43

[IMP] report_async 14.0

6c5cf35

[FIX] readme sample

65e4a41

KKamaa force-pushed the 14.0-imp-report-async branch from a44fa25 to 65e4a41 Compare August 17, 2022 10:44

KKamaa added 2 commits August 26, 2022 11:49

[FIX] support for attachment to record

cb5bbcf

[FIX] isort,prettier,black

997909e

KKamaa requested a review from kittiu August 26, 2022 09:17

thomaspaulb reviewed Aug 29, 2022

View reviewed changes

[FIX] sudo

1a344c5

KKamaa force-pushed the 14.0-imp-report-async branch from fdbc931 to 1a344c5 Compare August 30, 2022 06:49

KKamaa requested review from thomaspaulb and removed request for kittiu August 30, 2022 07:01

francesco-ooops reviewed Oct 3, 2022

View reviewed changes

francesco-ooops approved these changes Oct 3, 2022

View reviewed changes

OCA-git-bot added the approved label Oct 3, 2022

kittiu approved these changes Oct 12, 2022

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Oct 21, 2022

OCA-git-bot merged commit 2bfc591 into OCA:14.0 Oct 21, 2022

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Oct 21, 2022

Uh oh!

Conversation

KKamaa commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OCA-git-bot commented Apr 13, 2022

Uh oh!

kittiu commented Apr 13, 2022

Uh oh!

HviorForgeFlow commented Apr 14, 2022

Uh oh!

thomaspaulb commented Apr 14, 2022

Uh oh!

thomaspaulb commented Apr 14, 2022

Uh oh!

KKamaa commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KKamaa commented Apr 15, 2022

Uh oh!

kittiu commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaspaulb commented Apr 26, 2022

Uh oh!

kittiu commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kittiu commented Apr 26, 2022

Uh oh!

thomaspaulb commented Apr 26, 2022

Uh oh!

NL66278 left a comment

Choose a reason for hiding this comment

Uh oh!

thomaspaulb commented Apr 28, 2022

Uh oh!

kittiu commented Apr 29, 2022

Uh oh!

kittiu Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KKamaa Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

elvise commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KKamaa commented Jul 29, 2022

Uh oh!

elvise commented Jul 29, 2022

Uh oh!

elvise commented Aug 15, 2022

Uh oh!

thomaspaulb commented Aug 16, 2022

Uh oh!

elvise commented Aug 16, 2022

Uh oh!

thomaspaulb commented Aug 16, 2022

Uh oh!

KKamaa commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francesco-ooops commented Aug 24, 2022

Uh oh!

KKamaa commented Aug 24, 2022

Uh oh!

KKamaa commented Aug 26, 2022

Uh oh!

thomaspaulb Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

KKamaa Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

francesco-ooops commented Sep 8, 2022

Uh oh!

francesco-ooops commented Sep 19, 2022

Uh oh!

KKamaa commented Apr 13, 2022 •

edited

Loading

KKamaa commented Apr 15, 2022 •

edited

Loading

kittiu commented Apr 21, 2022 •

edited

Loading

kittiu commented Apr 26, 2022 •

edited

Loading

kittiu Apr 29, 2022 •

edited

Loading

elvise commented Jul 29, 2022 •

edited

Loading

KKamaa commented Aug 17, 2022 •

edited

Loading