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

[8.0][WIP] Added Earned Value Management Report #107

Conversation

sudhir-serpentcs
Copy link
Contributor

@elicoidal Added Earned Value Management Report.

@elicoidal elicoidal added this to the 8.0 milestone Apr 5, 2017
Copy link
Contributor

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

Some remarks.
Besides, to be tested.

tools.drop_view_if_exists(cr,
'business_requirement_earned_value_report')
cr.execute("""
CREATE VIEW business_requirement_earned_value_report AS (
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a lot of div by zero potential issues in the current view.

@@ -181,7 +181,7 @@ def _compute_resource_task_total(self):
br.mapped('deliverable_lines').mapped(
'resource_ids').filtered(
lambda r: r.resource_type == 'task').mapped(
'price_total'))
Copy link
Contributor

Choose a reason for hiding this comment

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

irrelevant

Copy link
Member

Choose a reason for hiding this comment

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

Don't make this change in the PR.

@@ -0,0 +1,5 @@
# -*- coding: utf-8 -*-
# © 2016 Elico Corp (www.elico-corp.com).
Copy link
Contributor

Choose a reason for hiding this comment

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

2017 (everywhere)

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal
Please review code.

Copy link
Contributor

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

Almost there!

<field name="view_id" ref="view_earned_value_graph"/>
</record>

<menuitem action="action_earned_value_report" id="menu_action_deliverable_report"
Copy link
Contributor

Choose a reason for hiding this comment

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

This report should only visible to the cost group

@elicoidal
Copy link
Contributor

almost there!

@elicoidal
Copy link
Contributor

please check Travis and Runbot as well

Copy link
Collaborator

@victormmtorres victormmtorres left a comment

Choose a reason for hiding this comment

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

Please have a check and amend if necessary

readonly=True)
res_product = fields.Many2one('product.product', 'Res Product',
readonly=True)
hr_timesheet_product =\
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sudhir-serpentcs layout is not ok.
as example do as here:
hr_timesheet_product = fields.Many2one(
'product.product',
'HR Timesheet Product',
...,)

And amend for the others fields as well

res.unit_price) as per_variances,
pt.remaining_hours,
(pt.effective_hours + pt.remaining_hours) as total_expected_time,
CASE WHEN (pt.effective_hours + pt.remaining_hours) > 0 THEN
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sudhir-serpentcs if
pt.effective_hours = 0
pt.remaining_hours = 2

condition (pt.effective_hours + pt.remaining_hours) > 0 is true then a division 0/2 on Postgress will cause error, don't you think?

Choose a reason for hiding this comment

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

@victormartinelicocorp

no if 0/2
condition should be (pt.remaining_hours) > 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please amend

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @victormartinelicocorp
Added group and Improved code.
please review code.

</record>

<menuitem action="action_earned_value_report" id="menu_action_deliverable_report"
parent="base.menu_project_report" groups="business_requirement_deliverable_cost.group_business_requirement_cost_control"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please improve formatting (one attribute one line)

{
'name': ' Earned Value Management',
'category': 'Business Requirements Management',
'summary': 'Manage the Business Requirement Deliverables and \
Copy link
Contributor

Choose a reason for hiding this comment

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

BI report for the based on Earned Value Management

Copy link
Contributor

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

Please add a README file (based on business_requirement module) so that I can create the content

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @victormartinelicocorp

Added README.rst and Improved code.

@elicoidal
Copy link
Contributor

elicoidal commented Apr 9, 2017

I made some functional tests and I have several feedbacks.

  1. I cannot relate the report result with the set up I did in the BR.
    Here are the DL/RL (101 in total)
    screenshot from 2017-04-09 19-28-43
    screenshot from 2017-04-09 19-28-32

Here are the timesheets I have inputed:
screenshot from 2017-04-09 19-27-03

Here is the result:
screenshot from 2017-04-09 19-26-23

I would have expected:

  • Planned Hours: 101 instead of 404
  • Product cost: 25.099 (is a calculation as we cannot get actual cost for multiple lines: cost/hours)
  • Cost: 2535 instead of 10140
  • Actual time: 60 instead of 240
  • Product cost from TS: 30 instead 600 (could be calculation: Actual cost / Actual time)
  • Actual Cost: 60x30= 1800 instead of 18000
    (rest of column are obviously calculated).
  1. Besides can you change the following column string:
  • Planned Time in RL: Planned Time
  • Product Cost in RL: Plan. Unit Cost
  • Actual Time in Timesheet => Actual Time
  • Product Cost from Timesheet Product => Act. Unit Cost
  1. Other remarks
  • If there is no timesheet, the report is empty: it should display the BR content with actual time = 0
  • Name: should be BR name+description

Copy link
Contributor

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

Failed functional tests

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @victormartinelicocorp
please review code.

@elicoidal
Copy link
Contributor

@sudhir-serpentcs
Retested and still not correct (although seems improving: now it is double instead of quadruple...).
screenshot from 2017-04-13 09-12-14

Please check the test I have written in #107 (comment) before submitting for review.

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @victormartinelicocorp

please review code

@elicoidal
Copy link
Contributor

elicoidal commented Apr 14, 2017

@sudhir-serpentcs it is better. Some additional feedback:

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @victormartinelicocorp

please review code

@elicoidal
Copy link
Contributor

For #107 (comment) all is clear!
Small details I want to adjust:

  • Total expected time => Total Exp. time
  • % Project Completion => % Completion
    After that, good to merge :)

@sudhir-serpentcs
Copy link
Contributor Author

@elicoidal @victormartinelicocorp

please review code

@elicoidal
Copy link
Contributor

Good job! @sudhir-serpentcs

@elicoidal
Copy link
Contributor

@seb-elico @victormartinelicocorp can you review?

@elicoidal
Copy link
Contributor

@pedrobaeza @dreispt @seb-elico @victormartinelicocorp Let's move forward?

@@ -181,7 +181,7 @@ def _compute_resource_task_total(self):
br.mapped('deliverable_lines').mapped(
'resource_ids').filtered(
lambda r: r.resource_type == 'task').mapped(
'price_total'))
Copy link
Member

Choose a reason for hiding this comment

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

Don't make this change in the PR.

@sudhir-serpentcs
Copy link
Contributor Author

@pedrobaeza @elicoidal

please review code

@pedrobaeza pedrobaeza merged commit fe7fe7b into OCA:8.0 Apr 17, 2017
@sudhir-serpentcs sudhir-serpentcs deleted the 8.0-19936-BR000803-Earned_Value_Management_Report branch July 11, 2017 06:00
ruter-lyu pushed a commit to ruter-lyu/business-requirement that referenced this pull request Mar 22, 2019
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.

5 participants