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

[9.0] [ADD] sell loose goods using tare barcode label #447

Merged
merged 27 commits into from Feb 14, 2020

Conversation

Fkawala
Copy link
Contributor

@Fkawala Fkawala commented Jan 28, 2020

This add-on enables POS to read and print tare barcodes. We print a barcode tare label to sell loose goods in a Bring Your Own pot (BYOC) scheme.

The BYOC scheme has five steps:

  1. The cashier weighs the pot and sticks the tare barcode onto the customer's pot.
  2. The customer go and put loose goods into the labeled pot.
  3. The cashier weighs the pot with loose goods inside. POS computes the price including the pot.
  4. The cashier scans the tare barcode. POS get the pot weight from the barcode. POS subtracts the pot weight from the weight of the latest product. POS sets the billable price for the loose goods.
  5. The customer pays.

This add-on adds a news screen to POS to print (web) the tare barcode labels. This add-on enables POS to read a tare barcode. Reading a barcode makes POS adjust the weight of the latest article in the order list. The new weight is equal to the total weight minus the tare weight. The price is updated accordingly to the weight change.

@Fkawala
Copy link
Contributor Author

Fkawala commented Jan 28, 2020

Hello @robinkeunen, hello @legalsylvain, would you please have time to give a look at this PR? Thx!

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.

thanks a lot for sharing this module.

some comments inline. no important blockings points.

FYI, I wrote a module for V12, named pos_tare. Complementary features.
maybe we could imagine to merge that feature in a second step.

could you take a look, and make a review ?
Thanks !
#436

pos_barcode_tare/static/src/js/pos_barcode_tare.js Outdated Show resolved Hide resolved
pos_barcode_tare/static/src/js/pos_barcode_tare.js Outdated Show resolved Hide resolved
pos_barcode_tare/models/pos_config.py Outdated Show resolved Hide resolved
pos_barcode_tare/static/src/js/pos_barcode_tare.js Outdated Show resolved Hide resolved
pos_barcode_tare/static/src/js/pos_barcode_tare.js Outdated Show resolved Hide resolved
pos_barcode_tare/readme/DESCRIPTION.rst Outdated Show resolved Hide resolved
@robinkeunen
Copy link

robinkeunen commented Jan 29, 2020

@vvrossem @houssine78 fyi

@Fkawala Fkawala changed the title [9.0] [ADD] sell loose good using tare barcode label [9.0] [ADD] sell loose goods using tare barcode label Jan 29, 2020
@Fkawala
Copy link
Contributor Author

Fkawala commented Jan 29, 2020

Hello @legalsylvain,

Thank you for the thoughtful review! I'll (try to) do the same for #436.

I agree with you #447 and #436 are complementary and should merge at some point!

Thank you,
François

Hi @Fkawala.

thanks a lot for sharing this module.

some comments inline. no important blockings points.

FYI, I wrote a module for V12, named pos_tare. Complementary features.
maybe we could imagine to merge that feature in a second step.

could you take a look, and make a review ?
Thanks !
#436

@Fkawala
Copy link
Contributor Author

Fkawala commented Jan 30, 2020

Howdy @robinkeunen @vvrossem @houssine78 a review would be much appreciated.

@houssine78
Copy link
Sponsor

Hi @Fkawala Thanks for this very nice contribution. I don't know this POS part enough to give relevant comment. However the whole module seems to be good. I didn't went through the js code sorry. @vvrossem will probably give you a better review than mine.

@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 4, 2020

Hi @legalsylvain, would you have time to take a look? I updated the code using the UOM as you do in #436. You'll need to define a "Kg" UOM if you plan to test that using the debug module. The UOM matching is case sensitive and the debug panel sets the weight to "Kg" whereas it should be "kg" (see https://github.com/odoo/odoo/blob/9.0/addons/point_of_sale/static/src/js/devices.js#L380).

@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 4, 2020

Hi @robinkeunen! I would love to get that addon running in our shop. This addon would massively speed up the checkout process. This is one of our highest pain-point right now. Would you mind to check if someone from coopITEasycould can review this? Thank you in advance!

@vvrossem
Copy link

vvrossem commented Feb 4, 2020

Hi @legalsylvain, would you have time to take a look? I updated the code using the UOM as you do in #436. You'll need to define a "Kg" UOM if you plan to test that using the debug module. The UOM matching is case sensitive and the debug panel sets the weight to "Kg" whereas it should be "kg" (see https://github.com/odoo/odoo/blob/9.0/addons/point_of_sale/static/src/js/devices.js#L380).

Hi @Fkawala, could you please explain me what happens if you put 'kg' instead of 'Kg' ?
Many thanks

@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 4, 2020

@vvrossem, thank you for looking into this PR. Please find a more detail explanation below.

How the Addon works?

The add-on saves weight on the tare label using the "kg" unit of measure (UOM). This UOM should exist for the add-on to work. The UOM is fetched from the POS and matched by its name (case sensitive). See here and there for the code responsible to find the UOM based on UOM name.

When the electronic scale sends the weight, it comes with the unit name, for instance, "kg". The add-on fetches the UOM corresponding to the unit name. The UOM that matches the unit name sent by the scale is the "scale UOM". The add-on converts the scale weight from the scale UOM to the "kg" UOM. At this point, we're ready to create a tare label. When we read the tare label, we read a tare weight saved in "kg" UOM. We convert that tare weight into the product UOM so that we can compute the net weight in the product UOM.

In summary, we have three UOM:

  • Product UOM (fetch from the last product in order list)
  • Tare label UOM (which is always equal to the default "kg" reference UOM)
  • Electronic scale UOM (matched from the unit name sent by the electronic scale)

Today all the electronic scales that are supported (ADAM / Mettler-Toledo) use the "kg" UOM. However, this architecture would allow us to use an electronic scale that use another UOM, as long as this UOM is defined.

What can be the problem in debug?

My note on the "Kg" UOM is there because of the particular behavior of the "debug panel". As you can see here the debug pannel forges weights with a non-standard unit name "Kg". If you do run this code using the debug panel to moke the electronic scale, then the add-on will search for the "Kg" UOM. This UOM does not exist by default. Hence the add-on would fail to create a tare label.

I hope this is clear enough, if not, please let me know!

@legalsylvain
Copy link
Contributor

I'll add the demo data (ie. missing UOM) that's a good idea.

nice !

How should we proceed to merge both our modules?
Would you be willing to do the migration work to get the result working on odoo 12.0?

i propose to :

  1. merge this PR in 9.0, if you consider it as finished. 9.0 serie is quite old (unsupported recently)

  2. integrate on [12.0] [ADD] pos_tare #436 the feature present in this module. do you have a deadline ? I'd like to do it, but I'm currently quite busy in the next weeks.

kind regards.

@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 13, 2020 via email

@legalsylvain
Copy link
Contributor

@PierrickBrun could you take a time to make a little functional review on that PR ?
Thanks !

@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 14, 2020

Hello @legalsylvain and @PierrickBrun. This is ready to be reviewed and hopefully merged. Thank you in advance!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 14, 2020

Thanks for the review @PierrickBrun! Could we proceed forward and merge this? Best regards

@legalsylvain
Copy link
Contributor

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 9.0-ocabot-merge-pr-447-by-legalsylvain-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Feb 14, 2020
Signed-off-by legalsylvain
@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 14, 2020

Thank you @legalsylvain !

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.

Hi @Fkawala ! First of all, thank you for your work !
I've tested and reviewed the module. I have minor suggestions that can be added in the roadmap if the community agrees :)

barcode_tare_action: function (code) {
try {
var order = this.pos.get_order();
var last_order_line = order.get_last_orderline();

Choose a reason for hiding this comment

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

I suggest to replace order.get_last_orderline() with order.get_selected_orderline(). It allows the cashier to scan the tare of a pot after another orderline has been added :
order get_selected_orderline

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 agree this would be quite more flexible!

return self.pos.proxy.scale_read().then(function (weight) {
try {
self.set_weight(weight);
} catch (error) {

Choose a reason for hiding this comment

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

could the syntax scale_read().then(function (onFulfilled){...}, function (onRejected){...}) be an alternative for UOM management? scale_read in point_of_sale.devices uses it

Copy link
Contributor Author

@Fkawala Fkawala Feb 14, 2020

Choose a reason for hiding this comment

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

I do not get the rationale here, could you please explain it to me? Thank you.

Choose a reason for hiding this comment

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

On a first opinion, I was thinking it would be better to move the UOM management in an overridden scale_read function, but my second opinion is that it's better to handle this on the TareScreenWidget and not in the ProxyDevice


The command line to start a chrome base browser in kiosk mode with silent printing looks like:

``chromium-browser --use-system-default-printer --kiosk --kiosk-printing http://localhost:8069/``

Choose a reason for hiding this comment

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

I would also add a Configuration section explaining to check the show tare label button checkbox in Loose good options for the functional users

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 agree this can help and it is fairly straightforward.

<li class="info">
tare = <t t-esc="line.get_tare_str_with_unit()" />
</li>
</t>

Choose a reason for hiding this comment

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

Since the tare and net weight is specified on the orderline, I wonder if the gross weight should be included as well for legal reasons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add it, it does not cost much. But I think this would be less readable. I'm not too worried about legal compliance since the whole POS scaling hasn't been certified (at least not in UE).

@OCA-git-bot OCA-git-bot merged commit d0ca9b9 into OCA:9.0 Feb 14, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at ccb42d6. Thanks a lot for contributing to OCA. ❤️

@OCA-git-bot OCA-git-bot changed the title [9.0] [ADD] sell loose goods using tare barcode label [9.0] [ADD] sell loose goods using tare barcode label Feb 14, 2020
@Fkawala
Copy link
Contributor Author

Fkawala commented Feb 14, 2020

@vvrossem What would the procedure to ship the changes you suggested to do (now that this PR has been merged)? Thanks!

@legalsylvain
Copy link
Contributor

@vvrossem could you make a new PR agains 9.0 with all the nice improvments you propose ?
Thanks

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.

None yet

7 participants