Skip to content

mgmtsystem_kpi rename to kpi - #543

Merged
dreispt merged 8 commits into
OCA:9.0from
naousse:9.0-kpi
Dec 2, 2016
Merged

mgmtsystem_kpi rename to kpi#543
dreispt merged 8 commits into
OCA:9.0from
naousse:9.0-kpi

Conversation

@naousse

@naousse naousse commented Sep 6, 2016

Copy link
Copy Markdown

No description provided.

@max3903 max3903 added this to the 9.0 milestone Sep 6, 2016
@max3903 max3903 self-assigned this Sep 6, 2016
max3903
max3903 reviewed Sep 6, 2016
View reviewed changes
Comment thread kpi/README.rst

When all actions of the plan are done, their effectiveness must be evaluated
before closing the NC.

@max3903 max3903 Sep 6, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@naousse Replace this usage section (from the nonconformity module) with the link to the video I sent you.

@naousse

naousse commented Sep 6, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

base.group_hr_user is the employee user group i have and it is defined here:
/home/odoo/odoo-dev/odoo/addons/hr/security/hr_security.xml.
Without the hr dependance the system output that id is missed and when i add the hr dependance, everything works fine.

@max3903

max3903 commented Sep 7, 2016

Copy link
Copy Markdown
Member

@naousse This is the wrong group. The Employee group (base.group_user) is created here:
https://github.com/odoo/odoo/blob/9.0/openerp/addons/base/security/base_security.xml#L24

@naousse

naousse commented Sep 7, 2016

Copy link
Copy Markdown
Author

Everything is ok now

@max3903

max3903 commented Sep 7, 2016

Copy link
Copy Markdown
Member

@naousse Please review: naousse#1

@naousse

naousse commented Sep 7, 2016

Copy link
Copy Markdown
Author

👍
I haven't paid attention about that

@max3903

max3903 commented Sep 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@max3903

max3903 commented Sep 8, 2016

Copy link
Copy Markdown
Member

@naousse Can you rebase please?

@max3903

max3903 commented Sep 9, 2016

Copy link
Copy Markdown
Member

@naousse Can you rebase again please? Runbot should be usable now.

@max3903

max3903 commented Sep 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@naousse Few UI changes:

KPI

  • Hide the company field on the list and form view. Should be displayed to multi_company group only.

Thresholds

  • Hide the company field on the list and form view. Should be displayed to multi_company group only.
  • Move the company field on the same line as the name field
  • Add the label for the name
  • Hide the company field on the list view of Ranges and KPIs. Should be displayed to multi_company group only.

Ranges

  • Hide the company field on the list and form view. Should be displayed to multi_company group only.
  • Add the label for the name and color
  • Name and company field should be on the same line
  • Type of min and max takes the whole line. Half of it is fine.
  • Fixed value and the datasource should be on the same line as the type.
  • Hide the company field on the list view of Thresholds. Should be displayed to multi_company group only.

@max3903

max3903 commented Sep 17, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@naousse There is one error on Runbot:
openerp.addons.base.ir.ir_cron:139 _callback Model 'mgmtsystem.kpi' does not exist.

KPI History

  • Hide the company field on the list and form view. Should be displayed to multi_company group only.

Ranges

  • The minimum and maximum columns are empty on the list view.

max3903
max3903 approved these changes Sep 19, 2016
View reviewed changes

@max3903 max3903 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

elicoidal
elicoidal suggested changes Sep 20, 2016
View reviewed changes
Comment thread kpi/README.rst Outdated
Bugs are tracked on `GitHub Issues <https://github.com/OCA/Management-system/issues>`_.
In case of trouble, please check there if your issue has already been reported.
If you spotted it first, help us smashing it by providing a detailed and welcomed feedback `here <https://github.com/OCA/
server-tools/issues/new?body=module:%20

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this line following newest templates

Comment thread kpi/README.rst Outdated
* color (RGB code like #00FF00 for green, #FFA500 for orange, #FF0000 for red)


Installation

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove this section

Comment thread kpi/README.rst Outdated
mission is to support the collaborative development of Odoo features and
promote its widespread use.

To contribute to this module, please visit http://odoo-community.org.

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

https

Comment thread kpi/__openerp__.py Outdated
@@ -65,6 +48,5 @@
],
"demo": [],

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove these 2 keys

Comment thread kpi/models/kpi.py Outdated
}
history_obj = self.env['kpi.history']
history_obj.create(values)
# obj.history_ids = history_obj.search([("kpi_id", "=", obj.id)])

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove dead code

Comment thread kpi/models/kpi.py Outdated
res = None

try:
# res = self.compute_kpi_value()

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove dead code

Comment thread kpi/models/kpi_category.py
@@ -0,0 +1,32 @@
# -*- coding: utf-8 -*-

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you use short headers

@elicoidal elicoidal Sep 29, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry: there must have been a misunderstanding: can you use short headers similar to https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L1 including your actual information?

Comment thread kpi/models/kpi_threshold.py Outdated
if obj.valid:
result[obj.id] = ""
else:
result[obj.id] = ("2 of your ranges are overlapping! Please "

@elicoidal elicoidal Sep 20, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/2/Two

@naousse naousse Sep 21, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@elicoidal I made changes base on your review. You can check if everything is correct. Please I don't understand what you mean here

@elicoidal elicoidal Sep 22, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@naousse In English (and in French too actually), you normally do not start a sentence with a number but number writen in plain English hence my suggestion to change "2" by "Two".

@max3903

max3903 commented Sep 28, 2016

Copy link
Copy Markdown
Member

@elicoidal Can you update your review please?

@max3903

max3903 commented Sep 28, 2016

Copy link
Copy Markdown
Member

Ping @dreispt

elicoidal
elicoidal reviewed Sep 29, 2016
View reviewed changes

@elicoidal elicoidal left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think there has been a misunderstanding about the headers. Please check the example.
Note that you should have copyright notices in xml files as well (check: https://github.com/OCA/maintainer-tools/blob/master/template/module/data/model_name_data.xml#L2)

Comment thread kpi/models/__init__.py
@@ -0,0 +1,7 @@
# -*- coding: utf-8 -*-

@elicoidal elicoidal Sep 29, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't you need copyright notice?

Comment thread kpi/models/kpi.py
@@ -0,0 +1,187 @@
# -*- coding: utf-8 -*-

@elicoidal elicoidal Sep 29, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't you need copyright notice?

Comment thread kpi/models/kpi_category.py
@@ -0,0 +1,32 @@
# -*- coding: utf-8 -*-

@elicoidal elicoidal Sep 29, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry: there must have been a misunderstanding: can you use short headers similar to https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L1 including your actual information?

Comment thread kpi/views/kpi.xml Outdated
<field name="arch" type="xml">
<search string="KPI">
<group>
<filter name="active" icon="terp-document-new" domain="[('active','=',True)]" string="Active" help="Only active KPIs are computed by the scheduler based on the periodicity configuration."/>

@elicoidal elicoidal Sep 29, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you try to improve the format of the line for readability (like above one)

@naousse naousse mentioned this pull request Oct 3, 2016
Merged
1 task
@max3903

max3903 commented Oct 13, 2016

Copy link
Copy Markdown
Member

@naousse Can you fix based on @elicoidal latest comments? Thanks!

@max3903 max3903 mentioned this pull request Oct 19, 2016
Closed
dreispt
dreispt approved these changes Oct 23, 2016
View reviewed changes

@dreispt dreispt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread kpi/__init__.py
# Copyright <YEAR(S)> Loic Lacroix <loic.lacroix@savoirfairelinux.com>
# Copyright <YEAR(S)> Sandy Carter <sandy.carter@savoirfairelinux.com>
# Copyright <YEAR(S)>Gervais Naoussi <gervaisnaoussi@gmail.com>
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

@dreispt dreispt Oct 23, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO copyright is attributed to original authors only, and contributors should not be included on authorship and copyright attribution.

@max3903

max3903 commented Oct 24, 2016

Copy link
Copy Markdown
Member

@elicoidal Can you update your review? Copyrights headers are correct now.

elicoidal
elicoidal reviewed Oct 25, 2016
View reviewed changes
Comment thread kpi/__init__.py Outdated
@@ -0,0 +1,5 @@
# -*- coding: utf-8 -*-
# Copyright <YEAR(S)> Savoir-faire Linux <https://www.savoirfairelinux.com/>

@elicoidal elicoidal Oct 25, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please update the year

elicoidal
elicoidal reviewed Oct 25, 2016
View reviewed changes
Comment thread kpi/models/kpi_threshold.py Outdated
range_obj1.min_value < range_obj2.min_value):
if range_obj1.max_value > range_obj2.min_value:
raise exceptions.Warning(
_("Two of your ranges are overlapping!"),

@elicoidal elicoidal Oct 25, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid "!" in error message, it makes users uncomfortable

@elicoidal

elicoidal commented Oct 25, 2016

Copy link
Copy Markdown

Please fix the copyright then 👍 on my side

@naousse

naousse commented Oct 26, 2016

Copy link
Copy Markdown
Author

@max3903 Please can you provide the right year for the copyright?

@max3903

max3903 commented Oct 26, 2016

Copy link
Copy Markdown
Member

@naousse 2012 - Now

@dreispt

dreispt commented Dec 2, 2016

Copy link
Copy Markdown
Member

@elicoidal fix request was done - merging!

@dreispt
dreispt merged commit 40694c4 into OCA:9.0 Dec 2, 2016
AdriaGForgeFlow pushed a commit to ForgeFlow/server-tools that referenced this pull request Nov 12, 2018
@naousse @AdriaGForgeFlow
mgmtsystem_kpi rename to kpi (OCA#543)
c017dc6 
* kpi migration to odoo 9
* UI error connected
* Correction base on last maxime review
* correction on @elicoidal review
* Warning exception improve in kpi_threshold
* Latest Ellicoidal comment implemented
* last Ellicoial recommendation done.
* Copyright corrected
AdriaGForgeFlow pushed a commit to ForgeFlow/server-tools that referenced this pull request Nov 12, 2018
@naousse @AdriaGS
mgmtsystem_kpi rename to kpi (OCA#543)
27e2911 
* kpi migration to odoo 9
* UI error connected
* Correction base on last maxime review
* correction on @elicoidal review
* Warning exception improve in kpi_threshold
* Latest Ellicoidal comment implemented
* last Ellicoial recommendation done.
* Copyright corrected
JordiBForgeFlow pushed a commit to ForgeFlow/server-tools that referenced this pull request Nov 27, 2018
@naousse @JordiBForgeFlow
mgmtsystem_kpi rename to kpi (OCA#543)
db2f727 
* kpi migration to odoo 9
* UI error connected
* Correction base on last maxime review
* correction on @elicoidal review
* Warning exception improve in kpi_threshold
* Latest Ellicoidal comment implemented
* last Ellicoial recommendation done.
* Copyright corrected
AdriaGForgeFlow pushed a commit to ForgeFlow/server-tools that referenced this pull request Dec 3, 2018
@naousse @AdriaGForgeFlow
mgmtsystem_kpi rename to kpi (OCA#543)
5230f31 
* kpi migration to odoo 9
* UI error connected
* Correction base on last maxime review
* correction on @elicoidal review
* Warning exception improve in kpi_threshold
* Latest Ellicoidal comment implemented
* last Ellicoial recommendation done.
* Copyright corrected
JordiBForgeFlow pushed a commit to ForgeFlow/server-tools that referenced this pull request Jan 7, 2019
@naousse @JordiBForgeFlow
mgmtsystem_kpi rename to kpi (OCA#543)
388713a 
* kpi migration to odoo 9
* UI error connected
* Correction base on last maxime review
* correction on @elicoidal review
* Warning exception improve in kpi_threshold
* Latest Ellicoidal comment implemented
* last Ellicoial recommendation done.
* Copyright corrected
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
@bt-admin
Merge pull request OCA#543 from OCA/12.0
0123551 
Syncing from upstream OCA/server-tools (12.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@max3903 max3903 max3903 approved these changes
@dreispt dreispt dreispt approved these changes
+1 more reviewer
@elicoidal elicoidal elicoidal requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@max3903 max3903

Labels

needs review

Projects

None yet

Milestone

9.0

Development

Successfully merging this pull request may close these issues.

4 participants

@naousse @max3903 @elicoidal @dreispt