[11.0][MIG] hr_timesheet_sheet #125

MiquelRForgeFlow · 2018-05-08T17:23:59Z

Migrated from Odoo, 10.0 version.

Attend reviews from people
Add compatibility with multicompany
Add tests
Pending to add migration scripts

pedrobaeza · 2018-05-08T18:14:16Z

Hey, you did it finally using 2D matrix! How do you perform the addition of a new row?

JordiBForgeFlow · 2018-05-08T18:30:14Z

black magic..... ;)

pedrobaeza · 2018-05-08T18:34:19Z

hehe, I will check by curiosity when you tell me.

pedrobaeza · 2018-05-08T18:34:43Z

My idea was more to add the feature on 2D matrix option instead any way.

MiquelRForgeFlow · 2018-05-09T11:27:54Z

PR is green 🍏

Reviews are welcome!

@pedrobaeza right, that would be a nice feature, but I don't know much about javascript :S

pedrobaeza

Tried functionally, and it's working great. The only problem I have is with the tab key for moving inside the matrix, that is moving away from the full matrix instead of going to the next field inside the matrix, but I think this is something of the 2D matrix instead of this module. @lreficent and @simahawk I think this more for you.

pedrobaeza · 2018-05-10T19:22:40Z

Ah, and another flaw is that you can't remove a line once it's added, but well, it's not critical.

JordiBForgeFlow · 2018-05-10T21:37:24Z

@pedrobaeza I discussed the option to remove a line with @mreficent. Seems to me that the user should be able to select the project or project/task from the selectio and press a button 'Remove Line'. Although is not very intuitive. Perhaps another interesting option would be to allow the widget to handle a remove icon, but not sure.

But for sure that is not critical.

simahawk · 2018-05-11T06:47:51Z

@pedrobaeza @lreficent @jbeficent being able to navigate easily across cells is a TODO item (in code comments). Tab indexes are set on each cell but they are ignored. Shouldn't be that hard to solve.

simahawk

also, AFAIS there are no tests: would you mind adding some or - at least - drop a line in the roadmap section?

simahawk · 2018-05-11T06:52:26Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+    employee_id = fields.Many2one(
+        comodel_name='hr.employee',
+        string='Employee',
+        default=_default_employee,


proxy default methods w/ lambda default=lambda self: self._default_foo()

simahawk · 2018-05-11T06:55:11Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+                return
+            dates = sheet._get_dates()
+            if not dates:
+                return


shouldn't be continue?

simahawk · 2018-05-11T06:57:47Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+                ('sheet_id', 'in', [sheet and sheet.id or False, False]),
+            ])
+            lines = self.env['hr_timesheet.sheet.line']
+            for date in dates:


here we have N nested loops: any chance to improve it? If not possible now you can add a comment to not forget about it.

simahawk · 2018-05-11T06:58:30Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+                raise UserError(
+                    _('In order to create a sheet for this employee, '
+                      'you must link him/her to an user.'))
+        res = super(Sheet, self).create(vals)


you can use empty super()

Yes, but I prefer filled supers 😋

simahawk · 2018-05-11T06:59:03Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+
+    @api.multi
+    def write(self, vals):
+        if 'employee_id' in vals:


this snippet is duplicated. What about moving it to _check_employee_user method?

the methods with @api.contrains are executed at the last step of the write but here it seems that interested them to be executed before the write.

simahawk · 2018-05-11T07:01:37Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+
+    def clean_timesheets(self, timesheet):
+        if self.id:
+            for anal_line in timesheet.filtered(lambda a: not a.sheet_id):


🔥 can we change the spelling here? 😄

@lreficent 😂

here nobody was burned 😜

simahawk · 2018-05-11T07:05:08Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+    )
+
+    @api.onchange('unit_amount')
+    def onchange_unit_amount(self):


aren't we doing too many things here?
In any case, please, add a docstring saying what we do.

Seems "too many things" but are "just a few". It's just that are many conditions. I will add docstring explaining it well.

Added docstring. I will be sure to fully add full tests for this method.

Method fully tested.

simahawk · 2018-05-11T07:07:16Z

hr_timesheet_sheet/models/account_analytic_line.py

+class AccountAnalyticLine(models.Model):
+    _inherit = 'account.analytic.line'
+
+    sheet_id_computed = fields.Many2one(


computed_sheet_id? Also, would be nice to have a comment explaining the purpose of this field (compared w/ the next one)

I think it's better as is, for having the same name beginning

This is the same that v10. Here I didn't do any change.

astirpe · 2018-05-11T09:45:39Z

Thank you for this porting!

Are field 'account_ids' and model 'hr_timesheet.sheet.account' going to be removed? Or are they still present for some reason?

MiquelRForgeFlow · 2018-05-11T09:52:44Z

@pedrobaeza Removing matrix lines was already not possible in v10. But, you can always delete timesheet lines from the "Details" tab.

@simahawk Yes, my intention is to add tests but I wanted a preliminary review from all of you. It's easy to do changes if there aren't tests than doing changes when there are tests.

@astirpe They are present because @jbeficent wanted them to be present. He wanted me to do the least changes possibles from v10.

pedrobaeza · 2018-05-11T09:57:11Z

If they are useless, you can remove them in an extra commit, so we can review isolately each one.

yung-wang · 2018-05-11T11:57:13Z

Looking good. Can you also sum up the total of the period in the 'overview' tab? The lines and the days has a total. It would be nice to have a grand total also. This already on the 'details' tab.
I will test some more the coming next weeks.
Great work so far!

MiquelRForgeFlow · 2018-05-11T12:10:07Z

@yung-wang but that's something of the matrix widget and this PR can't modify the widget. May @simahawk or @grindtildeath want to improve the widget in the web repository.

yung-wang · 2018-05-11T12:12:55Z

@mreficent ah ok, it is also fine if that can be improved in the widget. thanks.

astirpe · 2018-05-11T12:38:44Z

hr_timesheet_sheet/__manifest__.py

+    'author': 'Eficent, Odoo Community Association (OCA)',
+    'website': 'https://github.com/OCA/hr-timesheet',
+    'depends': [
+        'hr_timesheet', 'account',


Why is there a dependency on 'account' here?

because v10 hr_timesheet has a dependency on account and v11 hr_timesheet doesn't have it, and I thought that the previous dependencies of hr_timesheet_sheet (although indirectly) should remain.

Do you think I should remove it?

I think so. I don't think you made use of any particular field introduced by 'account'. It should be fine to remove it, please do it and I will try with some extra tests ;)

astirpe · 2018-05-11T12:43:50Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+                ('date', '>=', sheet.date_start),
+                ('employee_id', '=', sheet.employee_id.id),
+                ('sheet_id', 'in', [sheet and sheet.id or False, False]),
+            ])


In a multicompany environment I guess we also need to search per company:
('company_id', '=', sheet.company_id.id)

Yeah, as it is now, I haven't taken into account the multicompany yet.

astirpe · 2018-05-11T12:45:45Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+        default='new', track_visibility='onchange',
+        string='Status', required=True, readonly=True, index=True,
+    )
+    account_ids = fields.One2many(


Please remove field account_ids and all the code regarding the hr_timesheet.sheet.account model, since it's not used anymore

astirpe · 2018-05-11T12:48:20Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+        if r == WEEKLY:
+            return today - relativedelta(days=today.weekday())
+        elif r == MONTHLY:
+            return fields.Date.date(today.year, today.month, 1)


Monthly timesheet doesn't seem to work properly, could you have a try?

I got this:

Error: Odoo Server Error Traceback (most recent call last): File "/.repo_requirements/odoo/odoo/http.py", line 650, in _handle_exception return super(JsonRequest, self)._handle_exception(exception) File "/.repo_requirements/odoo/odoo/http.py", line 310, in _handle_exception raise pycompat.reraise(type(exception), exception, sys.exc_info()[2]) File "/.repo_requirements/odoo/odoo/tools/pycompat.py", line 87, in reraise raise value File "/.repo_requirements/odoo/odoo/http.py", line 692, in dispatch result = self._call_function(**self.params) File "/.repo_requirements/odoo/odoo/http.py", line 342, in _call_function return checked_call(self.db, *args, **kwargs) File "/.repo_requirements/odoo/odoo/service/model.py", line 97, in wrapper return f(dbname, *args, **kwargs) File "/.repo_requirements/odoo/odoo/http.py", line 335, in checked_call result = self.endpoint(*a, **kw) File "/.repo_requirements/odoo/odoo/http.py", line 936, in __call__ return self.method(*args, **kw) File "/.repo_requirements/odoo/odoo/http.py", line 515, in response_wrap response = f(*args, **kw) File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 931, in call_kw return self._call_kw(model, method, args, kwargs) File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 923, in _call_kw return call_kw(request.env[model], method, args, kwargs) File "/.repo_requirements/odoo/odoo/api.py", line 687, in call_kw return call_kw_model(method, model, args, kwargs) File "/.repo_requirements/odoo/odoo/api.py", line 672, in call_kw_model result = method(recs, *args, **kwargs) File "/.repo_requirements/odoo/odoo/models.py", line 1078, in default_get defaults[name] = field.default(self) File "/home/odoo/build/OCA/hr-timesheet/hr_timesheet_sheet/models/hr_timesheet_sheet.py", line 28, in _default_date_start return fields.Date.date(today.year, today.month, 1) AttributeError: type object 'Date' has no attribute 'date'

astirpe · 2018-05-11T12:51:47Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+                },
+            }
+
+    def copy(self, *args, **argv):


The signature of this method is a bit "old fashioned"

copypasted from v10

astirpe · 2018-05-11T12:54:27Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+        comodel_name='hr.department',
+        string='Department'
+    )
+    project_ids = fields.Many2many(


Why do you need project_ids ?

astirpe · 2018-05-11T13:02:22Z

hr_timesheet_sheet/models/hr_timesheet_sheet.py

+            'new': [('readonly', False)]},
+    )
+    state = fields.Selection([
+        ('new', 'New'),


Consider to remove the item 'new' of the state field.
Actually the timesheet sheet is created with state field as 'new' by default, but it automatically goes to 'draft' while created (see def create()). Then 'new' is never used again ... so I would say that state 'new' is completely useless.

the difference is that in new, you select the dates. In draft you can't change the dates.

Ok, then for me it's better to keep this part as it is. Thank you for checking!

Well, I finally deleted this state.

astirpe · 2018-05-11T13:34:04Z

@jbeficent about removing a line (row of the matrix), it could be done automatically in case you have a line containing only zeros. When you save the timesheet sheet, such line can be simply removed by the orm: no extra buttons needed. But as you said it is just a nice to have, not really needed at this moment.

MiquelRForgeFlow · 2018-05-11T13:56:05Z

@astirpe as it is now, the module already removes rows that only contains zeros in delete_empty_lines method

astirpe · 2018-05-15T06:32:03Z

In Runbot i got this:

Error:
Odoo Server Error

Traceback (most recent call last):
  File "/.repo_requirements/odoo/odoo/http.py", line 650, in _handle_exception
    return super(JsonRequest, self)._handle_exception(exception)
  File "/.repo_requirements/odoo/odoo/http.py", line 310, in _handle_exception
    raise pycompat.reraise(type(exception), exception, sys.exc_info()[2])
  File "/.repo_requirements/odoo/odoo/tools/pycompat.py", line 87, in reraise
    raise value
  File "/.repo_requirements/odoo/odoo/http.py", line 692, in dispatch
    result = self._call_function(**self.params)
  File "/.repo_requirements/odoo/odoo/http.py", line 342, in _call_function
    return checked_call(self.db, *args, **kwargs)
  File "/.repo_requirements/odoo/odoo/service/model.py", line 97, in wrapper
    return f(dbname, *args, **kwargs)
  File "/.repo_requirements/odoo/odoo/http.py", line 335, in checked_call
    result = self.endpoint(*a, **kw)
  File "/.repo_requirements/odoo/odoo/http.py", line 936, in __call__
    return self.method(*args, **kw)
  File "/.repo_requirements/odoo/odoo/http.py", line 515, in response_wrap
    response = f(*args, **kw)
  File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 931, in call_kw
    return self._call_kw(model, method, args, kwargs)
  File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 923, in _call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/.repo_requirements/odoo/odoo/api.py", line 687, in call_kw
    return call_kw_model(method, model, args, kwargs)
  File "/.repo_requirements/odoo/odoo/api.py", line 672, in call_kw_model
    result = method(recs, *args, **kwargs)
  File "/.repo_requirements/odoo/odoo/models.py", line 1087, in default_get
    defaults = self._convert_to_write(defaults)
  File "/.repo_requirements/odoo/odoo/models.py", line 4471, in _convert_to_write
    value = field.convert_to_cache(value, self, validate=False)
  File "/.repo_requirements/odoo/odoo/fields.py", line 1542, in convert_to_cache
    return self.to_string(value)
  File "/.repo_requirements/odoo/odoo/fields.py", line 1529, in to_string
    return value.strftime(DATE_FORMAT) if value else False
AttributeError: 'function' object has no attribute 'strftime'

MiquelRForgeFlow · 2018-05-15T10:42:52Z

@astirpe what did you do to get this traceback?

astirpe · 2018-05-15T11:33:22Z

@mreficent this morning I just opened the Runbot instance of this PR and I clicked on "Timesheet Sheet"

* [10.0] hr_timesheet_sheet * [11.0][MIG] hr_timesheet_sheet * [REMOVE] hr_timesheet.sheet.account * [REMOVE] 'new' state * [ADD] Tests * [UPD] Adapt to multicompany * [ADD] Add more tests (include multicompany tests) * [FIX] project_task_stage_allow_timesheet: show error message only if task * [ADD] Migration scripts to v11

[10.0] hr_timesheet_sheet

fd42765

MiquelRForgeFlow force-pushed the 11.0-mig-hr_timesheet_sheet branch from 10e6fc2 to 6a0a064 Compare May 8, 2018 17:40

JordiBForgeFlow added needs review work in progress labels May 9, 2018

JordiBForgeFlow added this to the 11.0 milestone May 9, 2018

MiquelRForgeFlow force-pushed the 11.0-mig-hr_timesheet_sheet branch 2 times, most recently from 65ec009 to 319b8cf Compare May 9, 2018 11:00

pedrobaeza reviewed May 10, 2018

View reviewed changes

simahawk requested changes May 11, 2018

View reviewed changes

astirpe suggested changes May 11, 2018

View reviewed changes

MiquelRForgeFlow force-pushed the 11.0-mig-hr_timesheet_sheet branch from b6d68d9 to 9a3a2ac Compare May 11, 2018 16:07

dreispt mentioned this pull request Nov 26, 2023

[MIG] project_task_stage_allow_timesheet: Migration to 16.0 #621

Merged

dreispt mentioned this pull request Jan 21, 2024

[17.0][MIG] hr_timesheet_sheet: Migration to 17.0 #629

Closed

[11.0][MIG] hr_timesheet_sheet #125

[11.0][MIG] hr_timesheet_sheet #125

Conversation

MiquelRForgeFlow commented May 8, 2018 • edited Loading

pedrobaeza commented May 8, 2018

JordiBForgeFlow commented May 8, 2018

pedrobaeza commented May 8, 2018

pedrobaeza commented May 8, 2018

MiquelRForgeFlow commented May 9, 2018

pedrobaeza left a comment

Choose a reason for hiding this comment

pedrobaeza commented May 10, 2018

JordiBForgeFlow commented May 10, 2018

simahawk commented May 11, 2018

simahawk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astirpe commented May 11, 2018 • edited Loading

MiquelRForgeFlow commented May 11, 2018 • edited Loading

pedrobaeza commented May 11, 2018

yung-wang commented May 11, 2018

MiquelRForgeFlow commented May 11, 2018

yung-wang commented May 11, 2018

Choose a reason for hiding this comment

MiquelRForgeFlow May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astirpe commented May 11, 2018

MiquelRForgeFlow commented May 11, 2018

astirpe commented May 15, 2018

MiquelRForgeFlow commented May 15, 2018 • edited Loading

astirpe commented May 15, 2018

MiquelRForgeFlow commented May 8, 2018 •

edited

Loading

astirpe commented May 11, 2018 •

edited

Loading

MiquelRForgeFlow commented May 11, 2018 •

edited

Loading

MiquelRForgeFlow May 11, 2018 •

edited

Loading

MiquelRForgeFlow commented May 15, 2018 •

edited

Loading