[9.0][ADD] Migrated business_requirement_deliverable_report module from v8 to v9 #31

sudhir-serpentcs · 2016-09-30T12:03:20Z

~~WIP until #25 and #26 is merged~~

Migrated business_requirement_deliverable_report module from v8 to v9.
@dreispt @pedrobaeza @jbeficent @moylop260 Please review.

… to v9.

elicoidal · 2016-09-30T16:18:16Z

👍 (waiting for #25 to be merged)

elicoidal · 2016-09-30T16:17:14Z

business_requirement_deliverable_report/README.rst

+Introduction
+============
+
+This module is part of a set of modules (`Business Requirements <https://github.com/OCA/business-requirement/blob/8.0/README.md>`_) 


elicoidal · 2016-09-30T16:22:39Z

business_requirement_deliverable_report/__openerp__.py

+        'report/report.xml',
+    ],
+    'image': [
+        'static/img/bus_req_report1.png',


add the icon

Add Test Case in printable report (as it is in V8)

Update report_business_requirement.xml

…o 9.0-migrate_business_requirement_deliverable_report

sudhir-serpentcs · 2017-02-09T13:18:36Z

Rebased the PR.
@dreispt @pedrobaeza @jbeficent @moylop260 @elicoidal

elicoidal · 2017-02-10T01:18:19Z

@sudhir-serpentcs tested functionally. 3 small details for improvements in the 3 reports:

when saving the name should be "BR000001-test.pdf" (name+"-"+description)
there is a big gap between the header and the start of the page ("Business Requirement Document" text). Not sure this is not due to standard Odoo header spacing. Please confirm whether you can improve or not (simple spacing is enough)
The spacing between "Business Requirement Document" and "BR000001-test" is too big. "BR000001-test" should be centered below "Business Requirement Document"
(check example in Runbot)

sudhir-serpentcs · 2017-02-10T10:20:49Z

@elicoidal Thanks for you feedback. I will do the needful.
I guess there is not functionality of having dynamic report name (based on the value of the fields).

when saving the name should be "BR000001-test.pdf" (name+"-"+description)

Do you mean by report name or attachment name of the report?

elicoidal · 2017-02-11T00:36:03Z

@sudhir-serpentcs I mean attachment name of the report, thanks for pointing out

… generated reports.

sudhir-serpentcs · 2017-02-20T12:01:36Z

@elicoidal Added file attachment option and its name for the generated reports.

elicoidal

Not sure what was done but runbot says it is not working ;).
My downloaded attachments have respectively the names:

"Business Requirement Report.pdf",
"Business Requirement Deliverable Report.pdf" and
"Business Requirement Deliverable Resource Report.pdf"

Did I miss something?

Besides following issues are not yet attended (you plan them for later?):

there is a big gap between the header and the start of the page ("Business Requirement Document" text). Not sure this is not due to standard Odoo header spacing. Please confirm whether you can improve or not (simple spacing is enough)

The spacing between "Business Requirement Document" and "BR000001-test" is too big. "BR000001-test" should be centered below "Business Requirement Document"
(check example in Runbot)

elicoidal · 2017-02-21T13:33:46Z

business_requirement_deliverable_report/report/report.xml

+        name="business_requirement_deliverable_report.br_report"
+        file="business_requirement_deliverable_report.br_report"
+        attachment_use="True"
+        attachment="((object.name+object.description)+'.pdf')"


following the logic below: "'BR- '+((object.name+object.description)+'.pdf')"

@elicoidal Done

elicoidal · 2017-02-24T07:24:36Z

@sudhir-serpentcs Any news on this front?

… and the Body.

sudhir-serpentcs · 2017-02-27T10:31:06Z

there is a big gap between the header and the start of the page ("Business Requirement Document" text). Not sure this is not due to standard Odoo header spacing. Please confirm whether you can improve or not (simple spacing is enough)

The spacing between "Business Requirement Document" and "BR000001-test" is too big. "BR000001-test" should be centered below "Business Requirement Document"
(check example in Runbot)

@elicoidal Done. Please review.

elicoidal · 2017-02-27T12:54:34Z

@sudhir-serpentcs thanks for the update.

there is a big gap between the header and the start of the page ("Business Requirement Document" text). Not sure this is not due to standard Odoo header spacing. Please confirm whether you can improve or not (simple spacing is enough)

OK!

The spacing between "Business Requirement Document" and "BR000001-test" is too big. "BR000001-test" should be centered below "Business Requirement Document"
(check example in Runbot)

You can reduce the space between "Business Requirement Document" and "BR000001-test" to half the current size.

My downloaded attachments have respectively the names:
"Business Requirement Report.pdf",
"Business Requirement Deliverable Report.pdf" and
"Business Requirement Deliverable Resource Report.pdf"

The naming of the files is still incorrect when you save them.

elicoidal · 2017-03-08T01:41:06Z

@sudhir-serpentcs any feedback here?

sudhir-serpentcs · 2017-03-08T10:19:26Z

 AFAIU we have only left the name of the report. right @sudhir-serpentcs ?

@elicoidal Thanks Eric. Please check my previous comment for having report name.

elicoidal · 2017-03-08T10:46:03Z

@sudhir-serpentcs I saw your comment and agree. go ahead

… in Report Attachment option.

sudhir-serpentcs · 2017-03-09T10:39:00Z

@elicoidal Added code (controller) for the dynamic report name but you know we don't know how to make UT for controllers and that has reduced the codecov.

elicoidal · 2017-03-10T08:56:53Z

@sudhir-serpentcs I think we can live with the decrease coverage for the moment.

elicoidal · 2017-03-10T09:41:28Z

To be ported to v8 and v10

dreispt · 2017-03-12T10:27:00Z

Do we really need to extend the Report generation controller?
That sound like bad practice, and adds tight coupling between two components that should be independent.

Do we really need to customize the output file name?
I feel that that is a different feature that should be implemented in an OCA/report module, that could then be leveraged here.

elicoidal · 2017-03-12T12:45:31Z

@dreispt

That sound like bad practice, and adds tight coupling between two components that should be independent.

Not sure why this is bad practice. What I know is that when you have a project with dozens of BR, you want to have all of them properly identified and I currently spend manual time just to adapt that.
For me it is more the other way round: not having the filename properly generated at printing that looks like bad practice.
In short: if there are good reasons not to change the controller I will follow them. If there is a better way to centralize this feature then let's go for it: when it is ready when could remove the controller in this module.
What do you think?

dreispt · 2017-03-12T17:45:32Z

@elicoidal The feature is good.
Having it implemented together in this module looks clumsy.
Tomorrow someone will have a similar need and will also overwrite the report controller when building some XYZ repo.

For me these are two distinct features.
The controller code should be isolated in it's own module.
This module here should only care to provide the report itself.
I believe that having both these modules installed will have the desired effect, even if they don't depend on each other.

elicoidal · 2017-03-13T01:21:25Z

@sudhir-serpentcs
Can you split the controller part in a separate module business_requirement_deliverable_report_filenaming?
Once done we can merge this one.

dreispt · 2017-03-13T14:03:39Z

@sudhir-serpentcs @elicoidal AFAICS the report file naming feature is universal and not specific to BRs. I think it should be proposed to https://github.com/OCA/reporting-engine.

elicoidal · 2017-03-14T07:05:44Z

@sudhir-serpentcs @elicoidal AFAICS the report file naming feature is universal and not specific to BRs. I think it should be proposed to https://github.com/OCA/reporting-engine.

Fine by me: I have no clue about naming though (base_filename?)

elicoidal · 2017-03-15T07:34:33Z

@sudhir-serpentcs when refactored (removed the controller part), please port this module to v8 and v10

sudhir-serpentcs · 2017-03-15T07:42:08Z

@elicoidal Ok I will separate the report file naming feature (controller) in a new module (report_base_filename). Should I propose the PR in https://github.com/OCA/reporting-engine repo?
We need dynamic reporting name module in v8-v9 only because v10 already have this feature in base report module.

elicoidal · 2017-03-15T08:22:47Z

@sudhir-serpentcs go ahead

sudhir-serpentcs · 2017-03-15T12:09:17Z

@elicoidal Removed the code to have dynamic report name. I will add the reference of the PR that will have dynamic report name feature.

serpentcs-dev1 · 2017-03-15T13:26:31Z

@sudhir-serpentcs @elicoidal

Please find the PR here OCA/reporting-engine#114 which will help to have the
dynamic feature name on report.

elicoidal · 2017-03-15T22:36:13Z

@dreispt can we move forward and merge this PR?

dreispt

Thanks for your patience!

dreispt · 2017-03-16T09:36:49Z

PS: Reading the discussion at OCA/reporting-engine#114, I'm now sure that doing the separation was the right thing to do.

elicoidal · 2017-03-16T10:07:18Z

@dreispt indeed thanks for your help :)

[ADD] Migrated business_requirement_deliverable_report module from v8…

31591c6

… to v9.

elicoidal suggested changes Sep 30, 2016

View reviewed changes

elicoidal reviewed Sep 30, 2016

View reviewed changes

elicoidal mentioned this pull request Sep 30, 2016

Migration to version 9.0 #20

Closed

8 tasks

elicoidal added the needs review label Sep 30, 2016

elicoidal and others added 6 commits September 30, 2016 18:32

Update README.rst

cb094e4

Update README.rst

6d8481f

Update report_business_requirement.xml

bdd3a3c

Add Test Case in printable report (as it is in V8)

Merge pull request #7 from astirpe/patch-4

857aa3a

Update report_business_requirement.xml

[IMP] added icon

262478f

Merge branch '9.0' of https://github.com/OCA/business-requirement int…

4e72068

…o 9.0-migrate_business_requirement_deliverable_report

elicoidal approved these changes Feb 9, 2017

View reviewed changes

[ADD] Added report file attachment option and attachment name for the…

0e6ed46

… generated reports.

elicoidal suggested changes Feb 22, 2017

View reviewed changes

elicoidal added needs fixing and removed needs review labels Feb 24, 2017

elicoidal changed the title ~~[ADD] Migrated business_requirement_deliverable_report module from v8 to v9~~ [9.0][ADD] Migrated business_requirement_deliverable_report module from v8 to v9 Feb 25, 2017

elicoidal added this to the 9.0 milestone Feb 25, 2017

[IMP] Added paper format to remove the blank space between the Header…

7a895e0

… and the Body.

sudhir-serpentcs force-pushed the 9.0-migrate_business_requirement_deliverable_report branch from 392e40f to 7a895e0 Compare February 27, 2017 10:17

[ADD] Added code to have dynamic Report names based on the name given…

fe85a79

… in Report Attachment option.

elicoidal approved these changes Mar 10, 2017

View reviewed changes

elicoidal added needs review and removed needs fixing labels Mar 10, 2017

elicoidal added needs fixing and removed needs review labels Mar 15, 2017

[REM] Removed the code to have Dynamic Report name.

0ff28ac

dreispt approved these changes Mar 16, 2017

View reviewed changes

dreispt merged commit 81f2b77 into OCA:9.0 Mar 16, 2017

sudhir-serpentcs deleted the 9.0-migrate_business_requirement_deliverable_report branch March 16, 2017 09:55

elicoidal removed the needs fixing label May 16, 2017

[9.0][ADD] Migrated business_requirement_deliverable_report module from v8 to v9 #31

[9.0][ADD] Migrated business_requirement_deliverable_report module from v8 to v9 #31

Conversation

sudhir-serpentcs commented Sep 30, 2016 • edited by dreispt Loading

elicoidal commented Sep 30, 2016

elicoidal Sep 30, 2016

Choose a reason for hiding this comment

elicoidal Sep 30, 2016

Choose a reason for hiding this comment

sudhir-serpentcs commented Feb 9, 2017

elicoidal commented Feb 10, 2017

sudhir-serpentcs commented Feb 10, 2017 • edited Loading

elicoidal commented Feb 11, 2017

sudhir-serpentcs commented Feb 20, 2017

elicoidal left a comment

Choose a reason for hiding this comment

elicoidal Feb 21, 2017

Choose a reason for hiding this comment

sudhir-serpentcs Feb 27, 2017

Choose a reason for hiding this comment

elicoidal commented Feb 24, 2017

sudhir-serpentcs commented Feb 27, 2017

elicoidal commented Feb 27, 2017

elicoidal commented Mar 8, 2017

sudhir-serpentcs commented Mar 8, 2017 • edited Loading

elicoidal commented Mar 8, 2017

sudhir-serpentcs commented Mar 9, 2017 • edited Loading

elicoidal commented Mar 10, 2017

elicoidal commented Mar 10, 2017

dreispt commented Mar 12, 2017

elicoidal commented Mar 12, 2017

dreispt commented Mar 12, 2017

elicoidal commented Mar 13, 2017

dreispt commented Mar 13, 2017

elicoidal commented Mar 14, 2017

elicoidal commented Mar 15, 2017

sudhir-serpentcs commented Mar 15, 2017

elicoidal commented Mar 15, 2017

sudhir-serpentcs commented Mar 15, 2017 • edited Loading

serpentcs-dev1 commented Mar 15, 2017 • edited Loading

elicoidal commented Mar 15, 2017

dreispt left a comment

Choose a reason for hiding this comment

dreispt commented Mar 16, 2017

elicoidal commented Mar 16, 2017

sudhir-serpentcs commented Sep 30, 2016 •

edited by dreispt

Loading

sudhir-serpentcs commented Feb 10, 2017 •

edited

Loading

sudhir-serpentcs commented Mar 8, 2017 •

edited

Loading

sudhir-serpentcs commented Mar 9, 2017 •

edited

Loading

sudhir-serpentcs commented Mar 15, 2017 •

edited

Loading

serpentcs-dev1 commented Mar 15, 2017 •

edited

Loading