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

[12.0][MIG] pos_tare_generate_barcode #527

Closed
wants to merge 6 commits into from

Conversation

Fkawala
Copy link
Contributor

@Fkawala Fkawala commented Aug 27, 2020

This PR contains the refactorized migration of the tare barcode generator. This work follows on PR #491 as decided by @legalsylvain and myself here #436 and there #447. This new add-on depends on pos_tare (PR #491).

This PR adds a new screen to the POS. The new screen enables to web-print barcode labels of tare values. The tare value is measured using the POS scale. A capture of the new screen below (no change since v9.0 / PR #447).

ready_to_print

cc. @houssine78 @vvrossem

@OCA-git-bot
Copy link
Contributor

Hi @legalsylvain, @Fkawala,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Hi @Fkawala. Just tested on runbot.

when I click on the button, and if the setting is not correctly set, you have loop on the error, and cannot go out of the screen.

image

(quick review only.)

pos_tare/i18n/fr_CH.po Show resolved Hide resolved
@Fkawala
Copy link
Contributor Author

Fkawala commented Aug 31, 2020

Hi @Fkawala. Just tested on runbot.

when I click on the button, and if the setting is not correctly set, you have loop on the error, and cannot go out of the screen.

image

(quick review only.)

Hi @legalsylvain,

This is a known problem/bug of the POS debug tool. The fake scale sends weight measures in an unexisting UOM "Kg", whereas the POS weight reference UOM is "kg" in lowercase. I'll fix the error loop problem.

Best regards,
François

ps. thank you for your note regarding the translation!

@Fkawala
Copy link
Contributor Author

Fkawala commented Nov 3, 2020

Hey, @remytms would you have 5 minutes to review that? Sylvain was OK with this PR, aside from the looping error that is now fixed. Cheers, François

@houssine78
Copy link
Sponsor

@Fkawala you probably wanted to ping @vvrossem on this PR for review.

<div class="centered-content">
<div class="pos-tare-label-container"></div>
<div class="pos-directions-for-user">
Set the pot on the scale and check the weight above
Copy link
Sponsor

Choose a reason for hiding this comment

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

I would rather use container instead of pot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change and I do not really care, "container" is more generic whereas "pot" is more specific to food/cooking.

<div class="pos-tare-paper">
<div class="pos-tare-label">
<img t-att-src="'/report/barcode/EAN13/' + widget.get_barcode_data()" />
<span class="caption">tare = <t t-esc="widget.get_tare_weight()" />kg</span>
Copy link
Sponsor

Choose a reason for hiding this comment

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

kg is hard coded.

Potential issue would be that the tare is on g and not kg. Any way to have uom if the tare configured somewhere ?

second point here is that it limits the use of this module on the kg metric system.

Copy link
Sponsor

Choose a reason for hiding this comment

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

I see that my proposition could to some uom conversion between the tare of the container and the uom of the contained product (not sure of that).

I agree to go ahead without change if you add a section explaining the limitation in the issue/limitation of the readme

Copy link
Sponsor

Choose a reason for hiding this comment

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

ok no issue with conversion, I just read the js code... and saw that you use the uom returned by the scale which is gr for our metric system. only limitation here is it doesn't support UK/US metric system. I'm fine with it !

},
set_tare_weight: function (scale_measure) {
var weight = scale_measure.weight;
var measure_unit = this.pos.units.filter(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@houssine78 We pick up the scale UOM from there. A scale using the Imperial scale would not be a problem there as long as the UOM exists.

if (weight > 0) {
var tare_uom = this.pos.config.iface_tare_uom_id[0];
var tare_unit = this.pos.units_by_id[tare_uom];
this.weight_in_tare_unit = convert_mass(weight,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@houssine78 The tare UOM is a parameter of the POS. Here we convert the weight expressed in the scale's UOM into the parameterized UOM.

}
return this.weight_in_tare_unit;
},
barcode_data: function (weight) {
Copy link
Contributor Author

@Fkawala Fkawala Nov 9, 2020

Choose a reason for hiding this comment

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

@houssine78 Here we might have a problem b/c we only consider weight to be in the international standard. We can not store the UOM in the label. We do not want the old labels to become obsolete if the user changes the UOM configuration. Hence we should stick to international standard mass units but fix the display to be in the configured UOM. The current solution would probably break with the Imperial scale. I'll try to propose a fix.

@Fkawala
Copy link
Contributor Author

Fkawala commented Nov 10, 2020

@houssine78 this solves the point you raised about UOM used in the barcode label. @vvrossem Could you please have a look? Thank you!

@Fkawala
Copy link
Contributor Author

Fkawala commented Nov 11, 2020

@legalsylvain Besides fixing the tare barcode UOM problem, I introduced an additional parameter in the pos_tare module. This parameter enables users to configure if they want to display the tare input field on the scale screen.

The rationale supporting this parameter is as follows. In our shop, we use Adam scales. Those scales do not feature a reset signal, hence odoo needs to reset the weight after <x> seconds to avoid having the operator ordering twice the same weight for two different products. A consequence of odoo resetting the weight after <x> seconds is that our cooperators do not have enough time to use the tare input field on the scale screen before reset happens. One could argue that we should increase <x> but that'll increase the risk of ordering with the wrong weight.

Therefore, to reduce the cognitive load of the operator using the scale screen, we need to hide the input tare field (until we have better scales)

I hope all that is clear. Let me know if not.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

partial review regarding pos_tare module.

pos_tare/models/pos_config.py Outdated Show resolved Hide resolved
var tare_unit = this.pos.units_by_id[tare_uom];
var kilogram_uom = this.pos.units.filter(
function (u) {
return u.name === "kg";
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry. I still don't understand why there are hardcoded text regarding uom in this module. there is no way to have something more modular ? if it's not possible, do you think you could write a section in the ROADMAP section, to mention to do it (maybe when odoo PoS will be refactored...)

Copy link
Contributor Author

@Fkawala Fkawala Nov 20, 2020

Choose a reason for hiding this comment

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

Assume you do not hard code this UOM (the one that is used to encode the tare barcode labels). Let's say this UOM is configured to be kilogram. Now print a tare label with 1 unit of UOM, that's one kilogram. Assume the user changes the configuration for this UOM and selects "metric ton". Afterwards one scans the old tare barcode label, she'll get a tare of one metric ton instead of the correct weight of the container that is one kilogram. If you have a better solution than hard coding, I'm happy to change that. Hope this is clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@legalsylvain What's your opinion on that? Are we all good?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. Back on that point. I was fully busy the last monthes.

I think " Assume the user changes the configuration for this UOM and selects "metric ton" can not occure.

I tested your Use case with demo data in V12 :

  • select the product with kilogram unit (Cable Management Box) (stock.product_cable_management_box)
  • change the quantity (70kg in hand).
  • select the uom kg (uom.product_uom_kgm)
  • try to change uom.

You'll have this errors.

image

My opinion is that odoo doesn't handle that use case. (that is quite hard to handle, I think). so, you should not have to handle that use case in a OCA module, if Odoo core module doesn't allow it.
It will make the code simpler.
Dont you think ?

Copy link

@vvrossem vvrossem left a comment

Choose a reason for hiding this comment

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

Thank your work on this module :)
My apologies for the very late review. Here are some remarks.

  1. the duration of your proxy_queue might create issues on the scale software-side

  2. In standard POS, Odoo implements set_weight() (in ScaleScreenWidget) as:

    set_weight: function(weight){
        this.weight = weight;
        this.$('.weight').text(this.get_product_weight_string());
        this.$('.computed-price').text(this.get_computed_price_string());
    },

where get_product_weight_string() returns a defaultstr (e.g. 0.000 Kg) in specific cases

If not already done, I think set_tare_weight() could use the same idea:

  • weight should be what the scale returns
  • get_tare_weight_string() should return a default value in case of missing UoM

self.pos.proxy_queue.clear();
}
});
}, {duration:150, repeat: true});
Copy link

Choose a reason for hiding this comment

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

There must be at least a 200-ms delay between commands to allow for processing data response time at the scale.

150 ms might be too low if, on the scale software-side, no protocol.commandDelay is used between weight readings

}
});
},
render_receipt: function () {
Copy link

Choose a reason for hiding this comment

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

a detail, but if you render PosTareLabel, you can name your function render_label

<div class="pos-center-align">
<div class="pos-tare-paper">
<div class="pos-tare-label">
<img t-att-src="'/report/barcode/EAN13/' + widget.get_barcode_data()" />
Copy link

Choose a reason for hiding this comment

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

TIL you can generate barcode that way 🎉

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 22, 2021
@github-actions github-actions bot closed this Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants