-
-
Notifications
You must be signed in to change notification settings - Fork 664
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
[11.0][MIG] hr_job_category #538
[11.0][MIG] hr_job_category #538
Conversation
Hey @i-vyshnevska, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
Please preserve commit history following technical method explained in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-11.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve Code
9e1becb
to
548697c
Compare
548697c
to
684c115
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hr_job_categories/models/hr.py
Outdated
job = self.env['hr.job'].browse(job_id) | ||
_logger.debug('Removing employee tags if tag exists on contract ' | ||
'job: %s', empl_tags) | ||
for tag in job.category_ids: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring is out of the scope of the PR but pls add a TODO comment to refactor these methods to write only once.
98b4a89
to
b3745cd
Compare
@nikul-serpentcs can you update your review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review LGTM 👍
@i-vyshnevska using oca-gen-addon-readme
and create Readme file
https://github.com/OCA/maintainer-tools/tree/master/template/module/readme
This PR has the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name should be in singular. For example, hr_job_multi_category
, and README should be at least in new format (but README by fragments is recommended).
b3745cd
to
7446041
Compare
7446041
to
e525c0f
Compare
@i-vyshnevska am I wrong or the README is outgenerated but the the readme folder is not here? |
e525c0f
to
08fbebe
Compare
@pedrobaeza sounds good now |
hr_job_category/README.rst
Outdated
|
||
To install this module, you need to: | ||
|
||
* clone the branch 8.0 of the repository https://github.com/OCA/hr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated instructions, but this shouldn't even be there as they are generic instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza I don't see it in the fragment.
Although, there's no reason to explain the cloning etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza @simahawk sorry guys, my bad,
seems ok now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then don't touch yourself the file, but generate it with the OCA/maintainer-tools script.
08fbebe
to
d850e8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change
@@ -0,0 +1,20 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i-vyshnevska Add license/Copyright
@@ -0,0 +1,57 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -0,0 +1,72 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor already asked but after that code LGTM
@@ -0,0 +1,2 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in init file you can remove the fist line
the next time you can use find . -name "*.py" -exec sed -i "/#.*coding\: /d" {} \;
to help you it's on section https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-11.0#tasks-to-do-in-the-migration
hr_job_category/models/hr.py
Outdated
def write(self, vals): | ||
prev_data = self.read(['job_id']) | ||
|
||
res = super(HRContract, self).write(vals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In python3 I think no need HRContract, self
in super but it's works well to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
travis somehow better works with self )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i-vyshnevska this is not something of Travis, but Python version syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza sorry for so much pain from my PR, it seems ok now
7ef16e4
to
0f42c8c
Compare
0f42c8c
to
e361b84
Compare
gentlemen, all good now? |
Hey @i-vyshnevska, Appreciation of efforts, |
Migration of module hr_job_categories to v11