-
-
Notifications
You must be signed in to change notification settings - Fork 294
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
[REF] 8.0 mgmtsystem hazard #28
Conversation
Needs tests |
Ok going to add tests |
3e6d26c
to
1a043d0
Compare
The problem in this PR is that it tries to set a field as required on res_company without setting a default value during an alter table. Existing records will trigger a constraint error.
We have to decide between adding a default value and keeping it required=True, or set it required=False and update some views with require on certain states. |
You can set A * B * C as the default value and add this configuration step in the README.rst |
1a043d0
to
aed0172
Compare
8b7f2e3
to
8298e28
Compare
8298e28
to
d61687d
Compare
Needs test for:
|
@max3903 @bwrsandman It seems that these modules are not multi-system at all and the multi-company seems incomplete. I think that most models should have both a system_id field and a company_id field (related to the company of the system). For example, the risk formula is taken from the user's company. It would make more sence to take it from the system related to the mgmtsystem.hazard record. |
@dufresnedavid Very true, but this is the lowest priority. |
d61687d
to
a3239c6
Compare
👍 |
"website": "http://www.savoirfairelinux.com", | ||
"license": "AGPL-3", | ||
"category": "Management System", | ||
"description": """\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description must go in README.rst
_name = "mgmtsystem.hazard" | ||
_description = __doc__ | ||
|
||
name = fields.Char('Name', size=50, required=True, translate=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Size shouldn't be specified
just few remarks otherwise 👍 (code review + french translations) |
Multi-system and multi-company support should be mentioned in the Roadmap section, then. |
f85cf28
to
6732070
Compare
|
||
name = fields.Char( | ||
'Control Measure', | ||
size=50, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop size
6732070
to
268c7f5
Compare
Build green, two approvals, remarks fixed - I would merge but it needs a rebase! |
268c7f5
to
b24f6db
Compare
BSCOS-109 Fix filters and search methods
Changed nothing and it seems to work
Depends on: