Skip to content

[ADD] 14.0 pos_product_multi_barcode#768

Merged
OCA-git-bot merged 2 commits into
OCA:14.0from
akretion:14.0-add-pos_product_multi_barcode
May 31, 2022
Merged

[ADD] 14.0 pos_product_multi_barcode#768
OCA-git-bot merged 2 commits into
OCA:14.0from
akretion:14.0-add-pos_product_multi_barcode

Conversation

@PierrickBrun
Copy link
Copy Markdown
Contributor

@PierrickBrun PierrickBrun commented Mar 22, 2022

@PierrickBrun PierrickBrun force-pushed the 14.0-add-pos_product_multi_barcode branch 3 times, most recently from 8f12ec6 to cd7d227 Compare March 22, 2022 12:29
Copy link
Copy Markdown

@petrus-v petrus-v left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, Code review only.

Strange travis is failing, I don't think it's related to this PR.

Probably some for loop could be replaced with forEach but this is only sugar syntax so LGTM 👍

@francesco-ooops
Copy link
Copy Markdown
Contributor

@PierrickBrun can you rebase for testing?

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

/ocabot rebase

@OCA-git-bot
Copy link
Copy Markdown
Contributor

Sorry @PierrickBrun you are not allowed to rebase.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

Copy link
Copy Markdown
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

It didn't seem to be working for me

STEPS TO REPRODUCE

Add 2 barcodes to same product
Scan both barcodes in POS
1st barcode scanned correctly
2nd barcode I get standard behavior:

image

@PierrickBrun PierrickBrun force-pushed the 14.0-add-pos_product_multi_barcode branch from cd7d227 to 9247aea Compare May 17, 2022 13:45
@PierrickBrun
Copy link
Copy Markdown
Contributor Author

It didn't seem to be working for me

STEPS TO REPRODUCE

Add 2 barcodes to same product Scan both barcodes in POS 1st barcode scanned correctly 2nd barcode I get standard behavior:

image

Thanks for the detailed report, I will look into it shortly

@francesco-ooops
Copy link
Copy Markdown
Contributor

It didn't seem to be working for me
STEPS TO REPRODUCE
Add 2 barcodes to same product Scan both barcodes in POS 1st barcode scanned correctly 2nd barcode I get standard behavior:
image

Thanks for the detailed report, I will look into it shortly

Thank you, let us know if you're short on time and you need a hand :)

@PierrickBrun PierrickBrun force-pushed the 14.0-add-pos_product_multi_barcode branch from 9247aea to 54c1f2a Compare May 20, 2022 14:05
@francesco-ooops
Copy link
Copy Markdown
Contributor

@PierrickBrun tried after last commit, same behavior as per screenshot above

@elvise
Copy link
Copy Markdown

elvise commented May 25, 2022

@PierrickBrun thanks for this PR!
Do you have any plan to fix this issue ?

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

Hi @elvise @francesco-ooops

I've looked into it, apparently the problem was that depending on the modules installed, barcodes where loaded before the products (and it was then impossible to use product_by_id).

Because there was no way of controlling the loading order I changed all the logic and used the same logic as pos_supplierinfo_barcode

@elvise
Copy link
Copy Markdown

elvise commented May 25, 2022

Hi @elvise @francesco-ooops

I've looked into it, apparently the problem was that depending on the modules installed, barcodes where loaded before the products (and it was then impossible to use product_by_id).

Because there was no way of controlling the loading order I changed all the logic and used the same logic as pos_supplierinfo_barcode

Great! Let me check

@francesco-ooops
Copy link
Copy Markdown
Contributor

@PierrickBrun with current version, added two barcodes to a product, but none of them is detected when scanned in POS

image

@elvise
Copy link
Copy Markdown

elvise commented May 27, 2022

@PierrickBrun with current version, added two barcodes to a product, but none of them is detected when scanned in POS

image

@PierrickBrun good news for this case ?

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

@PierrickBrun with current version, added two barcodes to a product, but none of them is detected when scanned in POS

image

I did the same test via runboat (and on my dev env obviously) and it all worked well.

Do you test using an actual barcode scanner ? I did it using the debug tool so maybe this is linked to a difference between the prod/debug mode of the pos

@elvise
Copy link
Copy Markdown

elvise commented May 27, 2022

@PierrickBrun with current version, added two barcodes to a product, but none of them is detected when scanned in POS
image

I did the same test via runboat (and on my dev env obviously) and it all worked well.

Do you test using an actual barcode scanner ? I did it using the debug tool so maybe this is linked to a difference between the prod/debug mode of the pos

Thanks for your reply!
@francesco-ooops please check it

Copy link
Copy Markdown
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Found the issue, I was scanning a barcode that was reserved for cashier in barcode nomenclature:

image

So it's all good for me! Functional review ok

@francesco-ooops
Copy link
Copy Markdown
Contributor

@OCA/pos-maintainers what do you say? :)

@elvise
Copy link
Copy Markdown

elvise commented May 27, 2022

@PierrickBrun with current version, added two barcodes to a product, but none of them is detected when scanned in POS
image

I did the same test via runboat (and on my dev env obviously) and it all worked well.

Do you test using an actual barcode scanner ? I did it using the debug tool so maybe this is linked to a difference between the prod/debug mode of the pos

@PierrickBrun Thanks for your support!

@elvise
Copy link
Copy Markdown

elvise commented May 29, 2022

@OCA/pos-maintainers merge ? :)

@legalsylvain
Copy link
Copy Markdown
Contributor

legalsylvain commented May 29, 2022

@PierrickBrun :

  • could you squach fixup commit ?
  • do you want to set you as maintainers ?

@elvise
Copy link
Copy Markdown

elvise commented May 30, 2022

@PierrickBrun just gentleman reminder :)

@PierrickBrun PierrickBrun force-pushed the 14.0-add-pos_product_multi_barcode branch from d65b1a9 to 968561b Compare May 30, 2022 09:24
@elvise
Copy link
Copy Markdown

elvise commented May 30, 2022

@legalsylvain take a look :)

@legalsylvain
Copy link
Copy Markdown
Contributor

No review.

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-768-by-legalsylvain-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 31, 2022
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Copy Markdown
Contributor

It looks like something changed on 14.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 14.0-ocabot-merge-pr-768-by-legalsylvain-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4894058 into OCA:14.0 May 31, 2022
@OCA-git-bot
Copy link
Copy Markdown
Contributor

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

@francesco-ooops
Copy link
Copy Markdown
Contributor

hi @PierrickBrun , I tried the module on runboat for pos-14 but the module does not seem to be working anymore, is it only me? or could you take a look?

It's strange, as it doesn't seem anything was merged on pos-14 after this PR 🤔

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

Hi @francesco-ooops ,
I did just test it and it worked, were you in the 14.0 branch ? I think that in the runboat you were, the module was not installed

@francesco-ooops
Copy link
Copy Markdown
Contributor

Hi @francesco-ooops , I did just test it and it worked, were you in the 14.0 branch ? I think that in the runboat you were, the module was not installed

would you be so kind to link runboat and tell me which product you used for testing?
Somehow I was unable to make it work both on runboat and locally

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

PierrickBrun commented Jun 14, 2022

Hi @francesco-ooops , I did just test it and it worked, were you in the 14.0 branch ? I think that in the runboat you were, the module was not installed

would you be so kind to link runboat and tell me which product you used for testing? Somehow I was unable to make it work both on runboat and locally

Here is the full URL of the product used: http://oca-pos-14-0-2875765502fd.runboat.odoo-community.org/web#id=15&action=368&model=product.template&view_type=form&cids=&menu_id=224

@francesco-ooops
Copy link
Copy Markdown
Contributor

Hi @francesco-ooops , I did just test it and it worked, were you in the 14.0 branch ? I think that in the runboat you were, the module was not installed

would you be so kind to link runboat and tell me which product you used for testing? Somehow I was unable to make it work both on runboat and locally

Here is the full URL of the product used: http://oca-pos-14-0-2875765502fd.runboat.odoo-community.org/web#id=15&action=368&model=product.template&view_type=form&cids=&menu_id=224

@PierrickBrun this is curious, just tested and secondary barcode doesn't seem to work: https://recordit.co/nyTbAEi5wr

btw I'm copy/pasting the barcode, not using a scanner, do you see anything strange in the video?

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

Hi @francesco-ooops , I did just test it and it worked, were you in the 14.0 branch ? I think that in the runboat you were, the module was not installed

would you be so kind to link runboat and tell me which product you used for testing? Somehow I was unable to make it work both on runboat and locally

Here is the full URL of the product used: http://oca-pos-14-0-2875765502fd.runboat.odoo-community.org/web#id=15&action=368&model=product.template&view_type=form&cids=&menu_id=224

@PierrickBrun this is curious, just tested and secondary barcode doesn't seem to work: https://recordit.co/nyTbAEi5wr

btw I'm copy/pasting the barcode, not using a scanner, do you see anything strange in the video?

Ok I understand now, I never tested via the search bar, only via barcode scanner (or the debugger version of the barcode scanner).

@francesco-ooops
Copy link
Copy Markdown
Contributor

thank you @PierrickBrun , do you think you can make a PR to fix this behavior?

@francesco-ooops
Copy link
Copy Markdown
Contributor

I also asked my colleague in the office to test with barcode scanner and that's not working for us as well https://recordit.co/ifkui6FTgC (test on runboat)

@francesco-ooops
Copy link
Copy Markdown
Contributor

@legalsylvain can I ask you for a functional test of this module? we're struggling to understand why we can't make it work on runbot or locally (and we tested it successfully on runbot before merge)

@legalsylvain
Copy link
Copy Markdown
Contributor

Hi sorry no time for the moment. I trust the contributor and reviewer to move forward on this topic.

@PierrickBrun
Copy link
Copy Markdown
Contributor Author

Hi @francesco-ooops I think I will fix this in a future PR but I do not know when.

About the barcode scanner, you should scan the barcode while not having your cursor in the search bar. That way Odoo will recognise it comes from a barcode scanner and use a different code path.
If you do it like in the video it will only search using the provided text, as if it was typed on a keyboard.

@legalsylvain
Copy link
Copy Markdown
Contributor

Hi @francesco-ooops I think I will fix this in a future PR but I do not know when.

Hi @PierrickBrun. Thanks for your answer. In the meantime, could you just make a simple PR to add this point as a limitation in a ROADMAP.rst file ?

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.

6 participants