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

11.0 mig stock move location WIP #522

Closed
wants to merge 10 commits into from

Conversation

mpanarin
Copy link

@mpanarin mpanarin commented Jan 2, 2019

WIP of migration and refactoring for the old module with additional functionality

  • fix known issue with reservations
  • improve readme info
  • add docs
  • get tests up

@pedrobaeza pedrobaeza added this to the 11.0 milestone Jan 2, 2019
@pedrobaeza pedrobaeza mentioned this pull request Jan 2, 2019
46 tasks
@mpanarin mpanarin force-pushed the 11.0-mig-stock_move_location branch 2 times, most recently from 565365e to e90fd70 Compare January 4, 2019 10:14
stock_move_location/models/stock_inventory.py Outdated Show resolved Hide resolved
stock_move_location/tests/test_common.py Show resolved Hide resolved
stock_move_location/wizard/stock_move_location.py Outdated Show resolved Hide resolved

product_data = []
for group in grouped_quants_data:
quants = quants_obj.search(group.get("__domain"))
Copy link
Contributor

Choose a reason for hiding this comment

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

seems that quants is needed only for the sum: can't you select it straight instead of using sum+mapped?

stock_move_location/wizard/stock_move_location.py Outdated Show resolved Hide resolved
def add_lines(self):
self.ensure_one()
if not self.stock_move_locaiton_line_ids:
for line_val in self._get_stock_move_location_lines_values():
Copy link
Contributor

Choose a reason for hiding this comment

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

how many records do we expect to create potentially? This can be reaaaaally heavy w/ all the writes and the computations after hand. We could do a multiple insert via SQL then trigger recompute on newly created records.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mpanarin any answer to this?

Copy link
Author

Choose a reason for hiding this comment

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

To be honest - I don't know about the number of lines. That depends on the workflow of the end user.

I minimized the amount of creates. So here - only lines of the wizards are created and nothing more. As well as i switched from inventories to pickings, which are less heavy.

Switching to sql inserts is not worth it in my opinion anyway :\

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@simahawk I think this operation is not an usual one. If your stock locations are fine tuned (just some product or just one in a same location), the solution is quite acceptable

def set_inventory_lines(self, inventory):
inventory_line_obj = self.env["stock.inventory.line"]
for line_vals in self._get_inventory_lines_values(inventory):
inventory_line_obj.create(line_vals)
Copy link
Contributor

Choose a reason for hiding this comment

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

same

return available_qty
return line.move_quantity

def _get_quants(self, line):
Copy link
Contributor

Choose a reason for hiding this comment

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

again: can't you just select the sum?

stock_move_location/wizard/stock_move_location_line.py Outdated Show resolved Hide resolved
@mpanarin mpanarin force-pushed the 11.0-mig-stock_move_location branch from 8cbd95b to 939e76a Compare January 9, 2019 00:07
'max_quantity': group.get("sum"),
'origin_location_id': self.origin_location_id.id,
'destination_location_id': self.destination_location_id.id,
# cursor returns None instead of False
Copy link
Contributor

Choose a reason for hiding this comment

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

None is fine anyway

def add_lines(self):
self.ensure_one()
if not self.stock_move_locaiton_line_ids:
for line_val in self._get_stock_move_location_lines_values():
Copy link
Contributor

Choose a reason for hiding this comment

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

@mpanarin any answer to this?

Copy link
Contributor

@Tonow-c2c Tonow-c2c left a comment

Choose a reason for hiding this comment

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

Just answer to @simahawk question and remove the README.rst and for me it's ok

stock_move_location/tests/test_common.py Outdated Show resolved Hide resolved
def _get_stock_move_location_lines_values(self):
product_obj = self.env['product.product']

# Using sql as search_group doesn't support aggregation functions
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@mpanarin You can use the read_group function. The aggregation function by default for float fields is 'sum'. See https://github.com/odoo/odoo/blob/9246375b25f4e90000bb6e578aceb8a116dfc28a/odoo/fields.py#L1182

FROM stock_quant
WHERE location_id = {location_id} AND company_id = {company_id}
GROUP BY product_id, lot_id
""".format(
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@mpanarin If you use cr.execute, pass parameters as second argument of the function.

@rousseldenis
Copy link
Sponsor Contributor

@mpanarin I'm not sure if the implementation is the right one.

I would have done something different like :

  • A new parameter on stock picking types : 'Product Change Location' (with a little help).
  • With this, you can go to the dashboard, create a picking with that type.
  • Add a button which is visible with that type that fill in the picking as you did.

So, the flow respect the standard one and maybe is more intuitive for the user.

Moreover, you can add a magic button on locations that with context creates a new picking of that type with the origin location already filled in.

What do you think about ?

for record in self:
if (float_compare(
record.move_quantity,
record.max_quantity, 3
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

@mpanarin You cannot hardcode rounding. As it concerns moves, I think you should use product uom rounding.

@sebalix
Copy link
Contributor

sebalix commented Jan 25, 2019

@rousseldenis I continue the work started by @mpanarin . It has been stated that your proposal is the good approach, but we are lacking of time to do it, so we think to keep it as it is for now, I added a roadmap with your suggestions.
I made another PR by rebasing/squashing current work: #535

@rousseldenis
Copy link
Sponsor Contributor

@mpanarin Can we close this ?

@sebalix
Copy link
Contributor

sebalix commented Jan 25, 2019

@rousseldenis you can, @mpanarin is on leave for a few days :)

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

Successfully merging this pull request may close these issues.

None yet

7 participants