-
-
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_attendance_rfid #469
Conversation
Can you take the occasion to improve the code coverage for the exception cases? |
fe995e1
to
b332c28
Compare
Sure |
932e480
to
701cda4
Compare
'logged': False, | ||
'action': 'FALSE', | ||
} | ||
employee = self.search([('rfid_card_code', '=', card_code)], limit=1) |
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 think we don't need put limit
in here. Because rfid_card_code
is unique.
@@ -0,0 +1,9 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<odoo> | |||
<data noupdate="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.
if noupdate
is 0, we don't need to keep <data>
tag.
|
||
from odoo.tests.common import TransactionCase | ||
from odoo.tools.misc import mute_logger | ||
from datetime import datetime, timedelta |
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.
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.
Some minor adjustments.
'data': [ | ||
'views/hr_employee_view.xml', | ||
'security/hr_attendance_rfid.xml', | ||
'security/ir.model.access.csv', |
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.
@hveficent security
file always first as par URL https://github.com/OCA/maintainer-tools/blob/master/template/module/__manifest__.py#L28
# License AGPL-3 - See http://www.gnu.org/licenses/agpl-3.0.html | ||
|
||
import logging | ||
from odoo import api, models, fields, _ |
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.
@hveficent IMHO uses an alphabetic naming convention, because better to us.
ex. api, fields, models, _
764721b
to
cf7a0f7
Compare
comments attended @nikul-serpentcs. Thank you for reviewing 😺 |
@hveficent hello! do you want to complete test? |
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
b5bc12f
to
8d33be8
Compare
"""Checkout is created for a future datetime""" | ||
self.env['hr.attendance'].create({ | ||
'employee_id': self.test_employee.id, | ||
'check_in': datetime.today().strftime( |
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.
Use fields.Date.today()
'employee_id': self.test_employee.id, | ||
'check_in': datetime.today().strftime( | ||
DEFAULT_SERVER_DATETIME_FORMAT), | ||
'check_out': (datetime.today()+timedelta(hours=8)).strftime( |
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.
Use fields.Datetime.to_string
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 have some comments, but not related to the migration.
res['action'] = 'check_out' | ||
else: | ||
res['action'] = 'check_in' | ||
return res |
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.
Should be nice to also add the created attendance id to the res. So it can be used e.g. to display it on the device.
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 don't understand created attendance id means, an specific id for the device to know where the attendance came from? On res['employee_name'] you can get that name.
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.
Doesn't matter, It's just an idea. I'll create a PR if I really need it 😉 probably not.
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.
It would be great
c245706
to
cb7f410
Compare
@pedrobaeza, comments attended, could you approve + merge?? Thanks! |
Minor changes