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

[10.0] Medical practitioner #180

Merged
merged 2 commits into from
Jul 7, 2017
Merged

[10.0] Medical practitioner #180

merged 2 commits into from
Jul 7, 2017

Conversation

rgarnau
Copy link

@rgarnau rgarnau commented Jun 26, 2017

This module adds medical practitioners.

Practitioner covers all individuals who are engaged in the healthcare process
and healthcare-related services as part of their formal responsibilities.

@lasley @jbeficent @etobella @gmeficent

@rgarnau rgarnau mentioned this pull request Jun 26, 2017
Closed
1 task
JordiBForgeFlow
JordiBForgeFlow reviewed Jun 26, 2017
View reviewed changes
medical_practitioner/models/medical_role.py
from odoo import models, fields


class MedicalRole(models.Model):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add active = fields.Boolean('Active', default=True) and show this field in the form view?

That way users can deactivate roles that have become obsolete.

Copy link
Author

Choose a reason for hiding this comment

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

Done 👍

lasley
lasley suggested changes Jun 26, 2017
View reviewed changes
Copy link

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @rgarnau

Shouldn't this PR also deprecate medical.physician?

medical_practitioner/models/medical_role.py Outdated
)

active = fields.Boolean(
'Active',
Copy link

Choose a reason for hiding this comment

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

This string is superfluous, Active is the default

medical_practitioner/README.rst Outdated
@@ -0,0 +1,91 @@
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
:target: http://www.gnu.org/licenses/agpl-3.0-standalone.html
:alt: License: AGPL-3
Copy link

Choose a reason for hiding this comment

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

LGPL

medical_practitioner/README.rst Outdated
@@ -0,0 +1,91 @@
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
:target: http://www.gnu.org/licenses/agpl-3.0-standalone.html
Copy link

Choose a reason for hiding this comment

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

lgpl

medical_practitioner/README.rst Outdated
@@ -0,0 +1,91 @@
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
Copy link

Choose a reason for hiding this comment

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

LGPL

medical_practitioner/__manifest__.py Outdated
'version': '10.0.1.0.0',
'summary': 'Defines medical practioners',
'author': 'Creu Blanca',
'maintainer': 'Eficent, Creu Blanca, Odoo Community Association (OCA)',
Copy link

Choose a reason for hiding this comment

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

This should be the author key. maintainer key is a thing, but not used in OCA AFAIK

Also - I see a lot of LasLabs copyrights and credits left here. I don't recognize any of this code, however. Did LasLabs actually have a hand in any of this? If so, we should share author credit as well. Otherwise, please remove our credits.

medical_practitioner/models/medical_role.py Outdated
_description = 'Practitioner Roles'

name = fields.Char(
required=True
Copy link

Choose a reason for hiding this comment

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

trailing comma

medical_practitioner/models/medical_role.py Outdated
)

description = fields.Char(
required=True
Copy link

Choose a reason for hiding this comment

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

trailing comma

medical_practitioner/models/medical_role.py Outdated

active = fields.Boolean(
'Active',
default=True
Copy link

Choose a reason for hiding this comment

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

trailing comma

medical_practitioner/views/medical_menu.xml Outdated
-->

<odoo>
<data>
Copy link

Choose a reason for hiding this comment

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

can remove this

medical_practitioner/views/medical_menu.xml Outdated
<odoo>
<data>

<menuitem id="medical_conf_practitioner_menu"
Copy link

Choose a reason for hiding this comment

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

Is there a reason we are creating a top level menu for this? Why not use the pre-existing "Medical Professionals" top level menu that Physicians is currently using?

Copy link
Author

Choose a reason for hiding this comment

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

As this module deprecates Physician, we are replacing this menu https://github.com/LasLabs/vertical-medical/blob/feature/10.0/medical_physician/medical_physician/views/medical_menu.xml#L20 for that new one.

Maybe I don't see your point, but IMO a new menu is necessary.

All the other changes are already added. Thanks!

@lasley lasley added this to the 10.0 milestone Jun 26, 2017
MiquelRForgeFlow
MiquelRForgeFlow reviewed Jul 4, 2017
View reviewed changes
medical_practitioner/models/medical_practitioner.py Outdated
# Copyright 2017 Eficent Business and IT Consulting Services, S.L.
# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html).

from odoo import models, fields
Copy link

Choose a reason for hiding this comment

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

alphabetical order please

@rgarnau
Copy link
Author

rgarnau commented Jul 4, 2017

@lasley @mreficent - all changes have been applied. Could you please update?

MiquelRForgeFlow
MiquelRForgeFlow approved these changes Jul 4, 2017
View reviewed changes
Copy link

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

code review 👍

@lasley
Copy link

lasley commented Jul 5, 2017

@rgarnau - isn't this module intended to deprecate medical_physician? If that is the case, we will want to perform that deprecation in this PR IMO

@rgarnau
Copy link
Author

rgarnau commented Jul 5, 2017

@lasley - this module does deprecate medical_physician, but what do you mean by 'perform that deprecation'?

@lasley
Copy link

lasley commented Jul 5, 2017

@rgarnau - We would want to delete the module that is no longer valid. So basically just delete medical_physician in a commit indicating such.

Also because we're modifying two isolated modules here, I will not be able to squash on merge. Please also amend f3e2a6b to note the proper module name (it currently says Medical Center), and squash your two 60c8476 b4b260a into each other.

@rgarnau
Copy link
Author

rgarnau commented Jul 6, 2017

@lasley - Just changed the module name in the commit and squashed the two other.
Referring to the deprecation, do you want me to delete Physician from your repo?

@lasley
Copy link

lasley commented Jul 6, 2017

Looks great, thanks @rgarnau!

Referring to the deprecation, do you want me to delete Physician from your repo?

If you're referring to the LasLabs fork, don't worry about it. We're actually going to have to make an adaptation layer there so that customers currently using it have a clear migration path.

Let's do delete it from the OCA repo in this PR though. There's no reason to keep deprecated code around IMO.

JordiBForgeFlow
JordiBForgeFlow approved these changes Jul 7, 2017
View reviewed changes
Copy link
Member

@JordiBForgeFlow JordiBForgeFlow left a comment

Choose a reason for hiding this comment

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

👍 Technical and functional review.

@JordiBForgeFlow
Copy link
Member

@lasley medical.physician is not currently present in any installable module of this repo in branch 10.0. That's why @rgarnau was a bit confused about your request. Since we have all reviewers needed, code coverage is 100% and runbot is green I am merging.

@JordiBForgeFlow JordiBForgeFlow merged commit c1a96ef into OCA:10.0 Jul 7, 2017
@lasley
Copy link

lasley commented Jul 7, 2017

Oops, good call! Sorry - so much code 😆 I'm having trouble keeping it all straight. Glad we're finally getting all of this out of incubator

@obulkin obulkin mentioned this pull request Jul 27, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lasley lasley lasley requested changes

@MiquelRForgeFlow MiquelRForgeFlow MiquelRForgeFlow approved these changes

@JordiBForgeFlow JordiBForgeFlow JordiBForgeFlow approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rgarnau @lasley @JordiBForgeFlow @MiquelRForgeFlow @etobella