-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
[12.0][MIG] crm_phonecall_timesheet #261
[12.0][MIG] crm_phonecall_timesheet #261
Conversation
@Tardo please make sure rebase all your commits to one. |
@rafaelbn @victormmtorres can you check? |
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 get exactly what you have made here. Phone calls timesheets should be independent from leads and their timesheets, so all the stuff for converting things seems not needed.
a6b18c0
to
71b8ed3
Compare
Ok, reverted changes and now follow the original module flow |
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.
Only functional test in runbor 👍
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 remarks
@@ -63,8 +70,7 @@ | |||
<notebook> | |||
<page string="Timesheet"> | |||
<field name="timesheet_ids" | |||
context="{'default_account_id': analytic_account_id, | |||
'default_user_id': uid}" /> |
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.
To keep the original behavior and inheritance:
<field name="timesheet_ids" context="{'tree_view_ref': 'analytic.view_account_analytic_line_tree','default_account_id': analytic_account_id, 'default_user_id': uid}"/>
self.assertEqual(phonecall.date[:DATE_LENGTH], timesheet.date) | ||
self.assertEqual( | ||
phonecall.date.strftime(DEFAULT_SERVER_DATE_FORMAT), | ||
timesheet.date.strftime(DEFAULT_SERVER_DATE_FORMAT)) |
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 odoo.date_utils
, it's more fun :)
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.
date_utils? i will use date objects self.assertEqual(phonecall.date.date(), timesheet.date)
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.
Sure, they're datetime objects now. Anyway, is it redundant the date()
method?
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.
phonecall it's an datetime, timesheet it's an 'date'... .date() convert datetime to date.
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.
Sure! :)
AccountAnalitycLineObj = self.env['account.analytic.line'] | ||
for record in self: | ||
timesheet = AccountAnalitycLineObj.search([ | ||
('phonecall_id', '=', record.id)]) |
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.
You dropped this domain filter from the original: ('code', '=', 'phone')
crm_phonecall_timesheet/README.rst
Outdated
timer when call ends, this will set exactly the time you spent. | ||
1. Create a phonecall in tree or directly an Opportunity | ||
2. If you create a phonecall, convert it to opportunity | ||
3. Set the project (with an analytic account associated) |
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.
Change this to fit the real flow
71b8ed3
to
134f30d
Compare
Changes done, except 'date_utils' usage... thanks. |
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.
Great! 👍
134f30d
to
1cf99c9
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.
You are still showing the analytic account instead of the project_id. You must put in views project_id
field and fill that one and the corresponding analytic account from project in the analytic account.
Then the original module was wrong all along, isn't it? |
No, it's just that the approach changed on v11: in v10, there were |
Currently translated at 100.0% (10 of 10 strings) Translation: hr-timesheet-10.0/hr-timesheet-10.0-crm_phonecall_timesheet Translate-URL: https://translation.odoo-community.org/projects/hr-timesheet-10-0/hr-timesheet-10-0-crm_phonecall_timesheet/it/
1cf99c9
to
5e793b8
Compare
Changes done |
5e793b8
to
6dbf61d
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.
Tested on runbot after the last changes 👍
Just some comments
# Copyright 2015 Antonio Espinosa <antonio.espinosa@tecnativa.com> | ||
# Copyright 2015 Javier Iniesta <javieria@antiun.com> | ||
# Copyright 2017 David Vidal <david.vidal@tecnativa.com> | ||
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). | ||
{ | ||
'name': "CRM Timesheet", | ||
'category': 'Customer Relationship Management', | ||
'version': '10.0.1.0.0', | ||
'version': '12.0.1.0.0', | ||
'depends': [ | ||
'crm_timesheet', |
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.
Would it be enough to depend on hr_timesheet
options="{'no_open': True, 'no_create': True}"/> | ||
</field> | ||
<field name="description" position="after"> | ||
<notebook> | ||
<page string="Timesheet"> | ||
<field name="timesheet_ids" | ||
context="{'default_account_id': analytic_account_id, | ||
'default_user_id': uid}" /> | ||
context="{'tree_view_ref': 'analytic.view_account_analytic_line_tree','default_project_id': project_id, 'default_user_id': uid}"/> |
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.
With the last changes, this would be the proper one: hr_timesheet.hr_timesheet_line_tree
6dbf61d
to
ce87f82
Compare
@chienandalu changes applied, thanks. |
Great! |
Some changes:
cc @Tecnativa