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

[18.0][MIG] stock_vertical_lift: Migration to 18.0 #2255

Open
wants to merge 56 commits into
base: 18.0
Choose a base branch
from

Conversation

LucasTran380381
Copy link

@LucasTran380381 LucasTran380381 commented Feb 4, 2025

Depend

#2257

Put from receipt

  • create receipt to shuttle
    image
  • Switch shuttle to put mode -> Insert barcode product and tray code -> Save to tray location
    image
  • Receipt change state to done
    image

Pick from Delivery

  • Create Delivery
    image
  • Switch Shuttle to Pick mode -> Insert barcode destination -> Save
    image
  • Delivery change state to done
    image

Adjust Inventory

  • Set stock.quant allow to adjust
    image
  • Input real quantity
    image
  • Inventory stock.quant has adjusted
    image

guewen and others added 30 commits January 8, 2025 10:06
* Add vertical_lift_shuttle_id field on stock.location, help to find the
shuttle for a location
* Add StockLocation.fetch_vertical_lift_tray(), that needs to be
implemented in addons to send commands to the hardward to fetch a tray,
and if existing show a cell (laser pointer, ...)
* Add helpers on stock.move.line fetch_vertical_lift_tray_source() and
fetch_vertical_lift_tray_dest() that fetch the tray directly from a move
line's source or destination location
Namely, the pick/put/inventory operations are now split in
different models.

Pick and Put share a model and customize their behavior, which is pretty
similar. The inventory operation will have a different view and
different workflow.

This changes will ease a lot the customization of the different
workflows and views.
When we refresh the page on the browser when we are using the "screen"
view, odoo loses the information that we want the view to be headless,
fullscreen, etc. so it's displayed pretty badly.  This view is a
work-around: its priority is lower, so it will be picked up by default
on loading, and a button allows to re-open the screen view with the
proper options.
There is no such action as 'ir.actions.do_nothing', it kinda works,
until you look into the js console and stares at the errors.

There is a nice OCA module that serves this purpose (more or less,
because it reloads the window, this is not an issue).
Example of usage in an odoo shell, when a screen is open:

>>> self.env['vertical.lift.shuttle'].browse(1)._operation_for_mode().operation_descr = 'foo'
>>> self.env['vertical.lift.shuttle'].browse(1)._send_notification_refresh()
>>> env.cr.commit()

Provided the longpolling is correctly configured with a proxy, the
screen should immediately refresh with 'foo' as operation description.
* The change of destination location was not updated on the screen when
  the barcode was scanned (it was when the "manual barcode wizard" is
  used though)
* We should be able to pick partially available move lines
* prevent to scan a location when no move line is selected or the move
  line has already been set to done
Instead of going through the onchange machinery.
The intended usage of onchange methods is to update something on the
screen, without side-effects in the database, then let the user save
the form with the proposed changes.

Weirdly, the barcode scanner event triggers an onchange on the field
`_barcode_scanned`.

It doesn't work well with our use case, as the whole form is read-only
and we only care about having the barcode events doing side-effects on
the backend and displaying back the changes.

This particular onchange will then be executed as a normal method, with
side-effects. However, contrarily to other actions on the form, the
frontend does not reload the view after an onchange, as it relies on the
data returned back in the values. As we cannot know which values may
have been changed in the different implementations (location
destination, state, ...), the onchange returns a read with every field.
The documentation of the state machine is in
VerticalLiftOperationBase._transitions.
Compatibility module between stock_vertical_lift and stock_storage_type
(in OCA/wms).

In the vertical lift's Putaway screen, when a good is scanned for a putaway, the
user has to scan the tray type of the corresponding size, so an empty place in a
matching tray is found. When we use storage types, we should know what tray is
compatible with the storage type.

Changes with this module:

* The storage types of trays cannot be selected in the locations form, they have
  to be set in the Tray types.
* In the lift put-away screen, when a package has a storage type, the user isn't
  asked to scan a tray type, instead, the putaway of the Package Storage Type is
  applied.
When the shuttle screen propose a tray based on a tray type and we
are in the 'save' step, where we are supposed to physically putaway
the good and save, we should still be able to change the tray type
to fetch another tray.
* Rename methods that fetch a tray to prevent confusion
* Add methods to release a tray
* The Kardex method to fetch a tray has to send "0" in the carrier and
  carrierNext field
* The pick and inventory screens release the tray only when there is no next
  line, because the release is implicit when we fetch the next line,
  the put screen releases everytime because the operator may take time
  to start the next line and we don't know if they are going to scan a
  next line or not.
* Exiting the screen or switching screen between put/pick/put-away has
  to release the tray as well.
As the template is not used by JS we can pass full objects to it.
This way we can use any recordset information directly in the template
without having to override the method.
* stock_vertical_lift: make pkg compute more solid

Somehow sometimes you can get a move line without product
while computing product packaging in inventory.

Make it more defensive and skip packaging rendering if no product is
there.
The check was means as an optimization: no need to fetch at tray already
open. But "fetch_tray" will not only open the tray, it may also move the
laser on the exact position. So  we should do it for every inventory line.
If we have several goods to put in the same tray, it is inefficient to
release (close) the tray between each line if we reopen the same tray.
Release the tray only when the last line is reached.
OCA-git-bot and others added 13 commits January 8, 2025 10:06
Currently translated at 7.1% (9 of 126 strings)

Translation: stock-logistics-warehouse-14.0/stock-logistics-warehouse-14.0-stock_vertical_lift
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-14-0/stock-logistics-warehouse-14-0-stock_vertical_lift/it/
If the same user opens multiple shuttles operations on different screens,
notifications for a specific shuttle will be displayed on all of them,
regardless of the shuttle operation that triggered the notification.

This commit allows filtering notifications per screen, so that only
screens displaying shuttle operations related to the current notifications
will display them.

Notifications that are not shuttle-related are not filtered out.
@rousseldenis
Copy link
Contributor

/ocabot migration stock_vertical_lift

Copy link

@henrybackman henrybackman left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a couple of comments

<span
class="packaging_name"
itemprop="name"
t-esc="packaging.name"

Choose a reason for hiding this comment

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

# give the unique shuttle for any location in the tree (whether it's a
# shuttle, a tray or a cell)
inverse_vertical_lift_shuttle_ids = fields.One2many(
comodel_name="vertical.lift.shuttle", inverse_name="location_id", readonly=True

Choose a reason for hiding this comment

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

tray_type_code = fields.Char(compute="_compute_tray_data", string="Tray Code")
tray_x = fields.Integer(string="X", compute="_compute_tray_data")
tray_y = fields.Integer(string="Y", compute="_compute_tray_data")
tray_matrix = Serialized(string="Cells", compute="_compute_tray_data")
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we can just use the Json field and drop the base_sparse_field dependency

class VerticalLiftController(http.Controller):
@http.route(["/vertical-lift"], type="http", auth="public", csrf=False)
def vertical_lift(self, answer, secret):
if secret == self._get_env_secret():
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 This seems unsafe.

I know this change isn't introduced by the migration, but we could take the opportunity to improve this

I don't see this VERTICAL_LIFT_SECRET variable documented anywhere, and the default is just an unsecured endpoint

IMO we should check this secret is set, and fail with a message explaining how to set it.
If an unprotected (no secret) option wants to be given, IMO it should be explicit (e.g.: by setting the secret to False, for example)

Additionally, it'd be better to not rely on an environment variable for this secret. Some environments like Odoo.sh don't allow customizing these variables. Instead, we should read this from an ir.config_parameter instead. The config parameter itself can optionally be configured through environment with the usage of https://github.com/OCA/server-env/tree/18.0/server_environment_ir_config_parameter (no direct dependency needed)

Copy link
Author

Choose a reason for hiding this comment

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

improved to read secret by config_param

Comment on lines 20 to 22
# We reload mainly because otherwise, it would close
# the popup. This action is provided by the OCA module
# web_ir_actions_act_view_reload
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# We reload mainly because otherwise, it would close
# the popup. This action is provided by the OCA module
# web_ir_actions_act_view_reload

misleading/outdated comment. In fact soft_reload is a core feature now, that module is deprecated

Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Thanks, minor comments

_logger.error("secret is not set")
msg = self.env._(
"Vertical Lift secret not set. "
"Pls set secret in Inventory/Settings/Vertical Lift"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Pls set secret in Inventory/Settings/Vertical Lift"
"Please set it in Inventory/Settings/Vertical Lift"

.get_param("stock_vertical_lift.secret", None)
)
if not secret:
_logger.error("secret is not set")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please a little bit more verbose with this message 🙏🏻
Mention also how to configure it.
You can probably use the same message than the exception

Copy link
Author

Choose a reason for hiding this comment

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

I use the same message than the exception to make it more verbose. Thanks for suggestion

@LucasTran380381 LucasTran380381 force-pushed the 18.0-mig-stock_vertical_lift branch from 42a0bbb to 07350dd Compare February 20, 2025 02:42
Copy link
Contributor

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks 👍

Couple small things to check, not blocking.

"move_line_ids": [],
"backorder_id": picking.id,
}
)
new_picking.message_post(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing this message?

@@ -351,7 +343,9 @@ def _send_notification_refresh(self):
"action": "refresh",
"params": self._get_user_notification_params(),
}
self.env["bus.bus"].sendone(channel, bus_message)
self.env["bus.listener.mixin"]._bus_send(
Copy link
Contributor

Choose a reason for hiding this comment

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

self.env.user._bus_send(

Looks weird to call the function on the mixin

Copy link

@Kimkhoi3010 Kimkhoi3010 Mar 11, 2025

Choose a reason for hiding this comment

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

Hi @grindtildeath, You can take a look bus.py #Line102
This docstring explains a low-level method used to send notification_type and message to a specific target. It recommends using _bus_send() from bus.listener.mixin for better simplicity and security. When using _sendone() directly, it's important to ensure that the target (if it's a string) is not guessable by an attacker to prevent unauthorized access.

Comment on lines 149 to 150
line_model = self.env["stock.quant"]
record.number_of_ops = line_model.search_count(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to rename to quant_model if it's not an inventory line anymore.

Comment on lines 185 to 190
float_compare(
line.theoretical_qty,
stock_quant.quantity,
self.quantity_input,
precision_rounding=line.product_uom_id.rounding,
precision_rounding=stock_quant.product_uom_id.rounding,
)
== 0
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use float_is_zero there

stock_quant = self.quant_id
if not stock_quant.vertical_lift_done:
stock_quant.vertical_lift_done = True
if self.quantity_input != stock_quant.inventory_quantity:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use float_compare

@Kimkhoi3010 Kimkhoi3010 force-pushed the 18.0-mig-stock_vertical_lift branch from 07350dd to ba36402 Compare March 11, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.