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

[WIP][8.0][hr_infraction] Migrate + Refactoring #207

Closed
wants to merge 21 commits into from

Conversation

andhit-r
Copy link
Member

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

PR still need few finishing. But reviews are most welcome.

Refactoring:

  1. Remove dependensi to hr_employee_state, hr_security, hr_transfer
  2. Remove workflow
  3. Remove infraction action

Squash will be perform after this PR get all the review needed.

@feketemihai
Copy link
Member

Hello @andhit-r , thanks for the work, but it's quite hard to review the module, first of all try to move the files with "git mv ...", it's more easy because it's keeping the history of the file, and we can have a comparition of what was changed.

I don't know how many people are using this module in V7 or older, and the one's that are using are used with certain feature of the module (like finish an infraction with an action, beside the warning). But i think the easier way is to merge those 2 tables into one, into the action one, because an action can also be a warning like a Oral Warning, Written Warning, Final Written Warning and after to have a Pre-termination Hearing and a Termination Hearing, but this is more depending of the company disciplinary policy.

Before review, i would like to know others opinion, should we have a module as a dependant managing this kind of policy?? @OCA/human-resources-maintainers

@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. I'm pretty sure i used git mv and git rm to migrate. andhit-r@c5c5f53 for example. Please let me know if i miss something.

I remove action primarily because it's related to modules that have not migrated yet and one of them has a small chance to be ported (hr_simplify #97). Plus by removing those dependency i want to keep modularity of this module.

I like your idea to combine warning and action though. Will change it ASAP.

# -*- coding: utf-8 -*-
# © 2013 Michael Telahun Makonnen <mmakonnen@gmail.com>
# © 2016 OpenSynergy Indonesia
# @author Andhitia Rama
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the author here, you already are in the contribuitor tab of the module.

@@ -0,0 +1,169 @@
# -*- coding: utf-8 -*-
# © <YEAR(S)> <AUTHOR(S)>
Copy link
Member

Choose a reason for hiding this comment

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

Addapt copyright year and author

@andhit-r
Copy link
Member Author

@feketemihai that one i missed and stil unrevised, sorry. Will be revised on my next push. Thanks

@max3903 max3903 mentioned this pull request Apr 8, 2016
58 tasks

from . import hr_infraction
from . import wizard
from . import models, tests
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to import tests...

@saltonmassally
Copy link

@andhit-r I think we should bring back hr.infraction.action. I have already ported this addon along with the dependencies just been too busy to do a PR yet. Maybe we can work on this together

@andhit-r
Copy link
Member Author

andhit-r commented Apr 8, 2016

@feketemihai @tarzan0820 thanks for your reviews. @tarzan0820 it will be great to work this module together. If you can create a PR from your work we can start there.

@saltonmassally
Copy link

@andhit-r i will do so tonight

========================

This module add employee infraction management

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 explain a bit more the concept of "infraction management" and the scope of the module here? An "executive summary" would be nice to help people decide if the module applies to their need.

@feketemihai
Copy link
Member

Please reopen the PR if it's important to you.

sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018
[BSSFL-320][Fix] Remove sf_sale_invoice_fiscal_position from migration.yml
Mraimou pushed a commit to camptocamp/hr that referenced this pull request Nov 25, 2019
add default exchange backend and assign it to every user
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants