Skip to content
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

[16.0][FIX] pos_stock_available_online: stock move updates #1250

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

danielduqma
Copy link
Contributor

Make stock updates not only on quant updates, but also when a stock move changes its state.

This fixes two bugs:

FL-556-4139

Copy link
Contributor

@miguelcb2003 miguelcb2003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@danielduqma danielduqma marked this pull request as ready for review October 2, 2024 07:53
Copy link

@Pablocce Pablocce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@GabbasovDinar GabbasovDinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielduqma Thank you for your contribution, please check out the small comments

_name = "stock.notifier.pos.mixin"
_description = "Stock Notifier POS Mixin"

def _prepare_pos_message(self, warehouse=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _prepare_pos_message is called directly in the warehouse loop in the _notify_pos method, which gets the list of warehouses using the _get_warehouses_to_notify method in this case warehouse will always be in the argument values, so it can be left as a mandatory argument.

def _prepare_pos_message(self, warehouse):
     """
     Return prepared message to send to POS
     """
     self.ensure_one()
     return warehouse._prepare_vals_for_pos(self.product_id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this was a first approach but the logic was moved to _get_warehouses_to_notify


def _get_warehouses_to_notify(self):
self.ensure_one()
return self.warehouse_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the current model is a mixin and can be plugged into any model this field may be missing which will cause an error, possible solution is to either return the model object return self.env["stock.warehouse"] or throw an exception raise NotImplementedError()

In the models where this mixin is connected, implement a method that returns the necessary warehouses

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mixin was intended just to be inherited by stock.move and stock.quant, but I modified it as you proposed to be more extendable

@sheragar
Copy link

sheragar commented Nov 8, 2024

LGTM

Make stock updates not only on quant updates, but also when a stock
move changes its state.

This fixes two bugs:

- Stock is not updated when a new incoming move is ready, or when a
  ready one is cancelled

- Stock is decreased twice because when quant is updated, move is
  taken in account because it is not yet on 'done' state
@danielduqma danielduqma force-pushed the 16.0-fix-pos_stock_available_online branch from 47225e9 to c9675a5 Compare November 11, 2024 13:01
Copy link
Member

@GabbasovDinar GabbasovDinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants