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

FIX: hr_job_category removes employee tags that are not into the contract #1178

Closed
wants to merge 10 commits into from
Closed

FIX: hr_job_category removes employee tags that are not into the contract #1178

wants to merge 10 commits into from

Conversation

leemannd
Copy link
Contributor

@leemannd leemannd commented Dec 22, 2022 •
edited
Loading

The module has been simplified during the migration 12.0 -> 13.0 #895
Since this simplification contained a regression:

  • The employee tags are now stricly binded from the contracts & job positions
  • Updating a contract would flush away all the existing tags

Tags on employee could come from distinct sources (manually added) or linked to other models.
We want to avoid this hard link.

For the moment, I only took back the code from 12.0 (with a small addition) which would require some optimizations.

@leemannd leemannd marked this pull request as draft December 22, 2022 12:08
@leemannd
IMP: add test hr_job_category
135e9d9 
When updating the hr.contract we shouldn't remove
already existing tags
@leemannd leemannd force-pushed the fix_hr_job_category branch 4 times, most recently from c10d670 to 8887e58 Compare December 22, 2022 14:29
@leemannd
FIX hr_job_category
a0f99cb 
When you update the contract, you dont remove tags that are manually added
@leemannd leemannd force-pushed the fix_hr_job_category branch from 8887e58 to a0f99cb Compare December 22, 2022 14:35
@leemannd leemannd marked this pull request as ready for review January 9, 2023 10:24
santostelmo
santostelmo reviewed Jan 9, 2023
View reviewed changes
hr_job_category/models/hr.py Outdated
def _remove_tags(self, employee_id=None, job_id=None):
# TODO write tags only once
if not employee_id or not job_id:
return
Copy link

Choose a reason for hiding this comment

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

Easy to test that condition for better test coverage. Or maybe this method does not need to return a value

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 14, 2023
@leemannd
Copy link
Contributor Author

Hello @jarroyomorales You have been doing a migration of this module with a simplification of the code. But it has been introducing a regression (listed in the PR description).
cc @etobella if you want to have a look at it as you reviewed the previous PR
cc @Saran440 as you merged the migration PR

@Saran440 Saran440 added needs review and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels May 18, 2023
@leemannd leemannd mentioned this pull request Aug 30, 2023
Closed
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Sep 17, 2023
florentx
florentx suggested changes Oct 16, 2023
View reviewed changes
Copy link

@florentx florentx left a comment

Choose a reason for hiding this comment

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

Need to remove the stale label on this PR.

And refactor proposed to simplify and have less writes.

hr_job_category/models/hr.py Outdated Show resolved Hide resolved
hr_job_category/models/hr.py Outdated Show resolved Hide resolved
hr_job_category/models/hr.py Outdated Show resolved Hide resolved
hr_job_category/models/hr.py Outdated Show resolved Hide resolved
hr_job_category/models/hr.py Outdated Show resolved Hide resolved
hr_job_category/models/hr.py Outdated Show resolved Hide resolved
@etobella etobella removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 16, 2023
@github-actions GitHub Actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Feb 18, 2024
@github-actions github-actions bot closed this Mar 24, 2024
@etobella etobella mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florentx florentx florentx requested changes

@santostelmo santostelmo santostelmo approved these changes

Assignees
No one assigned
Labels
needs review stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leemannd @florentx @santostelmo @Saran440 @etobella