-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
[ADD] pos_stock_available_online: Add new module #922
Conversation
b43575a
to
942fcd6
Compare
deb2f4b
to
f5f3cca
Compare
f5f3cca
to
5a6ecf0
Compare
5a6ecf0
to
8ba138a
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.
Functional is ok. Need to add a note regarding same company warehouses though.
|
||
def _process_pos_ui_product_product(self, products): | ||
config = self.config_id | ||
if config.display_product_quantity: |
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.
return early and save a identation level
if config.display_product_quantity: | |
if not config.display_product_quantity: | |
return super()._process_pos_ui_product_product(products) |
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.
Hello, @hugho-ad the negative condition is more difficult to read code I see no reason to change in this way
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 it's not, is called early return pattern
https://medium.com/swlh/return-early-pattern-3d18a41bba8
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.
@hugho-ad we have to return "super" in any case. In your case, you offer to return only if this setting is not specified, which is not true.
class ResConfigSettings(models.TransientModel): | ||
_inherit = "res.config.settings" | ||
|
||
pos_display_product_quantity = fields.Boolean( |
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.
res.configs.settings are per company
this fields should be on pos.config form view instead
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.
starting with Odoo 16 all settings specific to POS are defined in this model, you can check the original Odoo code. https://github.com/odoo/odoo/blob/b0df10c1e497f65b891f7141055e7f339d11b4b1/addons/point_of_sale/models/res_config_settings.py#L48-L105
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.
awesome, I didn't know that
thanks!
("config_id.display_product_quantity", "=", True), | ||
"|", | ||
("config_id.additional_warehouse_ids", "in", [warehouse_id]), | ||
("config_id.main_warehouse_id", "=", warehouse_id), |
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.
also quant.product in producs inside categories setup on the pos.config?
("config_id.main_warehouse_id", "=", warehouse_id), | ||
], | ||
).mapped("config_id") | ||
if configs: |
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.
you got a for loop inside the _notify_available_quantity, if not record on the set, then will not perform any
if configs: | |
configs._notify_available_quantity(quant._prepare_pos_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.
this check is added to prevent the _prepare_pos_message
method for the current quant from triggering unnecessarily if no config is found.
format_quantity(quantity) { | ||
const unit = this.env.pos.units_by_id[this.props.product.uom_id[0]]; | ||
var formattedQuantity = `${quantity}`; | ||
if (unit) { |
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.
if (unit) { | |
if (!unit) return `${formattedQuantity}`; |
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.
We should use no-negative condition: https://eslint.org/docs/latest/rules/no-negated-condition
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.
nice! there is a eslint about that!
const ProductIds = []; | ||
for (const payload of payloads) { | ||
for (const message of payload) { | ||
var product = db.get_product_by_id(message.product_id); |
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.
you could avoid this if you sent from backend only notifications used on the pos.config
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.
If a new product is added to the POS, this product will not be available in the POS until the POS product is read again (for example, until the page is updated), this check is necessary so that the POS does not process information about the product that is available in the POS but not actually displayed (because the pos db is not yet updated)
@@ -0,0 +1,43 @@ | |||
/** @odoo-module **/ |
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.
use
odoo.define'module.class', function (require) {
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.
also rename the fiele
Productitem.esm.js > Productitem_esm.js
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 see no reason to use the old approach of defining modules when there is OWL and the new approach, which also will not create problems in porting
- Why? the file with this extension will be picked up by the OCA .eslint rule for ESM modules.:
see https://github.com/OCA/pos/blob/16.0/.eslintrc.yml#L11
and [16.0] [MIG] pos_order_remove_line: migration to 16.0 #925 (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.
@GabbasovDinar thanks man, I need and update with v16.0
Hello @GabbasovDinar thanks for the contribution! awsome! how can we help you with the latest changes? |
Thanks for the contribution, the code looks okay to me, but I have some doubts about the UX, it looks very cluttered ActualProposalThis is a suggestion of UX changes I tried, changing a few things. Feel free to follow it or not, depending on general feedback. It would look like that: Changes made:Use the Warehouse class StockWarehouse(models.Model):
_inherit = "stock.warehouse"
def _prepare_vals_for_pos(self, product):
"""
Prepare warehouse info data to send a POS
"""
self.ensure_one()
return {
"id": self.id,
"name": self.name,
"code": self.code,
"quantity": product.with_context(warehouse=self.id).immediately_usable_qty,
"product_id": product.id,
} I changed the view with:
<t
t-name="ProductItem"
t-inherit="point_of_sale.ProductItem"
t-inherit-mode="extension"
owl="1"
>
<xpath expr="//div[hasclass('product-content')]" position="inside">
<div t-if="env.pos.config.display_product_quantity" class="warehouse-info">
<div class="flex">
<div t-if="warehouses.length > 1" class="warehouse total" title="Total">
<i class="fa fa-dropbox"/>
<span
t-attf-class="quantity {{total_quantity > env.pos.config.minimum_product_quantity_alert ? 'available' : 'not-available'}}"
>
<t t-esc="display_total_quantity" />
</span>
</div>
</div>
<div class="flex">
<t t-foreach="warehouses" t-as="warehouse" t-key="warehouse.id">
<div class="warehouse" t-att-title="warehouse.name">
<span class="warehouse-name">
<t t-esc="warehouse.code" />
</span>
<span
t-attf-class="quantity {{warehouse.quantity > env.pos.config.minimum_product_quantity_alert ? 'available' : 'not-available'}}"
>
<t t-esc="format_quantity(warehouse.quantity)" />
</span>
</div>
</t>
</div>
</div>
</xpath>
</t> Removed the absolute positioning and made use of flex to display elements in a more compact way: .pos .product-list .warehouse-info {
padding: 0 6px;
font-weight: bold;
display: flex;
border-top: 1px solid #efefef;
justify-content: space-between;
}
.pos .product-list .warehouse-info .warehouse {
display: flex;
flex-direction: row;
justify-content: space-between;
align-items: center;
}
.pos .product-list .warehouse-info .warehouse .quantity {
color: black;
padding: 1px 2px;
font-size: 11px;
}
.pos .product-list .warehouse-info .warehouse .warehouse-name {
display: block;
color: #696969;
font-size: 10px;
}
.pos .product-list .warehouse-info .warehouse.total .warehouse-name {
font-weight: bold;
}
.pos .product-list .warehouse-info .warehouse .quantity.available {
color: #32a868;
}
.pos .product-list .warehouse-info .warehouse .quantity.not-available {
color: #ef5350;
} |
8ba138a
to
7dc740d
Compare
@PhilDL Thanks for your suggestions, I applied these changes and it looks much better now |
what do you think? ready to merge? |
d6f6551
to
cb3fdf5
Compare
80fd992
to
5ed44b8
Compare
…lability with POS [FIX] fixed pre-commit errors
5ed44b8
to
f3be4c0
Compare
This PR has the |
@OCA/pos-maintainers could you please have a look into this PR? |
Why don't you use the Info tab ? You can display what you want without overloading the POS view.
|
Which info tab do you mean? Could you please provide some picture? |
I don't know if I'm missing something, but this is something already featured in: https://github.com/OCA/pos/tree/16.0/pos_product_quick_info (I think that's what @gregory-moka meant @isserver1 ) |
Yup. This is also our module that we have contributed earlier. However the current one has completely different approach. |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 55e2ef9. Thanks a lot for contributing to OCA. ❤️ |
Show the available quantity of products in the Point of Sale