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

[12.0][ADD] hr_holidays_accrual_advanced: Advanced accrual leave allocations #501

Merged
merged 1 commit into from May 14, 2019

Conversation

alexey-pelykh
Copy link
Contributor

@alexey-pelykh alexey-pelykh commented Oct 31, 2018

image
image

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_holidays_accrual branch 12 times, most recently from 0c0f43a to d0d2d4f Compare October 31, 2018 19:48
@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 31, 2018
@pedrobaeza
Copy link
Member

Please add usage and configuration instructions to the README for this being reviewable.

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_holidays_accrual branch 7 times, most recently from ec040b7 to ef77d16 Compare November 1, 2018 07:22
@alexey-pelykh
Copy link
Contributor Author

Depends on #502

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_holidays_accrual branch 2 times, most recently from 493b0f2 to 574f4e1 Compare November 1, 2018 08:41
@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza you've mentioned somewhere (if I remember correctly) that Odoo integrated some of the modules from the OCA, right? If yes, what's the procedure of submission except for the PR submission? Since current accrual leaves implementation in Odoo itself, well, not ok by many reasons, lightly speaking.

@pedrobaeza
Copy link
Member

Yes, you can make a PR over master branch with the added feature to an existing module. New modules will be surely rejected, but new features might be considered if they fit well with the guidelines, cover a general functional gap, and it's well explained.

@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_holidays_accrual branch 3 times, most recently from 89caa0a to 8293267 Compare November 6, 2018 05:32
@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza because changes were requested on the PR:
image
and they were addressed :)

@pedrobaeza
Copy link
Member

Well, then you know more than me...

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza let me rephrase my question: I need one more review, preferably functional one, to get this PR merged?

@nikul-serpentcs
Copy link
Member

@pedrobaeza And One more general Question: In Some repo. not added maintainer group like @OCA/accounting-maintainers

If Added maintainers groups then it's look like freeze or not quick responce

@pedrobaeza
Copy link
Member

@nikul-serpentcs pinging a group makes the message to get to that group, but this doesn't guarantee any reaction, and sometimes the maintainer group is very small (because people haven't applied for this role).

@pedrobaeza
Copy link
Member

@nikul-serpentcs the procedure for being in a team is not that one, as it requires some external steps. See the guide https://odoo-community.org/page/oca-project-steering-committee-guide and apply on contributors@odoo-community.org with your merits.

@nikul-serpentcs
Copy link
Member

nikul-serpentcs commented Mar 26, 2019

@pedrobaeza I think here we create all repo. maintainer group/team (like a HR-maintainer group, Project- maintainer group and so..)
And Active Reviewer Or Person directly add in our group

@nikul-serpentcs
Copy link
Member

because people haven't applied for this role

@pedrobaeza How I can apply for a maintainer?
Many time I sent Join Request here https://github.com/orgs/OCA/teams/website-maintainers/members or https://github.com/orgs/OCA/teams/accounting-maintainers/members
But, Not any replay or avoid my request.

@pedrobaeza
Copy link
Member

I have already told you.

@nikul-serpentcs
Copy link
Member

I have already told you.

Ok, I See

@nikul-serpentcs the procedure for being in a team is not that one, as it requires some external steps. > See the guide https://odoo-community.org/page/oca-project-steering-committee-guide and apply on
contributors@odoo-community.org with your merits.

It's Too long process 😄

Copy link

@sswapnesh sswapnesh left a comment

Choose a reason for hiding this comment

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

LGTM apart from comments I have added (But they are not major ones)

hr_holidays_accrual/__manifest__.py Outdated Show resolved Hide resolved
hr_holidays_accrual/models/hr_leave_allocation.py Outdated Show resolved Hide resolved
hr_holidays_accrual/models/hr_leave_allocation.py Outdated Show resolved Hide resolved
hr_holidays_accrual/views/hr_leave_allocation.xml Outdated Show resolved Hide resolved
@alexey-pelykh alexey-pelykh force-pushed the 12.0-add-hr_holidays_accrual branch 3 times, most recently from 0893a14 to 1669975 Compare March 27, 2019 05:27
@rafaelbn
Copy link
Member

Could we merge this? 3 approvals just missing 1 PSC

@pedrobaeza
Copy link
Member

The only issue I have right now is the module name, as accrual system already exists on core, so this is suggesting the contrary. I know we must avoid the word extension, but right now the only suitable name I find is hr_holidays_accrual_extension. Or any other suggestion?

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza it's so rudiment in core that calling it accrual system would be an overstatement. To very essence, it's not an extension, it's a replacement. Literally, this module does it's own computation, reusing nothing from core "accrual system"

@pedrobaeza
Copy link
Member

Well, I don't like too much that revelation, but OK, let's call it hr_holidays_accrual_alternative, and you must mention this in the README.

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza as is from readme:
This module provides advanced accrual leaves allocation as extension to
out-of-the-box per-employee accrual leave allocation capabilities of Odoo,
introducing following extra features:

  • Accrual allocation history
  • Accrual allocation calculator ("How many leave days I'll have in 3 months from today?")
  • Various accrual methods
  • Various limits to express complex corporate accrual leave policies
  • Takes into account employee service period instead of create_date

Regarding module name, I think hr_holidays_advanced_accruement?

@pedrobaeza
Copy link
Member

pedrobaeza commented Apr 26, 2019

Yeah, but you have just told me that this replaces standard, not extend it, so the README is incorrect, and in any case the name would be hr_holidays_accrual_advanced (not English order, but module + model + feature order).

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza technically, it does replace the calculation (since there was no way to extend it), but it's still backwards compatible, so for user experience it's an advanced extension.

@pedrobaeza
Copy link
Member

I insist, you should mention that "technical" detail, at least in known issues, but go with hr_holidays_accrual_advanced name.

@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza added that to ROADMAP.rst though I'd argue it's has anything to do with roadmap or known issues, and renamed the module

@alexey-pelykh alexey-pelykh changed the title [12.0][ADD] hr_holidays_accrual: Advanced accrual leave allocations [12.0][ADD] hr_holidays_accrual_advanced: Advanced accrual leave allocations Apr 26, 2019
@pedrobaeza pedrobaeza merged commit cc2d7ec into OCA:12.0 May 14, 2019
@sswapnesh
Copy link

Finally 📯 🎆

@alexey-pelykh alexey-pelykh deleted the 12.0-add-hr_holidays_accrual branch May 14, 2019 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants