-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[10.0][ADD] Add sale_project_fixed_price_task_completed_invoicing #485
[10.0][ADD] Add sale_project_fixed_price_task_completed_invoicing #485
Conversation
3b5488e
to
84cfae1
Compare
def toggle_invoiceable(self): | ||
for task in self: | ||
# We dont' want to modify when the related SOLine is invoiced | ||
if not task.sale_line_id or task.sale_line_id.state in ('done'): |
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.
in ('done', )
missing training comma. Probably 'cancel' should be handled in the same way.
for task in self: | ||
# We dont' want to modify when the related SOLine is invoiced | ||
if not task.sale_line_id or task.sale_line_id.state in ('done'): | ||
continue |
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.
raise a UserError to give the user some feedback.
SOLine = self.env['sale.order.line'] | ||
so_line = SOLine.browse(vals.get('sale_line_id')) | ||
# We don't want to add a project.task to an already invoiced line | ||
if so_line and so_line.state in ('done'): |
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.
('done', 'cancel')
def write(self, vals): | ||
for task in self: | ||
if (vals.get('sale_line_id') and | ||
task.sale_line_id.state in ('done')): |
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.
('done', 'cancel')
line.product_uom_qty != 1.0): | ||
raise ValidationError('Error! You cannot have more than one ' | ||
'"track_service" sold in one Sale Order ' | ||
'Line') |
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.
error message must be translatable
_('The quantity for 'Complete Task' products must be exactly one')
@api.constrains('product_id') | ||
def _onchange_product_id(self): | ||
for line in self: | ||
if ('track_service' == line.product_id.track_service and |
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.
'complete_task' ?
54c7ca8
to
f14b1bd
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.
Overall LGTM, but: some indentation error and above all, any tests?
def _need_procurement(self): | ||
for product in self: | ||
if (product.type == 'service' and | ||
product.track_service == 'completed_task'): |
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.
fix indentation
for task in self: | ||
# We dont' want to modify when the related SOLine is invoiced | ||
if (not task.sale_line_id or | ||
task.sale_line_id.state in ('done', 'cancel')): |
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.
fix indentation
def write(self, vals): | ||
for task in self: | ||
if (vals.get('sale_line_id') and | ||
task.sale_line_id.state in ('done', 'cancel')): |
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.
again
f14b1bd
to
c8b8583
Compare
c8b8583
to
66af3bd
Compare
BTW I'd say that "sale_project_fixed_price" would be enough for module name :) |
44b83ea
to
920fd90
Compare
@pedrobaeza Hello, this is a base new module. The base code is set and the next days I'm building tests. Is is possible to have a first review/opinion about it? |
Well, not these days due to the Spanish Odoo days and the big queue I have after the code sprint, and it's not a current interest, but I suggest meanwhile to fix bots. |
f4a7892
to
7198e0a
Compare
7198e0a
to
93ea2e5
Compare
In order to fix tests in local I had to change a module in project OCA/project#286 |
cd6670a
to
1448f8e
Compare
_inherit = 'sale.order.line' | ||
|
||
@api.model | ||
def create(self, vals): |
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.
What's append if you create sale.order.line and then update product? and updated product has diferent track_service
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.
@angelmoya it works fine. This bit is only when you create a line in state 'sale' and not draft, although I'm not quite sure when this can happen. @leemannd can you enlighten us?
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.
@angelmoya It takes the logic from sale_timesheet
and overrides it in order to make it work with the new selection in line.product_id.track_service
You'll find there the commit from odoo implementing it. odoo/odoo@9013074
Before this revision, a small function in sale was extended by
sale_timesheet to know when to (or not to) create an analytic
account for a SO.
This was nice, but since it was extended only in this module,
there's no real need for the added complexity (*reading
complexity*)
@rafaelbn Unless I'm mistaken, with the setting you propose, all the lines will be flagged as invoiceable in that setting, which means that when the sales try to create an invoice, all the lines with products invoiced based on sold quantity are there, regardless of the "delivered quantity" (i.e. the hours set on the timesheet). Another difference is that with this the quantity sold and the hours on the timesheets are disconnected. Say you are selling a "milestone" with UOM = Unit and qty = 1 for a fixed price. It is annoying to have the qty delivered set to 100 (implicitely 'hours') on the sale order line. I agree with you about the unsetting of 'invoiceable' when the related line has not yet been invoiced. @leemannd can you change this? |
@rafaelbn @angelmoya thanks for taking the time to review the PR, by the way. |
Fist of all, I would like to help because I'm sales manager in Tecnativa and I have same headache as the one who need this solution. "Invoice sales order when the task are flagged to be invoiced independently the quantity". As I'm also project manager, I really would like to get for OCA a solution with a general approach! 🙏 🤞
Hi @gurneyalex , you are mistaken 😄 . Each sale order line is related with a task so only will be flagged as invoiceable the lines of the task that the "task user" has flagged. If you have 4 sale order lines, then you have 4 tasks and maybe you only set 1 of 4 to invoiceable.
I really wish that you are getting my idea in this point to simplify. Trust me I have all cases 😭
YES! You can send a quotation of 20 hours or just 1 unit of Training (maybe 20, maybe 30 hours) This case in my side is (but of course could be improved in Odoo/OCA):
I open to make a hangout or similar with you to explain it if you consider it 😃 I just would like to cover this need as simple as possible Thanks! |
@rafaelbn @gurneyalex You can now unset 'invoiceable' on tasks when the sale.order.line is not shipped. |
…or better reusability
1448f8e
to
80c99c9
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.
(only code review)
Can you look at this approach? |
@sergio-teruel I looked at it and seems pretty nice work. As I don't have the full specs from @gurneyalex I must wait for his opinion about it. |
@sergio-teruel I gave your module a look. I see two issues there:
A typical example could be : you have a milestone for a project which was delivered, in a QA instance, and your customer is taking a lot of time to validate the milestone. In this case you could have the task not done (it is not in production), but you still want to invoice the work. |
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.
Thanks for the great work. From a business point of view, I approve the functionality.
@gurneyalex Ok, I understand and I have made changes, can you take a look? Thanks. |
@gurneyalex @pedrobaeza Hello, as reviews are done, is it possible to have this PR merged? Thank you. |
Will we finally have two modules to cover the same need? |
I discussed this with @rafaelbn and @sergio-teruel => #500 suits my needs and covers an additional use case I don't have but they do => I'll close this PR in favor of #500 once I've updated the customers who need it. |
Can we finally close this? |
@pedrobaeza Yes, I'm closing it |
Main Goal
The main goal of this module is to add the possibility to link a sale.order.line
to a project.task considering the delivery.
Unless sale_timesheet, the quantity shipped won't be linked to a Timesheet nor
to the time spent on the task. The price is fixed on the sale.order.line and it
will be considered as shipped once the task is accomplished.
sale.order.line
. Thesale.order.line
is going to be delivered once the task is accomplished.Nota Bene
project_timesheet_analytic_partner
from oca project when I launched tests in local.