-
-
Notifications
You must be signed in to change notification settings - Fork 351
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
[17.0][MIG] crm_timesheet: Migration to 17.0 #700
[17.0][MIG] crm_timesheet: Migration to 17.0 #700
Conversation
This addon assumed one will always fill the analytic account before the project, but current behavior does not let you choose a project outside current analytic account, which in practice means you can never change the project if `analytic_account_id` field is hidden (as it happens in other addons such as `project_issue_timesheet_time_control`). Now, if you choose a project it will fill the analytic account, and also the other way around, so why bother about the domains?
As now hr_timesheet bases all their functionality on projects, there's no sense to keep the old behavior linking crm.lead times to analytic account instead of projects, so I have changed this, making easier the associated logic (which, in the other hand, was not fully correct previously).
* Limit timesheet field addition in view to authorized users * Use proper view in lead embedded timesheet page * Don't show lead field in embedded timesheet page
Following the changes from OCA/project#586, here I implement the same UX when using it with CRM leads.
Imagine this scenario: 1. In tab 1 of the browser, you have opened lead 1. 2. In tab 2 of the browser, you have opened lead 2. 3. You go to tab 1 and start a timer. 4. Work, work, work... 5. You go to tab 2 and start a timer, stopping that of lead 1. 6. Work, work, work... 7. You go to tab 1 and see that timer as running (it is not, but you didn't refresh). You hit stop. Before this commit, it just seemed like the timer was actually stopped. What did happen behind the scenes is that your view was refreshed, but no timer was touched fortunately. After this commit, you get a message telling you that there's no timer to stop and that your browser is most likely out of sync. This mimics the behavior previously found when doing the same, but directly in the AAL. Now it's present in leads too.
Models related to timesheet time controls now inherit from a mixin that adds most needed logic automatically. This requires the changes introduced in OCA/project#596.
- README and manifest adapted - Views changed
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: timesheet-16.0/timesheet-16.0-crm_timesheet Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-crm_timesheet/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: timesheet-16.0/timesheet-16.0-crm_timesheet Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-crm_timesheet/
Currently translated at 100.0% (11 of 11 strings) Translation: timesheet-16.0/timesheet-16.0-crm_timesheet Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-crm_timesheet/it/
Currently translated at 100.0% (11 of 11 strings) Translation: timesheet-16.0/timesheet-16.0-crm_timesheet Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-crm_timesheet/es/
Currently translated at 100.0% (11 of 11 strings) Translation: timesheet-16.0/timesheet-16.0-crm_timesheet Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-crm_timesheet/pt_BR/
[IMP]crm_timesheet: make column optional
666d2dd
to
d8aff0d
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.
Functional Review: LGTM 👍🏻
@manuelregidor Can you review this pls? |
crm_timesheet/__manifest__.py
Outdated
"name": "CRM Timesheet", | ||
"category": "Customer Relationship Management", | ||
"version": "17.0.1.0.0", | ||
"depends": ["crm", "project_timesheet_time_control", "sale_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.
Why is the sale_timesheet dependency necessary in this version? Thank you
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.
Without this dependency, the tests fail because they do not find the billing_type field.
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've just tried locally and you don't need that dependency. You have added the billing_type field when creating project1 in test and I don't think that you should have done that (note that the field is not mentioned in v16 test file).
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.
Deleted
a490e29
to
6600531
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.
Technical review. LGTM
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.
/ocabot migration crm_timesheet
/ocabot merge nobump
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at e7c7d73. Thanks a lot for contributing to OCA. ❤️ |
Standard migration
T-6446
@HaraldPanten
@ValentinVinagre
@luis-ron