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

[hr_emergency_contact] Migrate to 8.0 + refactoring #204

Merged
merged 8 commits into from
Mar 22, 2016

Conversation

andhit-r
Copy link
Member

@andhit-r andhit-r commented Mar 6, 2016

See readme

@feketemihai
Copy link
Member

Hi @andhit-r , thanks for your work, can you check the copyright section of the files, it's quite messy and you should leave © 2011 Michael Telahun Makonnen, since he is the one that proposed the module to OCA, even if you refactor a little bit( rename of file), i think we should keep that, plus an author line in the copyright doesn't make sense...i think we should just stick to the author tab in the manifest file.

Plus can you remove the unused files from the static folder.

@feketemihai feketemihai added this to the 8.0 milestone Mar 9, 2016
@andhit-r
Copy link
Member Author

andhit-r commented Mar 9, 2016

@feketemihai thanks for your review. Would you show me which copyright? I am sure i have cleaned copyright file into latest OCA format and retain @miketelahun copyright. Maybe i miss something

Only one file that does not contain @miketelahun copyright:

That is because that file does not exist on previous version. But if @miketelahun copyright should be stated on that file i will add it. No problem.

@andhit-r
Copy link
Member Author

andhit-r commented Mar 9, 2016

@feketemihai i pushed PR contain revision based on your comments. You you kindly review it again. Many thanks.


class HrEmployee(models.Model):

_name = 'hr.employee'
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 remove the _name line...by inheriting you don't need it

@andhit-r
Copy link
Member Author

andhit-r commented Mar 9, 2016

@feketemihai done.

@feketemihai
Copy link
Member

👍

@andhit-r
Copy link
Member Author

andhit-r commented Mar 9, 2016

@feketemihai thanks for all your reviews.

@azmimr67
Copy link

azmimr67 commented Mar 9, 2016

Well tested 👍

@andhit-r
Copy link
Member Author

@feketemihai would you add needs review label to this PR. Thanks

@andhit-r
Copy link
Member Author

@OCA/human-resources-maintainers would you kindly review this PR. Thanks in advance.

@mikevhe18
Copy link

👍

@andhit-r
Copy link
Member Author

@OCA/human-resources-maintainers i think this PR is ready to be merged (3 reviews)

@@ -0,0 +1,79 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
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 include the SVG on the unaltered icon

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 86.199% when pulling 45be4e5 on andhit-r:8.0-hr_emergency_contact into 90c4d1f on OCA:8.0.

@andhit-r
Copy link
Member Author

@pedrobaeza thank you for your review. I pushed revision according your latest input.

@pedrobaeza
Copy link
Member

👍 Merging

pedrobaeza added a commit that referenced this pull request Mar 22, 2016
[hr_emergency_contact] Migrate to 8.0 + refactoring
@pedrobaeza pedrobaeza merged commit e6f1a4e into OCA:8.0 Mar 22, 2016
@andhit-r
Copy link
Member Author

Thanks @pedrobaeza. Thanks all reviewers.

@max3903 max3903 mentioned this pull request Apr 8, 2016
58 tasks
sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018
Speed up data loading by skiping creating of mail messages
Mraimou pushed a commit to camptocamp/hr that referenced this pull request Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants