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

Business requirement deliverable report #15

Merged
merged 22 commits into from
Sep 15, 2016

Conversation

RawEvan
Copy link
Contributor

@RawEvan RawEvan commented Aug 23, 2016

The report module for business requirement using Qweb.

Depends on:

  

@oca-clabot
Copy link

Hey @RawEvan, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

  • @RawEvan (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

@coveralls
Copy link

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-5.07%) to 85.356% when pulling dd3c649 on RawEvan:business_requirement_deliverable_report into 40cb2a0 on OCA:8.0.

@elicoidal
Copy link
Contributor

@RawEvan Can you please make sure Travis is Green?

@elicoidal
Copy link
Contributor

I have fixed the CLA (you are currently covered by the company CLA)

@RawEvan RawEvan changed the title Business requirement deliverable report [WIP]Business requirement deliverable report Aug 23, 2016
@coveralls
Copy link

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-4.0%) to 86.441% when pulling 6caeac3 on RawEvan:business_requirement_deliverable_report into 40cb2a0 on OCA:8.0.

@RawEvan
Copy link
Contributor Author

RawEvan commented Aug 23, 2016

@elicoidal The Travis is green now.

@RawEvan RawEvan force-pushed the business_requirement_deliverable_report branch from 6caeac3 to daf6c19 Compare August 24, 2016 02:25
@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-3.9%) to 86.722% when pulling daf6c19 on RawEvan:business_requirement_deliverable_report into 8442e3f on OCA:8.0.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-3.9%) to 86.722% when pulling 5b47584 on RawEvan:business_requirement_deliverable_report into 8442e3f on OCA:8.0.

@RawEvan
Copy link
Contributor Author

RawEvan commented Aug 24, 2016

@elicoidal It has been rebased, and the bug of the page number is fixed.

@elicoidal
Copy link
Contributor

please increase coverage.
I wil test in runbot

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage decreased (-3.9%) to 86.722% when pulling 5c5a0e1 on RawEvan:business_requirement_deliverable_report into 8442e3f on OCA:8.0.

@elicoidal
Copy link
Contributor

@RawEvan not bad!
Some details:

  • qty in deliverable should be right justified, along with uom
  • in resource line, qty and uom should be right justified
  • Procurement should be reduced as "Proc" (keep Tasks as is)
  • the decimal accuracy on amount should be the one from price and for quantity the one from UoM (from decimal accuracy setup)

Other than that LGTM: good job

@elicoidal
Copy link
Contributor

@RawEvan One other thing: can we reduce the space at the top of the document? (between header and the BR number)

@RawEvan
Copy link
Contributor Author

RawEvan commented Aug 25, 2016

@elicoidal Yes, the space can be reduced, and the details above are also working in progress.

@victormmtorres
Copy link
Collaborator

@RawEvan Don't forgot about increase or maintain coverall.

…uce the space under the header, change the precision, right justfied some columns, delete unnecessary py files
@coveralls
Copy link

coveralls commented Aug 29, 2016

Coverage Status

Coverage remained the same at 90.654% when pulling 1c3a8f2 on RawEvan:business_requirement_deliverable_report into 8442e3f on OCA:8.0.

@RawEvan
Copy link
Contributor Author

RawEvan commented Aug 29, 2016

@elicoidal The report is updated including the 5 details that you proposed except the decimal accuracy for quantity (but the decimal accuracy of price is OK now).
@victormartinelicocorp The coverall is maintained now, thanks for your reminding.

@elicoidal
Copy link
Contributor

only modification and then we can merge. Add "Business Requirement Document" in the first line before the sequence.
other than that, LGTM

@elicoidal
Copy link
Contributor

@dreispt @pedrobaeza can we merge this one?

Introduction
^^^^^^^^^^^^

This module is part of a set ("Business Requirement").
Copy link
Member

Choose a reason for hiding this comment

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

I think this generic description should be in the main module, but not copied on each submodule. You can put something like: Refer to the documentation on the main module to know what is a business requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem: I will adapt the README accordingly.
I miss some place in our website to gather the documentation...

@dreispt
Copy link
Sponsor Member

dreispt commented Sep 9, 2016

If this is ready for review please remove the WIP from the title.

'static/img/bus_req_report1.png',
'static/img/bus_req_report2.png',
'static/img/bus_req_report3.png',
],
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

As per Pedro's comment, keep only what is specifically relevant for this module features. You should remove images that don't illustrate features implemented here.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

@dreispt
Copy link
Sponsor Member

dreispt commented Sep 9, 2016

Looks good, once comments are fixed.

@elicoidal
Copy link
Contributor

@pedrobaeza @dreispt thanks for the review. We will amend accordingly

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 90.991% when pulling e6aaccd on RawEvan:business_requirement_deliverable_report into 8442e3f on OCA:8.0.

@RawEvan RawEvan changed the title [WIP]Business requirement deliverable report Business requirement deliverable report Sep 12, 2016
@RawEvan
Copy link
Contributor Author

RawEvan commented Sep 12, 2016

@elicoidal @pedrobaeza @dreispt It's fixed.

@elicoidal
Copy link
Contributor

👍
@pedrobaeza @dreispt can we merge this one?

@pedrobaeza
Copy link
Member

👍

@pedrobaeza
Copy link
Member

Please squash your commits by author and I'll merge.

@elicoidal
Copy link
Contributor

@RawEvan Please proceed with squash

@dreispt
Copy link
Sponsor Member

dreispt commented Sep 15, 2016

@elicoidal "Squash and Merge" gives authorship to the last commiter, so it should be fine. I'm merging.

@dreispt dreispt merged commit 90b750a into OCA:8.0 Sep 15, 2016
elicoidal pushed a commit that referenced this pull request Oct 4, 2016
Remove useless introduction from README.rst
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.

None yet

8 participants