ported hr_public_holidays to api 8 #85
Conversation
| @api.one | ||
| @api.constrains('year') | ||
| def _check_year(self): | ||
| for data in self: |
There was a problem hiding this comment.
If you use @api.one, you don't need to iterate over self.
grzekru
force-pushed
the
public_holidays_8
branch
from
c25e928
to
23e0411
Compare
|
Hey @grzekru, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
|
Can you use the todo listed here: #86 please ? |
|
|
||
| from openerp import models, fields, api, _ | ||
| from datetime import date | ||
| from openerp.http import request |
There was a problem hiding this comment.
./hr_public_holidays/hr_public_holidays.py:26:1: F401 'request' imported but unused
Removed unused import
| @@ -19,4 +19,4 @@ | |||
| # | |||
| # | |||
|
|
|||
| from . import hr_public_holidays | |||
| import models | |||
There was a problem hiding this comment.
Add relative import:
from . import models
…void code duplication and cleanup code
|
@grzekru I made a PR to your branch that takes into account the remarks of @feketemihai and avoids code duplication. Could you merge it ? Once my PR is merged in this branch, it will be ready to merge in main branch. |
hr_public_holidays: take into account the remarks of the reviewers, avoid code duplication
| ('id', '!=', self.id)]) | ||
| if pholidays: | ||
| raise ValidationError( | ||
| _('Error: Duplicate year %s') % self.year) |
There was a problem hiding this comment.
This validation is not required (from line 50 to 57). It is checked by the unique constraint below.
|
Works OK but there is a glitch in the treeview in the "Related State" column. 👍 |
| class HrPublicHolidays(models.Model): | ||
| _name = 'hr.holidays.public' | ||
| _description = 'Public Holidays' | ||
| _rec_name = 'year' |
There was a problem hiding this comment.
This rec_name is not enough for identifying a record. You should create a name_get method.
There was a problem hiding this comment.
I disagree with that. This module uses the new API and name_get() is sort of "deprecated" with the new API, to be replaced by a display_name as compute field.
In this case, I think _rec_name is enough.
There was a problem hiding this comment.
OK, what I mean is that we need a more complete name, not only the year, but possibly the states between brackets. Instead of using name_get, you can use display_name computed field, but the principle is to complete the shown info
ghost
closed this
|
The pb is that should PR should have been merged long ago. It's an issue with those important PRs that stays under review for too long and never get merged. This code works well, I contributed to it and I use it in production, so I'm not happy that it is abandoned now. |
|
@dufresnedavid, have you started from this branch in your PR or is a totally fresh work? |
|
I would reopen this branch in another PR. The fact is that @grzekru abandonned this PR. @alexis-via are you willing to finish his work ? |
ghost
mentioned this pull request
[BSSFL-194] Stock picking crystal report packing list link
No description provided.