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.

Sign up for GitHub

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

Jump to bottom

MIG mgmtsystem_system to V. 10.0 #166

Merged
merged 1 commit into from
Mar 27, 2017
Merged

MIG mgmtsystem_system to V. 10.0 #166

merged 1 commit into from
Mar 27, 2017

Conversation

eugen-don
Copy link
Member

MIG mgmtsystem_system to V. 10.0

@max3903 max3903 modified the milestone: 10.0 Mar 18, 2017
@pedrobaeza pedrobaeza mentioned this pull request Mar 18, 2017
Closed
31 tasks
@max3903 max3903 self-requested a review March 18, 2017 16:09
dreispt
dreispt reviewed Mar 20, 2017
View reviewed changes
Copy link
Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

I see some functional changes I'm not sure about.

mgmtsystem/__manifest__.py Outdated
"author": "Savoir-faire Linux,Odoo Community Association (OCA)",
"website": "http://www.savoirfairelinux.com",
"license": "AGPL-3",
"category": "Management System",
'images': ['images/mgmtsystem.png', 'images/mgmtsystem-hover.png'],
"complexity": "normal",
Copy link
Member

Choose a reason for hiding this comment

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

complexity key is deprecated, please remove

mgmtsystem/security/mgmtsystem_security.xml Outdated
@@ -18,22 +18,22 @@
<field name="name">User</field>
<field name="comment">Module user, with general read and write.</field>
<field name="category_id" ref="module_category_management_system"/>
<field name="implied_ids" eval="[(4,ref('mgmtsystem.group_mgmtsystem_viewer'))]"/>
<field name="implied_ids" eval="(4,ref('mgmtsystem.group_mgmtsystem_viewer'))"/>
Copy link
Member

Choose a reason for hiding this comment

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

Per documentation, the square brackets are required.

mgmtsystem/views/menus.xml Outdated
/>

<menuitem
id="menu_mgmtsystem_main"
name="Management System"
parent="menu_mgmtsystem_root"
sequence="0"
groups="group_mgmtsystem_viewer"/>
groups="base.group_user"/>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change? It unnecessarily forces access to everyone.

@dreispt dreispt mentioned this pull request Mar 20, 2017
Merged
2 tasks
@eugen-don eugen-don mentioned this pull request Mar 22, 2017
Merged
1 task
@max3903
Copy link
Member

max3903 commented Mar 23, 2017

@dreispt Can we merge?

grindtildeath
grindtildeath reviewed Mar 23, 2017
View reviewed changes
Copy link

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

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

Just a few things, otherwise looks quite good imo.

mgmtsystem/views/menus.xml Outdated
@@ -34,4 +34,4 @@
parent="mgmtsystem.menu_mgmtsystem_root"
groups="group_mgmtsystem_manager"
sequence="100"/>
</odoo>
</odoo>
Copy link

Choose a reason for hiding this comment

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

Missing EOL

mgmtsystem/views/mgmtsystem_system.xml Outdated
@@ -1,12 +1,15 @@
<?xml version="1.0" encoding="utf-8"?>
<odoo>
<record model="ir.ui.view" id="system_form">
<data>
Copy link

Choose a reason for hiding this comment

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

Why add data ?

mgmtsystem/models/mgmtsystem_system.py Outdated
from odoo import models, fields

import logging
_logger = logging.getLogger(__name__)
Copy link

Choose a reason for hiding this comment

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

Do we need this ?

mgmtsystem/views/mgmtsystem_system.xml Outdated
@@ -35,8 +39,10 @@
<menuitem
Copy link

Choose a reason for hiding this comment

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

Why not move it to views/menus.xml ?

@eugen-don eugen-don changed the title MIG mgmtsystem_system to V. 10.0 WIP MIG mgmtsystem_system to V. 10.0 Mar 23, 2017
@eugen-don eugen-don force-pushed the MIG-mgmtsystem_system-to-V.-10.0 branch from 35ad5a2 to a4033ba Compare March 25, 2017 15:04
@eugen-don eugen-don changed the title WIP MIG mgmtsystem_system to V. 10.0 MIG mgmtsystem_system to V. 10.0 Mar 25, 2017
This was referenced Mar 26, 2017
Merged
Merged
@eugen-don
Copy link
Member Author

eugen-don commented Mar 26, 2017 •
edited
Loading

I did a rebase on 10.0 to compress commit history... sorry for that... could you review once more?

@eugen-don eugen-don force-pushed the MIG-mgmtsystem_system-to-V.-10.0 branch from 417475c to 472fc35 Compare March 27, 2017 06:11
@eugen-don
Copy link
Member Author

runbot, travis, and cc passed, this can be merged

sbidoul
sbidoul reviewed Mar 27, 2017
View reviewed changes
mgmtsystem/__manifest__.py
@@ -39,5 +23,5 @@
'views/res_config.xml'
],
"demo": [],
'installable': False,
'installable': True,
}
Copy link
Member

Choose a reason for hiding this comment

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

Hi, In the changelog it's written "the module does no depends anymore on document_page module", however in the manifest the dependency on document_page is present. Which one is correct?

Copy link
Member

@sbidoul sbidoul Mar 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@eugen-don @max3903 @dreispt related, I notice there is a dependency on board that generates warnings during installation, and does not seem to be used inside this mgtsystem module.

The warnings/errors I get are

WARNING test-mgtsystem-v10c odoo.modules.registry: Models have no table: board.board.
INFO test-mgtsystem-v10c odoo.modules.registry: Recreate table of model board.board.
ERROR test-mgtsystem-v10c odoo.modules.registry: Model board.board has no table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @sbidoul
thanks nice spot.

Copy link
Member Author

@eugen-don eugen-don Mar 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@sbidoul
i just checked the 9.0 openerp.py
and it had still board and document_page in the dependencies even though readme tells its not depending on document_page anymore.

I testet the base module and the other that ive ported, they all install without board, but some have document_page as a dependency.

So its save to remove board and document_page from dependencies in the core module.

edit:
it seems @naousse removed the dependency in his port to v 9.0 and added it back in this commit later on: dc051b6#diff-6061d36dd5e773bfa61a36521d74ca81

Copy link
Member

Choose a reason for hiding this comment

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

document_page should be a dependency of mgmtsystem_manual, _procedure and _work_instruction

Copy link
Member Author

@eugen-don eugen-don Mar 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@max3903
at this moment mgmtsystem_manual and document_page_work_instruction depend both on document_page_approval and document_page_procedure depends on document_page_work_instruction.
document_page_approval depends on document_page

@eugen-don
MIG mgmtsystem_system to V. 10.0
c7da484 
MIG mgmtsystem_system to V. 10.0
@eugen-don eugen-don force-pushed the MIG-mgmtsystem_system-to-V.-10.0 branch from 4a26700 to c7da484 Compare March 27, 2017 15:33
@max3903 max3903 merged commit ae5e907 into OCA:10.0 Mar 27, 2017
@eugen-don eugen-don deleted the MIG-mgmtsystem_system-to-V.-10.0 branch March 27, 2017 17:05
@eugen-don eugen-don restored the MIG-mgmtsystem_system-to-V.-10.0 branch March 27, 2017 17:56
@eugen-don eugen-don deleted the MIG-mgmtsystem_system-to-V.-10.0 branch March 27, 2017 18:02
@eugen-don eugen-don restored the MIG-mgmtsystem_system-to-V.-10.0 branch March 29, 2017 13:07
@eugen-don eugen-don deleted the MIG-mgmtsystem_system-to-V.-10.0 branch April 18, 2017 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreispt dreispt dreispt left review comments

@max3903 max3903 max3903 approved these changes

@grindtildeath grindtildeath grindtildeath left review comments

@sbidoul sbidoul sbidoul left review comments

Assignees

@eugen-don eugen-don

Labels
needs review
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@eugen-don @max3903 @sbidoul @grindtildeath @dreispt