-
Notifications
You must be signed in to change notification settings - Fork 30.6k
[IMP] purchase, sale, stock: update dates logic/UX #221380
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
base: master
Are you sure you want to change the base?
Conversation
2383031 to
1fcd43d
Compare
4d9eccc to
32ddbff
Compare
32ddbff to
5d33dd3
Compare
b62ad05 to
a50776b
Compare
287d4a2 to
7b392a3
Compare
5e9690c to
bb6768e
Compare
clesgow
left a comment
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.
Still not done, but here's a first part
| @@ -33,6 +33,8 @@ def _ondelete_stock_moves(self): | |||
| forecasted_issue = fields.Boolean(compute='_compute_forecasted_issue') | |||
| is_storable = fields.Boolean(related='product_id.is_storable') | |||
| location_final_id = fields.Many2one('stock.location', 'Location from procurement') | |||
| date_promised = fields.Datetime('Promised Date', readonly=False, store=True, | |||
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_promised = fields.Datetime('Promised Date', readonly=False, store=True, | |
| date_promised = fields.Datetime('Promised Date', |
Non-computed fields are by default readonly=False and store=True
| new_date = fields.Datetime.to_datetime(values['date_promised']) | ||
| for move in self.move_ids: | ||
| if move.picking_id.picking_type_code == 'incoming': | ||
| self.filtered(lambda line: not line.display_type)._update_move_dates(date_promised=new_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.
update_move_dates already iterates on specific moves to update. I don't think you need to repeat that for every move_ids linked to the PO 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.
I believe I changed this because it caused a traceback randomly while I was testing, There was a use case where move was not just one, If I remember correctly 🤞.
I will double check.
Edit: I will just remove the check of picking type and it will get checked in the method anyways
|
|
||
| def write(self, vals): | ||
| res = super().write(vals) | ||
| if vals.get('date') and vals.get('state') != "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.
What if the date gets updated but the state isn't changed (thus not in the 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.
None != "done". Therefore, the condition will evaluate to True and date planned will be set accordingly.
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 just don't want the purchase orders dates to be updated when the receipt is validated, to avoid this bug and keep the original dates.
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.
None != "done". Therefore, the condition will evaluate to True and date planned will be set accordingly.
But what if the state is already done at that point? 😄
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 goes into _set_date_planned and checks for the state and if its done or cancelled nothing will happen
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.
Indeed.
However, you're already past super().write(vals) at that point, which means self contains the updated records. I think it's fine to check if date was in the vals, but imho you can just call set_date_planned(), and not bother to check the change in state here.
| if vals.get('date_planned'): | ||
| pickings = self.picking_ids.filtered(lambda p: p.state not in ['done', 'cancel'] and p.picking_type_code == "incoming") | ||
| pickings.scheduled_date = vals['date_planned'] |
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.
Is this necessary?
When you update the date_planned of the PO through the form:
- It will update the
date_plannedof the PO lines - Which will update the
dateof the related moves - Which will trigger the recompute of
_compute_scheduled_dateas one of its depends changed.
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.
Good catch 👌 Its not necessary 🙂
b3e2b96 to
e39d525
Compare
clesgow
left a comment
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.
Getting much closer! 👍
Most of the comments are formatting stuff
| help="* Red: Late\n" | ||
| "* Grey: Pending\n" | ||
| "* Blue: Partially Received\n" | ||
| "* Green: On time") |
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.
| lines.filtered(lambda l: l.order_id.state == 'purchase')._create_or_update_picking() | ||
| if lines.order_id.state == 'purchase': | ||
| lines._set_date_promised() |
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.
| lines.filtered(lambda l: l.order_id.state == 'purchase')._create_or_update_picking() | |
| if lines.order_id.state == 'purchase': | |
| lines._set_date_promised() | |
| confirmed_lines = lines.filtered(lambda l: l.order_id.state == 'purchase') | |
| confirmed_lines._create_or_update_picking() | |
| confirmed_lines._set_date_promised() |
lines could refer to lines from different POs (nothing forbids it). In that case, you'd have a singleton error in your condition. We can simply re-use the filtered already done above.
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 check
| @@ -17,6 +17,21 @@ class StockMove(models.Model): | |||
| 'purchase.order.line', 'stock_move_created_purchase_line_rel', | |||
| 'move_id', 'created_purchase_line_id', 'Created Purchase Order Lines', copy=False) | |||
|
|
|||
| def _set_date_planned(self, date_planned): | |||
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.
These two methods should be defined lower, according to our coding guidelines.
And yup, the file is already a mess, but let's not add to it, shall we? 😅
|
|
||
| def write(self, vals): | ||
| res = super().write(vals) | ||
| if vals.get('date') and vals.get('state') != "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.
Indeed.
However, you're already past super().write(vals) at that point, which means self contains the updated records. I think it's fine to check if date was in the vals, but imho you can just call set_date_planned(), and not bother to check the change in state here.
| move = move.with_context(date_planned_set_ids=already_set_ids) | ||
| if move.picking_id.state in ('done', 'cancel') or move.purchase_line_id.id in already_set_ids or move.picking_id.picking_type_code != 'incoming': | ||
| continue | ||
| already_set_ids.update(move.purchase_line_id.ids) | ||
| move.purchase_line_id.date_planned = date_planned |
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.
| move = move.with_context(date_planned_set_ids=already_set_ids) | |
| if move.picking_id.state in ('done', 'cancel') or move.purchase_line_id.id in already_set_ids or move.picking_id.picking_type_code != 'incoming': | |
| continue | |
| already_set_ids.update(move.purchase_line_id.ids) | |
| move.purchase_line_id.date_planned = date_planned | |
| if move.picking_id.state in ('done', 'cancel') or move.purchase_line_id.id in already_set_ids or move.picking_id.picking_type_code != 'incoming': | |
| continue | |
| already_set_ids.update(move.purchase_line_id.ids) | |
| move.with_context(date_planned_set_ids=already_set_ids) | |
| .purchase_line_id.date_planned = date_planned |
Nitpick
| @@ -210,6 +210,12 @@ def _compute_json_popover(self): | |||
| }) | |||
| order.show_json_popover = bool(late_stock_picking) | |||
|
|
|||
| def action_confirm(self): | |||
| for order in self: | |||
| if order.expected_date and not order.commitment_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.
As both fields are defined in sale, I don't think this should be done in sale_stock 🤔
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.
expected_date is redefined in sale_stock with a different help message
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.
Which doesn't change its purpose 🙂
addons/stock/models/stock_picking.py
Outdated
| @@ -592,7 +592,7 @@ def _default_picking_type_id(self): | |||
| PROCUREMENT_PRIORITIES, string='Priority', default='0', | |||
| help="Products will be reserved first for the transfers with the highest priorities.") | |||
| scheduled_date = fields.Datetime( | |||
| 'Scheduled Date', compute='_compute_scheduled_date', inverse='_set_scheduled_date', store=True, | |||
| 'Scheduled Date', compute='_compute_scheduled_date', inverse="_set_scheduled_date", store=True, | |||
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.
| 'Scheduled Date', compute='_compute_scheduled_date', inverse="_set_scheduled_date", store=True, | |
| 'Scheduled Date', compute='_compute_scheduled_date', inverse='_set_scheduled_date', store=True, |
Nope 👀
| self.assertEqual(incoming_shipment1.date_deadline, old_deadline1 + timedelta(days=1), 'Deadline should be propagate') | ||
| self.assertEqual(incoming_shipment2.date_deadline, old_deadline2, 'Deadline should remain unchanged') | ||
| self.assertEqual(incoming_shipment1.date_deadline, old_deadline1, 'Deadline should remain unchanged') | ||
| self.assertEqual(purchase.picking_ids.scheduled_date, purchase.date_planned, 'Scheduled Date should propagate') |
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.
| self.assertEqual(purchase.picking_ids.scheduled_date, purchase.date_planned, 'Scheduled Date should propagate') | |
| self.assertEqual(purchase.order_line.move_ids.scheduled_date, purchase.date_planned, 'Scheduled Date should propagate') |
I wouldn't rely on this, as it's kind of a side-effect that picking_ids doesn't contain the two other steps. I'd rather target the IN picking directly to avoid issues in the future.
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.
wouldn't that possibly cause a singleton traceback? given that move_ids is one2many
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 only throw the traceback if there are more than 1 move_id records, and if there are in this test, then you should test the date of each one.
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 only throw the traceback if there are more than 1 move_id records, and if there are in this test, then you should test the date of each one.
In this test, no its only one move, I'll just add it then but it will be purchase.order_line.move_ids.picking_id.scheduled_date
| self.assertEqual(incoming_shipment2.date_deadline, old_deadline2, 'Deadline should remain unchanged') | ||
| self.assertEqual(incoming_shipment1.date_deadline, old_deadline1, 'Deadline should remain unchanged') |
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.
Seems weird to me that we don't have any kind of date propagation now 🤔
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_planned only updates scheduled dates, deadlines are only updated by date_promised
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.
Yeah, but IIRC when I tested, the scheduled dates didn't move either.
And maybe you should now test that.
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.
Not sure what do you mean by the dates weren't propagated, the next assert right after these 2 checks if date_planned was propagated or not
521da4b to
3a1a6d1
Compare
3a1a6d1 to
2bdc050
Compare
ticodoo
left a comment
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.
Couple of small nitpicks, will look more/test tomorrow
| @@ -89,14 +94,18 @@ def _compute_forecasted_issue(self): | |||
| @api.model_create_multi | |||
| def create(self, vals_list): | |||
| lines = super().create(vals_list) | |||
| lines.filtered(lambda l: l.order_id.state == 'purchase')._create_or_update_picking() | |||
| confirmed_lines = lines.filtered(lambda l: l.order_id.state == 'purchase') | |||
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.
maybe this should also filter out display_type != False lines?
| def _set_date_planned(self, date_planned): | ||
| already_set_ids = self.env.context.get('date_planned_set_ids', set()) | ||
| for move in self: | ||
| if move.picking_id.state in ('done', 'cancel') or move.purchase_line_id.id in already_set_ids or move.picking_id.picking_type_code != 'incoming': |
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.
Small optimization, take advantage of short-circuiting:
| if move.picking_id.state in ('done', 'cancel') or move.purchase_line_id.id in already_set_ids or move.picking_id.picking_type_code != 'incoming': | |
| if move.picking_id.picking_type_code != 'incoming' or move.picking_id.state in ('done', 'cancel') move.purchase_line_id.id in already_set_ids or : |
this way the other conditions aren't checked for any moves for incoming pickings
53ccb51 to
4f1d9c5
Compare
ticodoo
left a comment
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'd also update your commit title too to be:
[IMP] purchase, sale, stock: update dates logic/UX
| for order in self: | ||
| dates_list = order.order_line.filtered(lambda line: not line.display_type and line.date_promised).mapped('date_promised') | ||
| if dates_list and order.state in ('purchase', 'cancel'): | ||
| order.date_promised = min(dates_list) | ||
| else: | ||
| order.date_promised = False |
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.
Slightly cleaner/more efficient
| for order in self: | |
| dates_list = order.order_line.filtered(lambda line: not line.display_type and line.date_promised).mapped('date_promised') | |
| if dates_list and order.state in ('purchase', 'cancel'): | |
| order.date_promised = min(dates_list) | |
| else: | |
| order.date_promised = False | |
| for order in self: | |
| if order.state not in ('purchase', 'cancel'): | |
| order.date_promised = False | |
| continue | |
| dates_list = order.order_line.filtered(lambda line: not line.display_type and line.date_promised).mapped('date_promised') | |
| order.date_promised = min(dates_list) if dates_list else False |
But I also don't understand why you're using the min date rather than the max date, since we would expect the vendor would promise the delivery date for ALL goods, not for the first good to be delivered. Also couldn't see where this is specified in the spec
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 wasn't specified in the spec if it should be any different than date_planned, for me I mentioned before that even expected arrival (date_planned) should show the maximum date and arm told me they did this because of some tickets and if a customer needs it to be different they can customize it? but for me It makes sense to have max for both of the dates.
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.
The description of date_promised should probably be updated to reflect that then because right now it's written in a way where it looks like the date everything should be delivered by. Maybe something like:
Date promised by the vendor for at least 1 or more products to be delivered by
| self.assertEqual(incoming_shipment1.date_deadline, old_deadline1 + timedelta(days=1), 'Deadline should be propagate') | ||
| self.assertEqual(incoming_shipment2.date_deadline, old_deadline2, 'Deadline should remain unchanged') | ||
| self.assertEqual(incoming_shipment1.date_deadline, old_deadline1, 'Deadline should remain unchanged') | ||
| self.assertEqual(purchase.picking_ids.scheduled_date, purchase.date_planned, 'Scheduled Date should propagate') |
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 only throw the traceback if there are more than 1 move_id records, and if there are in this test, then you should test the date of each one.
| Green: On time") | ||
| ], string='Receipt Status', compute='_compute_receipt_status', store=True) | ||
| date_promised = fields.Datetime('Promised Date', index=True, copy=False, compute="_compute_date_promised", store=True, readonly=False, | ||
| help="Delivery Date promised by the vendor. If the vendor delivers products after this date, their On-Time rate will be negatively impacted.") |
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.
This is inaccurate since the On-Time rate is calculated per POL date_promise. It should be more specific, but what the wording should be depends on my comment in the _compute_date_promised method
d4f9d82 to
a9fe242
Compare
ticodoo
left a comment
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's also the "Sender Reminder" (email) action that's selectable in POs that should be checked.
There also used to be a feature where vendors could update the date_planned that looks like was lost during some portal refactorings, but the code is still there:
odoo/addons/purchase/controllers/portal.py
Lines 167 to 194 in b1e1fd0
| def portal_my_purchase_order_update_dates(self, order_id=None, access_token=None, **kw): | |
| """User update scheduled date on purchase order line. | |
| """ | |
| try: | |
| order_sudo = self._document_check_access('purchase.order', order_id, access_token=access_token) | |
| except (AccessError, MissingError): | |
| return request.redirect('/my') | |
| updated_dates = [] | |
| for id_str, date_str in kw.items(): | |
| try: | |
| line_id = int(id_str) | |
| except ValueError: | |
| return request.redirect(order_sudo.get_portal_url()) | |
| line = order_sudo.order_line.filtered(lambda l: l.id == line_id) | |
| if not line: | |
| return request.redirect(order_sudo.get_portal_url()) | |
| try: | |
| updated_date = line._convert_to_middle_of_day(datetime.strptime(date_str, '%Y-%m-%d')) | |
| except ValueError: | |
| continue | |
| updated_dates.append((line, updated_date)) | |
| if updated_dates: | |
| order_sudo._update_date_planned_for_lines(updated_dates) | |
| return Response(status=204) |
If we still want the feature, then we should fix it + check which "date" the vendor should be able to update. If we don't, then we should remove the code since it may not make sense for 3rd party customizations anymore
c6472a7 to
a50a876
Compare
|
@robodoo r+ |
To avoid confusion after confirming an RFQ: - New field has been added 'Promised Date' in Purchase Order and Purchase Order Line. - 'Promised Date' updates the deadline on a receipt. - 'Promised Date' only shows after confirming an RFQ, taking the place of 'Confirmation Date'. - Confirmation Date was moved to 'Other information' section. - Updating "Expected Arrival" on PO or PO line updates the date scheduled on a receipt and no longer updates the deadline. - Updated "Expected Arrival" help message to adapt to the change. In Sales Order: - 'Delivery Date' now uses a dynamic placeholder giving it a cleaner look. - On Confirmation, if no date is set it will be assigned to the placeholder's value and 'Delivery Date' label is changed to 'Promised Delivery'. - Some UI Changes. Task: 4687135
a50a876 to
5862f83
Compare
|
@robodoo r+ |
|
@abyo-odoo @ticodoo linked pull request(s) odoo/upgrade#8687 not ready. Linked PRs are not staged until all of them are ready. |

To avoid confusion after confirming an RFQ:
to stock move scheduled date (and the other way around), and promised date propagates to deadlines.
In Sales Order:
Task: 4687135
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr